Repository navigation
Conversation
g1050
changed the title
|
I see the PR went to |
|
Hi @zucchini-nlp, it’s ready for review when you have a chance. Thanks! |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. Pages changed in this PR: |
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
Thanks a lot for working on the model. I think we can use modular in more cases and thus reduce LOC to review and maintain. I left comments below
|
Hi @zucchini-nlp, thanks for your review~ I’ve addressed your review comments point by point and this should be ready for your review again. The main changes are:
If there’s anything else that needs changes, please ping me anytime🤗. |
|
Thanks, reviewing today-tomorrow |
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
Thanks for iterating, looks much cleaner. I left a few questions and suggestions below, but approving so it can be reviewed by a core maintainer next
After addressing the comments and fixing the CI, you can ping Cyrilvallez for final review
| # Initialize weights and apply final processing | ||
| self.post_init() | ||
|
|
||
| # modified from transformers.models.qwen2_5_vl.modeling_qwen2_5_vl.Qwen2_5_VLModel.forward |
There was a problem hiding this comment.
we can copy. The diff is only in position ids and tbh I don't think it is needed, as long as the generation prepares the position ids for us
There was a problem hiding this comment.
You are correct. Our primary motivation is to reuse the existing functions from Qwen2.5-VL, such as get_rope_index and prepare_inputs_for_generation. The key modification in the forward function is the insertion of the line position_ids = position_ids[0] if len(position_ids.shape) == 3 else position_ids before the data is passed to the Qwen3 model. However, as the current modular tool does not support this specific operation, I had to directly copy the relevant code and implement the modification. Do you have any better solution?
| "MllamaVisionModel", # Building part of bigger (tested) model. # TODO: add tests | ||
| "Llama4TextModel", # Building part of bigger (tested) model. # TODO: add tests | ||
| "Llama4VisionModel", # Building part of bigger (tested) model. # TODO: add tests | ||
| "LlavaOnevision1_5Model", # Building part of bigger (tested) model. Tested implicitly through LlavaOnevision1_5ForConditionalGeneration. |
There was a problem hiding this comment.
this can be added in all_models_classes in the test file
There was a problem hiding this comment.
Thanks. But I did not find all_models_classes in the test file.
| "BltLocalDecoder", # Building part of bigger (tested) model. Tested implicitly through BLTForCausalLM. | ||
| "BltGlobalTransformer", # Building part of bigger (tested) model. Tested implicitly through BLTForCausalLM. | ||
| "Ovis2VisionModel", | ||
| "LlavaOnevision1_5VisionPretrainedModel", |
There was a problem hiding this comment.
let's expose it, usually PretrainedModel are not private. You need to add it in docs and in __all__
There was a problem hiding this comment.
LlavaOnevision1_5PreTrainedModel has been exposed in __all__, but LlavaOnevision1_5VisionPretrainedModel is only the vision part of LlavaOnevision1_5 and not an independent model. We might consider keeping it internal. For example, Qwen2_5_VisionTransformerPretrainedModel in Qwen2.5-VL is also not exposed, isn't that right?
|
Hi, @zucchini-nlp @Cyrilvallez. The comments have been addressed, I think... and this should be ready for final review. If there’s anything else that needs changes, please ping me anytime🤗. |
|
Dear @Cyrilvallez , apologies for the gentle nudge. Whenever convenient, could you please perform the final review? All feedback has been addressed and CI is passing. Thank you very much for your time and guidance. |
|
@Rocketknight1 @ArthurZucker @zucchini-nlp Sorry for the gentle nudge. Whenever convenient, could you please take a look for a final review/approval? All feedback has been addressed and CI is green. If there’s anything else you’d like me to adjust, I’m happy to update right away. Thank you very much for your time and guidance! |
|
run-slow: llava_onevision1_5 |
|
This comment contains models: ["models/llava_onevision1_5"] |
|
Hey @anxiangsir! Deeply deeply sorry for the delay on this! We are really focused on the upcoming v5 currently, which explain my late answer on this. Next iterations will be much quicker now that things are calming down. |
Cyrilvallez
left a comment
Cyrilvallez
left a comment
There was a problem hiding this comment.
Sorry again for the delay! here are a few things to still consider!
| # default: Load the model on the available device(s) | ||
| model = LlavaOnevision1_5ForConditionalGeneration.from_pretrained( | ||
| model_path, torch_dtype="auto", device_map="auto", trust_remote_code=True | ||
| ) | ||
|
|
||
| # default processer | ||
| processor = AutoProcessor.from_pretrained(model_path, trust_remote_code=True) |
There was a problem hiding this comment.
No trust_remote_code in the examples! We use native hf format!
| if isinstance(text_config, dict): | ||
| text_config["model_type"] = text_config.get("model_type", "qwen3") | ||
| self.sub_configs["text_config"] = CONFIG_MAPPING[text_config["model_type"]] | ||
| elif text_config is None: | ||
| self.sub_configs["text_config"] = CONFIG_MAPPING["qwen3"] |
There was a problem hiding this comment.
We never set sub_configs like this! I think this can simply be removed as not useful
|
|
||
| class LlavaOnevision1_5VisionMlp(VisionMlp): | ||
| pass |
There was a problem hiding this comment.
Let's use another base class, or redefine the init: we want the modules to be fully configured by the config, not different kwargs coming from the config
| torch.nn.init.normal_(module.class_embedding, mean=0.0, std=std_cls) | ||
| torch.nn.init.normal_(module.class_pos_emb, mean=0.0, std=std_cls) |
There was a problem hiding this comment.
Should use transformer inits: from ... import initialization as init and use this instead of torch.nn
|
|
||
| class LlavaOnevision1_5VisionPretrainedModel(Qwen2_5_VisionTransformerPretrainedModel): |
There was a problem hiding this comment.
Let's rename, PreTrained is always kept for the actual PreTrainedModel base class
| class LlavaOnevision1_5VisionPretrainedModel(Qwen2_5_VisionTransformerPretrainedModel): | |
| class LlavaOnevision1_5VisionModel(Qwen2_5_VisionTransformerPretrainedModel): |
| def __init__(self, config: LlavaOnevision1_5Config): | ||
| PreTrainedModel.__init__(self, config) | ||
| self.visual = LlavaOnevision1_5VisionPretrainedModel._from_config(config.vision_config) | ||
| self.language_model = AutoModel.from_config(config.text_config) | ||
| self.rope_deltas = None | ||
| # Initialize weights and apply final processing | ||
| self.post_init() | ||
|
|
||
| @auto_docstring | ||
| def forward( | ||
| self, | ||
| input_ids: Optional[torch.LongTensor] = None, | ||
| attention_mask: Optional[torch.Tensor] = None, | ||
| position_ids: Optional[torch.LongTensor] = None, | ||
| past_key_values: Optional[Cache] = None, | ||
| inputs_embeds: Optional[torch.FloatTensor] = None, | ||
| use_cache: Optional[bool] = None, | ||
| output_attentions: Optional[bool] = None, | ||
| output_hidden_states: Optional[bool] = None, | ||
| return_dict: Optional[bool] = None, | ||
| pixel_values: Optional[torch.Tensor] = None, | ||
| pixel_values_videos: Optional[torch.FloatTensor] = None, | ||
| image_grid_thw: Optional[torch.LongTensor] = None, | ||
| video_grid_thw: Optional[torch.LongTensor] = None, | ||
| rope_deltas: Optional[torch.LongTensor] = None, | ||
| cache_position: Optional[torch.LongTensor] = None, | ||
| second_per_grid_ts: Optional[torch.Tensor] = None, | ||
| **kwargs: Unpack[TransformersKwargs], | ||
| ) -> Union[tuple, LlavaOnevision1_5ModelOutputWithPast]: | ||
| r""" | ||
| image_grid_thw (`torch.LongTensor` of shape `(num_images, 3)`, *optional*): |
There was a problem hiding this comment.
Are those 2 different from Qwen ones?
| @unittest.skip( | ||
| reason="This architecture seem to not compute gradients properly when using GC, LlavaOnevision1_5VisionPretrainedModel does not support standalone training" | ||
| ) | ||
| def test_training_gradient_checkpointing(self): | ||
| pass | ||
|
|
||
| @unittest.skip | ||
| def test_training(self): | ||
| pass | ||
|
|
||
| @unittest.skip( | ||
| reason="This architecture seem to not compute gradients properly when using GC, LlavaOnevision1_5VisionPretrainedModel does not support standalone training" | ||
| ) | ||
| def test_training_gradient_checkpointing_use_reentrant(self): | ||
| pass | ||
|
|
||
| @unittest.skip( | ||
| reason="This architecture seem to not compute gradients properly when using GC, LlavaOnevision1_5VisionPretrainedModel does not support standalone training" | ||
| ) | ||
| def test_training_gradient_checkpointing_use_reentrant_false(self): | ||
| pass | ||
|
|
||
| @unittest.skip( | ||
| "VLMs need lots of steps to prepare images/mask correctly to get pad-free inputs. Can be tested as part of LLM test" | ||
| ) | ||
| def test_flash_attention_2_padding_matches_padding_free_with_position_ids(self): | ||
| pass |
There was a problem hiding this comment.
Are you sure those are not passing? Looks a lot like copy/paste
| def test_initialization(self): | ||
| config, inputs_dict = self.model_tester.prepare_config_and_inputs_for_common() | ||
|
|
||
| configs_no_init = _config_zero_init(config) | ||
| for model_class in self.all_model_classes: | ||
| model = model_class(config=configs_no_init) | ||
| for name, param in model.named_parameters(): | ||
| if param.requires_grad and "class_pos_emb" not in name and "class_embedding" not in name: | ||
| self.assertIn( | ||
| ((param.data.mean() * 1e9).round() / 1e9).item(), | ||
| [0.0, 1.0], | ||
| msg=f"Parameter {name} of model {model_class} seems not properly initialized", | ||
| ) |
There was a problem hiding this comment.
Is there any reason to overload the main one?
| test_pruning = False | ||
| test_head_masking = False |
There was a problem hiding this comment.
To remove, they disappeared
CI ResultsModel CI Report❌ Failed tests
|
|
Thanks for the work on LLaVA-OneVision-1.5! This branch predates v5 and now conflicts with main (auto mappings, model init and docs moved a lot since Nov 2025). The model is still not on main, so it is still welcome: could you rebase on current main and regenerate the modeling file from the modular file (ideally reusing the shared Qwen/LLaVA components) so we can do a fresh review? |
7d10c0b to
b07d0cd
Compare
|
Thank you for your contribution 🤗! CI Security Gate — waiting for maintainer approvalCI is approved automatically only for authors who already have a contribution merged in this repository. A maintainer will review this PR and approve CI to run on it. |
Use modular generation and shared model components for the native model, Auto registrations and checkpoint loading. Add documentation, vision export support and tests for inference, training and frame isolation.
b07d0cd to
0ed53a9
Compare
|
Hi @ArthurZucker @Cyrilvallez, I’m one of the developers of the LLaVA-OneVision 2 series, helping update this OV1.5 PR. I’ve rebased the branch and regenerated the modeling file from the modular source, reusing shared components. Local CPU/GPU model tests and checkpoint integration tests passed. CI auto-approval is blocked by the two Thanks for your help! |
|
@ZijunLi402 hey 👋🏻 very niice, i will take a look now. We had a few other PRs for Revewing in a sec 👁️ |
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
Very nice! I might have repeated myself and commented on each nit, overall it looks good :)
Re the tests - lets clean up and remove some that arent really important or covered implicitly via IntegrationTest/ModelTesterMixin
| @@ -0,0 +1,68 @@ | |||
| <!--Copyright 2025 The HuggingFace Team. All rights reserved. | |||
There was a problem hiding this comment.
to re-check that it's 2026 everywhere :)
| LLaVA-OneVision-1.5 was introduced in [LLaVA-OneVision-1.5: Fully Open Framework for Democratized Multimodal Training](https://huggingface.co/papers/2509.23661). | ||
| It combines a native resolution vision encoder with a Qwen3 language backbone. The vision encoder adds a learned class token to each image, applies full attention, removes the class token, and merges spatial patches into visual features for the language model. The language backbone uses ordinary one dimensional rotary positions for the combined image and text sequence. | ||
|
|
||
| The implementation reuses the Qwen2.5-VL processor and supports the converted [4B checkpoint](https://huggingface.co/Deep-VLM/LLaVA-OneVision-1.5-4B-Instruct-hf). Use left padding for batched generation. |
There was a problem hiding this comment.
no need to mentioned code implem here, can just say we support these ckpts/hub collection
|
|
||
| The implementation reuses the Qwen2.5-VL processor and supports the converted [4B checkpoint](https://huggingface.co/Deep-VLM/LLaVA-OneVision-1.5-4B-Instruct-hf). Use left padding for batched generation. | ||
|
|
||
| ## Abstract |
There was a problem hiding this comment.
no new section needed, abstract is part of overview and is usually formatted in italics
| ## Usage | ||
|
|
||
| ```python | ||
| from transformers import AutoProcessor, LlavaOnevision15ForConditionalGeneration |
There was a problem hiding this comment.
can we use AutoForImageTextToText in model
| model_id = "Deep-VLM/LLaVA-OneVision-1.5-4B-Instruct-hf" | ||
| processor = AutoProcessor.from_pretrained(model_id) | ||
| model = LlavaOnevision15ForConditionalGeneration.from_pretrained(model_id, device_map="auto") | ||
| messages = [{"role": "user", "content": [ | ||
| {"type": "image", "url": "https://huggingface.co/datasets/huggingface/documentation-images/resolve/main/pipeline-cat-chonk.jpeg"}, | ||
| {"type": "text", "text": "Describe the image."}, | ||
| ]}] | ||
| inputs = processor.apply_chat_template( | ||
| messages, tokenize=True, add_generation_prompt=True, return_dict=True, return_tensors="pt" | ||
| ).to(model.device) | ||
| output = model.generate(**inputs, max_new_tokens=64) | ||
| processor.batch_decode(output[:, inputs.input_ids.shape[1]:], skip_special_tokens=True) | ||
| ``` |
There was a problem hiding this comment.
niit: just a little more spacing for readability would be nice
| def test_mismatching_num_image_tokens(self): | ||
| """ | ||
| Tests that VLMs through an error with explicit message saying what is wrong | ||
| when number of images don't match number of image tokens in the text. | ||
| Also we need to test multi-image cases when one prompr has multiple image tokens. | ||
| """ | ||
| config, input_dict = self.model_tester.prepare_config_and_inputs_for_common() | ||
| for model_class in self.all_model_classes: |
There was a problem hiding this comment.
with VLMTester this one would not need to be overriden
| @require_torch | ||
| class LlavaOnevision15VisionModelTest(unittest.TestCase): | ||
| all_model_classes = (LlavaOnevision15VisionModel,) if is_torch_available() else () | ||
|
|
||
| @slow | ||
| @require_torch_greater_or_equal("2.11") |
There was a problem hiding this comment.
we need a single test class with all_model_class = (both classes here,)
| self.messages = [ | ||
| { | ||
| "role": "user", | ||
| "content": [ | ||
| {"type": "image"}, | ||
| {"type": "text", "text": "What kind of dog is this?"}, | ||
| ], | ||
| } | ||
| ] | ||
| url = "https://qianwen-res.oss-accelerate-overseas.aliyuncs.com/Qwen2-VL/demo_small.jpg" | ||
| self.image = Image.open(requests.get(url, stream=True).raw) |
There was a problem hiding this comment.
can we put url inside a conversation dict and use ...test_processing.url_to_local_peth to load from cached location
If possible, also lets use an existing image from other tests, we are trying to keep small number of testing images and use it across all models
| expected_input_ids = [151644, 8948, 198, 2610, 525, 264, 10950, 17847, 13, 151645, 198, 151644, 872, 198, 151652, 151655, 151655] # fmt: skip | ||
| torch.testing.assert_close(expected_input_ids, inputs.input_ids[0].tolist()[:17]) | ||
|
|
||
| expected_pixel_slice = torch.tensor( | ||
| [ | ||
| [0.8792, 0.8792, 0.9084], | ||
| [1.1858, 1.1858, 1.2296], | ||
| [1.2004, 1.2004, 1.2150], | ||
| [1.4340, 1.4340, 1.4194], | ||
| [1.3902, 1.4048, 1.4194], | ||
| [1.5216, 1.5362, 1.5362], | ||
| ], | ||
| dtype=torch.float32, | ||
| device="cpu", | ||
| ) |
There was a problem hiding this comment.
Expectations class pls to get expectation for given hardware
| messages = [{"role": "user", "content": [{"type": "video"}, {"type": "text", "text": "Describe the video."}]}] | ||
| text = self.processor.apply_chat_template(messages, tokenize=False, add_generation_prompt=True) | ||
| video = np.stack([np.asarray(self.image), np.asarray(self.image.transpose(Image.Transpose.FLIP_LEFT_RIGHT))]) |
There was a problem hiding this comment.
can we use real video from https://huggingface.co/datasets/hf-internal-testing/fixtures_videos and same comment about url-to-local-path
Reuse shared model components and VLM tests, simplify configuration and checkpoint mappings, and use cached image and real video fixtures. Preserve learned CLS gradients and compatibility with published and native checkpoints.
Preserve explicit weight tying in new configurations while retaining legacy checkpoint compatibility. Restore standalone vision loading from full checkpoints and cover both paths with small regression tests.
|
Thank you for your contribution 🤗! CI Security Gate — automatic approval blockedThis PR was not automatically approved for CI because the security gate failed. Possible reasons:
See the workflow run for the exact violations. A maintainer can review and manually approve CI if a finding is a false positive. |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto, llava_onevision1_5 |
What does this PR do?
This PR adds LLaVA-OneVision-1.5
Fixes #41081
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.