[megatron] Support ovis2.5 #5719
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @Jintao-Huang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request integrates the Ovis2.5 multimodal model into the system, extending its capabilities to handle advanced vision-language tasks. The changes involve updating model registration, refining configuration management for multimodal architectures, and adjusting data processing pipelines to correctly interpret and embed visual and textual inputs. Additionally, documentation has been updated to guide users on the new model and address known issues related to precision testing.
Highlights
- Ovis2.5 Model Support: Added comprehensive support for the Ovis2.5 multimodal model within the Megatron framework, including model registration, conversion utilities, and specific handling for its visual components.
- Multimodal Configuration Enhancements: Improved the
HfConfigFactoryto correctly apply configuration attributes, such astorch_dtypeand attention implementation, to vision-related sub-configurations (e.g.,vit_config,vision_config) within multimodal models. - Multimodal Input Processing Refinement: Refactored the
_post_encodemethod in the Qwen template to explicitly manage multimodal inputs, includingpixel_valuesand special indicator tokens, for accurate embedding generation in Ovis2.5. - Padding-Free and Packing Logic Correction: Corrected the logic for
padding_freewhenpackingis enabled, ensuring thatpadding_freeis set toTrueto maintain consistent behavior. - Documentation Updates: Updated both Chinese and English documentation to reflect Ovis2.5 model support, clarify potential
FlashAttentionprecision issues during conversion tests, and remove an outdated note about channel loss and sequence parallelism. - Robust Megatron Embedding Handling: Introduced a patch in
_patch_word_embeddingsto mask negative input IDs to zero, preventing potential issues when processing special tokens in Megatron models.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds support for the ovis2.5 model to Megatron-SWIFT. The changes include updating model registration, templates, and documentation. The core logic for handling ovis2.5's multimodal inputs has been implemented. The code is generally well-structured, but there is a significant code duplication between the Megatron and non-Megatron paths for ovis2.5, which should be refactored to improve maintainability.
| def get_inputs_embeds(self, inputs_embeds, **kwargs): | ||
| model = self._hf_model[0] | ||
| input_ids = kwargs['input_ids'] | ||
| pixel_values = kwargs.get('pixel_values', None) | ||
| grid_thws = kwargs.get('grid_thws') | ||
| INDICATOR_IDS = [-301, -302, -303, -304] | ||
| VISUAL_ATOM_ID = -300 | ||
| device = inputs_embeds.device | ||
| visual_indicator_embeds = self.vte(model.indicator_token_indices.to(device=device)).to( | ||
| dtype=inputs_embeds.dtype, device=device) | ||
| for i, indicator_id in enumerate(INDICATOR_IDS): | ||
| inputs_embeds[input_ids == indicator_id] = visual_indicator_embeds[i] | ||
| if pixel_values is None: | ||
| media_inputs = self.visual_tokenizer.preprocess(Image.new('RGB', (32, 32), (0, 0, 0))) | ||
| media_inputs = to_device(media_inputs, input_ids.device) | ||
| pixel_values = media_inputs['pixel_values'].type(inputs_embeds.dtype) | ||
| visual_tokens = self.visual_tokenizer(pixel_values, media_inputs['grid_thws']) | ||
| visual_embeds = self.vte(visual_tokens).to(dtype=inputs_embeds.dtype, device=inputs_embeds.device) | ||
| inputs_embeds = inputs_embeds + visual_embeds.mean() * 0. | ||
| else: | ||
| visual_tokens = self.visual_tokenizer(pixel_values, grid_thws) | ||
| visual_embeds = self.vte(visual_tokens).to(dtype=inputs_embeds.dtype, device=inputs_embeds.device) | ||
| inputs_embeds[input_ids == VISUAL_ATOM_ID] = visual_embeds | ||
| return inputs_embeds |
There was a problem hiding this comment.
This get_inputs_embeds method has nearly identical logic to Ovis2_5Template._post_encode in swift/llm/template/template/qwen.py. To improve maintainability and avoid code duplication, consider refactoring this shared logic into a common utility function. This function could take the necessary components (e.g., inputs_embeds, input_ids, pixel_values, visual_tokenizer, vte) as arguments.
| def _post_encode(self, model: nn.Module, inputs: Dict[str, Any]) -> Dict[str, Any]: | ||
| inputs_embeds = model.merge_multimodal( | ||
| input_ids=inputs['input_ids'], | ||
| pixel_values=inputs.pop('pixel_values', None), | ||
| grid_thws=inputs.pop('grid_thws', None)) | ||
| input_ids = inputs['input_ids'] | ||
| pixel_values = inputs.get('pixel_values', None) | ||
| grid_thws = inputs.get('grid_thws') | ||
| INDICATOR_IDS = [-301, -302, -303, -304] | ||
| VISUAL_ATOM_ID = -300 | ||
| placeholder_token_mask = torch.lt(input_ids, 0) | ||
| inputs_embeds = model.get_wte()(torch.masked_fill(input_ids, placeholder_token_mask, 0)) | ||
|
|
||
| visual_indicator_embeds = model.vte(model.indicator_token_indices).to( | ||
| dtype=inputs_embeds.dtype, device=inputs_embeds.device) | ||
| for i, indicator_id in enumerate(INDICATOR_IDS): | ||
| inputs_embeds[input_ids == indicator_id] = visual_indicator_embeds[i] | ||
| if pixel_values is None: | ||
| media_inputs = model.visual_tokenizer.preprocess(Image.new('RGB', (32, 32), (0, 0, 0))) | ||
| media_inputs = to_device(media_inputs, input_ids.device) | ||
| pixel_values = media_inputs['pixel_values'].type(inputs_embeds.dtype) | ||
| visual_tokens = model.visual_tokenizer(pixel_values, media_inputs['grid_thws']) | ||
| visual_embeds = model.vte(visual_tokens).to(dtype=inputs_embeds.dtype, device=inputs_embeds.device) | ||
| inputs_embeds = inputs_embeds + visual_embeds.mean() * 0. | ||
| else: | ||
| visual_tokens = model.visual_tokenizer(pixel_values, grid_thws) | ||
| visual_embeds = model.vte(visual_tokens).to(dtype=inputs_embeds.dtype, device=inputs_embeds.device) | ||
| inputs_embeds[input_ids == VISUAL_ATOM_ID] = visual_embeds | ||
|
|
||
| return {'inputs_embeds': inputs_embeds} |
There was a problem hiding this comment.
This _post_encode method has nearly identical logic to Ovis2_5Vit.get_inputs_embeds in swift/megatron/model/mm_gpt/qwen.py. To improve maintainability and avoid code duplication, consider refactoring this shared logic into a common utility function. This function could take the necessary components (e.g., inputs_embeds, input_ids, pixel_values, visual_tokenizer, vte) as arguments.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for the ovis2.5 model, including its integration with Megatron. The changes are comprehensive, covering documentation, model registration, template implementation, and trainer adjustments. The implementation for ovis2.5's multimodal input handling is well-done for both standard and Megatron training frameworks. My main feedback is to refactor model-specific magic numbers into constants to enhance code readability and maintainability.
| INDICATOR_IDS = [-301, -302, -303, -304] | ||
| VISUAL_ATOM_ID = -300 |
There was a problem hiding this comment.
These magic numbers are specific to the ovis2.5 model. It would be better to define them as constants at the class or module level to improve readability and maintainability. For example, you could define them as class attributes on Ovis2_5Template.
| INDICATOR_IDS = [-301, -302, -303, -304] | ||
| VISUAL_ATOM_ID = -300 |
There was a problem hiding this comment.
Similar to the implementation in Ovis2_5Template, these magic numbers should be defined as constants at the class or module level to avoid repetition and improve maintainability. For example, as class attributes on Ovis2_5Vit.
No description provided.