Simplify Idefics2, Idefics3, SmolVLM images handling #37291
Conversation
|
Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. The CI will be paused while the PR is in draft mode. When it is ready for review, please click the |
|
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. |
|
@zucchini-nlp Hello! Pinging you here as smolvlm also handles video inputs, and I'm wondering what you think about having flattened pixel_values by default when processing videos, instead of grouping them by frames or video instance. Also since most image (and maybe video?) processors for vlm using some kind of patching/splitting flatten the patches when preprocessing, we might want to update the base processing tests to account for that? Or at least make them parameterized |
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
Cool, thanks for cleaning it! I have a few concerns though.
- After this PR idefics models will output pixels where first dim is not necessarily the batch size, whenever an image splitting happens. We had problems in the past with Gemma3 (huggingface/trl#3121 (comment)) and Qwen2-VL (#33666) for the same reason. Tl;DR; train loaders/frameworks iterate over data assuming the first dim is batch and fail when it is not.
I realize this is not a common case, but we might be breaking train for some users with this. So I'm a bit hesitant to return flat images. LMK what you think about it - Do the model logits stay same if we test with several batches and several images per batch? Let's run slow tests before merging :)
| elif inputs_embeds is not None: | ||
| batch_size, seq_length, _ = inputs_embeds.shape | ||
| else: | ||
| if input_ids is None and inputs_embeds is None: |
There was a problem hiding this comment.
need to also check cases when both are not None:
if (input_ids is None) ^ (inputs_embeds is not None):
| elif inputs_embeds is not None: | ||
| batch_size, seq_length, _ = inputs_embeds.shape | ||
| else: | ||
| if input_ids is None and inputs_embeds is None: |
ArthurZucker
left a comment
ArthurZucker
left a comment
There was a problem hiding this comment.
thanks for working on this!
| input_data_format = infer_channel_dimension_format(images_list[0][0]) | ||
| input_data_format = infer_channel_dimension_format(images[0]) | ||
|
|
||
| if do_image_splitting: |
There was a problem hiding this comment.
why don't we try to do a single for loop while we are at it?
|
Just for info, there's a bug in video processing with smolvlm2 where the list of frames is malformed after calling I saw it reported here, but it's a transformers issue, not MLX's. |
5 participants
Simplify the handling of images in both processing and modeling.
Now the images/patches are flattened before being processed and passed to the models. This means that the image processing is simplified (no need for padding in the number of images/patches dimension), along with the modeling code ( No more padding images/patches containing only 0/False needing to be removed).
I tested thoroughly for each models with multiple images, batched images etc. and found no differences.
Cc @andimarafioti @orrzohar