Repository navigation
Fix SmolVLM inputs_merger when image tokens are not a multiple of patch_size - #49725
Conversation
|
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. |
| @@ -121,25 +118,7 @@ def inputs_merger( | |||
| else: | |||
There was a problem hiding this comment.
can get rid of inputs merger in that case and just merge inside a froward call
There was a problem hiding this comment.
Done: inputs_merger is removed, and forward (in SmolVLM and in ModernVBert, which inherits it) now uses get_placeholder_mask + masked_scatter, as in llava. Its token/feature count check exposed two test bugs, also fixed: the resize tests set model.image_token_id instead of model.config.image_token_id (so they ran with 0 image tokens), and the ModernVBert one reused inputs_dict mutated by the previous model class.
|
[For maintainers] Suggested jobs to run (before merge) run-slow: modernvbert, smolvlm |
CI recapDashboard: View test results in Grafana |
|
Thanks for the review and merge! @Cyrilvallez @zucchini-nlp, would it be possible to include this fix in the next patch release? The regression came with v5.20.1 (#49557), and v5.20.2 does not include the fix, so for instance |
|
@albertvillanova it might get lost in GH, pls tag core maintainers in slack. I will add for patch label as well |
What does this PR do?
Fixes #49724
#49557 made
get_image_featuresreturn a list of flattened per-sample features, and changedSmolVLMModel.inputs_mergerto split them into blocks ofself.config.vision_config.patch_size. That is the pixel patch size of the vision encoder, not the number of image tokens per image, so the forward pass with images fails whenever the number of image tokens per image is not a multiple of it, e.g.HuggingFaceTB/SmolVLM2-2.2B-Instruct(81 tokens per image, patch size 14):With the features already flattened, the block indexing only fills the image tokens in order, so this PR removes
inputs_mergerand merges the image features directly inforwardwithget_placeholder_mask+masked_scatter, as in llava (also inModernVBertModel, which inherits fromSmolVLMModel). I did not compute the tokens per image from the config (as suggested in the issue) because the ModernVBert config has noscale_factor.The token/feature count check of
get_placeholder_maskexposed two test bugs, fixed here: the resize tests setmodel.image_token_idinstead ofmodel.config.image_token_id(so they ran with 0 image tokens), and the ModernVBerttest_resize_tokens_embeddingsreused theinputs_dictmutated by the previous model class.The tester
image_sizegoes from 16 to 24 (9 image tokens per image, patch size 4), so the common tests cover this case: without the fix, 62 SmolVLM tests fail. On the repro from the issue, the logits are identical to v5.20.0.Before submitting
Who can review?
@zucchini-nlp