Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the Ming model in LLaMA-Factory, a popular fine-tuning framework. The integration enables users to fine-tune the Ming-Lite-Omni multimodal model using LLaMA-Factory's tools and configuration system.
Key changes:
- Adds a comprehensive patch file that integrates Ming model support into LLaMA-Factory
- Provides a YAML configuration file for LoRA-based supervised fine-tuning
- Updates documentation with detailed setup and usage instructions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| ming.patch | Comprehensive patch file adding Ming model support to LLaMA-Factory including multimodal processing, template registration, and model configuration |
| ming_lora_sft.yaml | Configuration file for LoRA-based supervised fine-tuning of Ming model with training parameters and dataset settings |
| README.md | Documentation updates with LLaMA-Factory setup instructions and usage examples, plus a Docker command fix |
Comments suppressed due to low confidence (1)
ming.patch:1
- Large blocks of commented-out code should be removed rather than left in the codebase. If this audio processing functionality is planned for future implementation, consider adding a TODO comment with context instead.
diff --git a/src/llamafactory/chat/hf_engine.py b/src/llamafactory/chat/hf_engine.py
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| plot_loss: true | ||
| overwrite_output_dir: true | ||
| save_only_model: false | ||
| report_to: none # choices: [none, wandb, tensorboard, swanlab, mlflow] |
There was a problem hiding this comment.
The comment incorrectly lists 'swanlab' as a choice. The correct name is 'wandb' for Weights & Biases, but 'swanlab' appears to be a typo - it should likely be 'wandb' or removed if it's not a valid option.
| report_to: none # choices: [none, wandb, tensorboard, swanlab, mlflow] | |
| report_to: none # choices: [none, wandb, tensorboard, mlflow] |
| + # if len(audios) != 0: | ||
| + # sampling_rate = 16000 | ||
| + # audios = self._regularize_audios(audios, sampling_rate=sampling_rate)["audios"] | ||
| + # audios = [(torch.tensor(audio), sampling_rate)for audio in audios] |
There was a problem hiding this comment.
Missing space before 'for' in the list comprehension. Should be [(torch.tensor(audio), sampling_rate) for audio in audios].
| + image_grid_thw = image_inputs["image_grid_thw"] | ||
| + | ||
| + if len(videos): | ||
| + # assert len(videos) <= 1, "Video count must be at most 1!" |
There was a problem hiding this comment.
Commented-out assertion should either be removed or properly implemented if the constraint is still valid. If this limitation exists, it should be enforced through proper validation.
| + # assert len(videos) <= 1, "Video count must be at most 1!" | |
| + if len(videos) > 1: | |
| + raise ValueError("Video count must be at most 1!") |
| + # if len(audios): | ||
| + # audio_inputs = self._get_mm_inputs([], [], audios, processor) | ||
| + # audio_feats_lengths = audio_inputs["encoder_feats_lengths"] |
There was a problem hiding this comment.
Another block of commented-out code that should be removed or properly implemented. This creates inconsistency with the audio processing logic above.
| + # if len(audios): | |
| + # audio_inputs = self._get_mm_inputs([], [], audios, processor) | |
| + # audio_feats_lengths = audio_inputs["encoder_feats_lengths"] | |
| + if len(audios): | |
| + audio_inputs = self._get_mm_inputs([], [], audios, processor) | |
| + audio_feats_lengths = audio_inputs["encoder_feats_lengths"] |
| ├── am.mvn | ||
| ├── audio_detokenizer | ||
| ├── ... | ||
| ├── inclutionAI |
There was a problem hiding this comment.
Typo in directory name - should be 'inclusionAI' to match the actual organization name referenced elsewhere in the documentation.
| ├── inclutionAI | |
| ├── inclusionAI |
No description provided.