[model] support LLaVA-OneVision-1.5 #6284
Conversation
update faq.
Fixed the inconsistencies between the Chinese and English documentation.
Update link to sequence parallel example
Summary of ChangesHello @slin000111, 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 significantly expands the framework's capabilities by integrating the LLaVA-OneVision-1.5 series of multimodal models. The changes involve not only listing these new models in the documentation but also deeply embedding their support within the system's core, including new model types, architectures, and a dedicated template for processing complex multimodal inputs. This ensures that users can leverage the advanced vision and language understanding of LLaVA-OneVision-1.5 models seamlessly. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for the LLaVA-OneVision-1.5 model, including necessary changes to model registration, architecture definitions, and template handling for multimodal inputs. The implementation is mostly sound, with updates to documentation to reflect the new model. My review focuses on improving maintainability by replacing magic numbers with constants, ensuring dependency declarations are complete and accurate, and enhancing the robustness of the video processing logic.
| requires=['transformers>=4.53.0', 'qwen_vl_utils'], | ||
| tags=['vision'], |
There was a problem hiding this comment.
The requires list is missing the decord dependency, which is necessary for video processing, and a version for qwen_vl_utils. Also, the tags list is missing 'video'. The documentation correctly includes these. Please update them to ensure all dependencies are declared and the model's capabilities are correctly tagged.
| requires=['transformers>=4.53.0', 'qwen_vl_utils'], | |
| tags=['vision'], | |
| requires=['transformers>=4.53.0', 'qwen_vl_utils>=0.0.6', 'decord'], | |
| tags=['vision', 'video'], |
| model_dir) | ||
| model_cls._no_split_modules = ['LLaVAOneVision1_5_DecoderLayer', 'RiceBlock'] | ||
| model, processor = get_model_tokenizer_multimodal(model_dir, *args, **kwargs) | ||
| model.config.vision_start_token_id = 151652 |
There was a problem hiding this comment.
The value 151652 is a magic number. To improve code readability and maintainability, it's better to define it as a constant with a descriptive name (e.g., LLAVA_ONEVISION_VISION_START_TOKEN_ID) at the module level and use the constant here.
| image_token_id = 151655 | ||
| video_token_id = 151656 |
There was a problem hiding this comment.
The token IDs 151655 and 151656 are used as magic numbers. For better readability and maintainability, please define them as constants with descriptive names at the module level and use them here.
| if hasattr(processor, 'video_processor'): | ||
| processor_func = processor.video_processor | ||
| else: | ||
| processor_func = processor.image_processor | ||
| kwargs['images'] = None |
There was a problem hiding this comment.
The fallback to processor.image_processor for video processing is fragile. It assumes image_processor can handle videos if video_processor is absent. This might not hold for other models, potentially causing future bugs. A more robust implementation would be to either check if image_processor supports video or raise an error if a video is provided without a video_processor.
1 participant
PR type
PR information
Write the detail information belongs to this PR.
support LLaVA-OneVision-1.5, #6123
Experiment results
Paste your experiment result here(if needed).