VLM: fixes after refactor #32907

zucchini-nlp · 2024-08-21T04:43:23Z

What does this PR do?

After merging #30962 I noticed that some models stopped working in multi-image/image-video setting. Also cache_position wasn't being updated accordingly after merging image and text inputs in legacy way. This PR addresses the issue and adds slow tests for that

Also, I think we should use num_image_tokens instead of trying to infer it from image size and patch size. The reason is that some image backbones add CLS (CLIP) and other don't (SigLIP). If we try to catch every edge case like this, we'll end up bloating the code, so it's better to let users set the number of tokens needed for one image after encoding with the vision tower. That way we can support different vision towers out-of-the-box

Ran slow tests to test if expanding matches legacy, the new tests and a few older tests. Also changes video llava tests as those weren't formatting the prompt correctly

HuggingFaceDocBuilderDev · 2024-08-21T05:03:23Z

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.

amyeroberts

This is a very big PR that touches a lot of modeling code in different ways. I just started reviewing but I think we should split up the changes here: switching to num_image_tokens is independent of fixing tests. This will make it easier to make sure the changes here are sensible.

A few comments:

Why weren't the breaks caught previously? Was it lack of tests or the test fetcher? If lack of tests, tests should be added which will catch this
I'm not sure about this switch to num_image_tokens -- can we guarantee this is always fixed? We need to properly handle this change in a non-breaking way
Slow tests should be run for all these models

src/transformers/models/llava/processing_llava.py

amyeroberts · 2024-08-22T18:03:40Z

src/transformers/models/llava_next/processing_llava_next.py

+        patches_height = int(math.sqrt(self.num_image_tokens))
+        patches_width = int(math.sqrt(self.num_image_tokens))


This now assumes the image is square

The image after vision tower is a square, if i'm not mistaken? Or are there any vision backbones that have non-square format?

I am thinking of the way to not rely on the patch_size/image_size args, because it came out that some models add CLS token and some do not. But I may be still missing some edge cases

tests/models/video_llava/test_modeling_video_llava.py

zucchini-nlp · 2024-08-30T12:41:32Z

Oke, I will separate these into two PRs and then I have another one, which adds Generation tests in VLMs. That will be a third PR

Why weren't the breaks caught previously? Was it lack of tests or the test fetcher? If lack of tests, tests should be added which will catch this
Yes, no tests for these cases were added, I added some more. I'll add one more for llava-interleave checkpoint as it uses SigLIP as backbone
I'm not sure about this switch to num_image_tokens -- can we guarantee this is always fixed? We need to properly handle this change in a non-breaking way
If we didn't change any checkpoint and the latest major release still doesn't have the new processing logic, this shouldn't cause BC errors. WDYT?
Slow tests should be run for all these models
Yes, will enable in each new PR

zucchini-nlp · 2024-08-30T14:02:36Z

To be reviewed and merged after #33168

amyeroberts · 2024-08-30T18:12:34Z

@zucchini-nlp OK - just ping when you want another review!

zucchini-nlp · 2024-09-05T11:03:32Z

src/transformers/models/llava_next/processing_llava_next.py

+        prompt_strings = text
+        if image_inputs:
+            if self.patch_size is None or self.vision_feature_select_strategy is None:
+                logger.warning_once(
+                    "Expanding inputs for image tokens in LLaVa-NeXT should be done in processing. "
+                    "Please add `patch_size` and `vision_feature_select_strategy` to the model's processing config or set directly "
+                    "with `processor.patch_size = {{patch_size}}` and processor.vision_feature_select_strategy = {{vision_feature_select_strategy}}`. "
+                    "Using processors without these attributes in the config is deprecated and will throw an error in v4.47."


This was needed for cases with mutli-image inputs where we cannot be sure that number of image sizes is same as text. For ex, one text and two images

zucchini-nlp · 2024-09-05T11:04:12Z

src/transformers/models/llava_next/processing_llava_next.py

-        current_width = patches_height * scale_height
-        current_height = patches_width * scale_width
+        current_height = patches_height * scale_height
+        current_width = patches_width * scale_width


The order was corrected in other PR for modeling code but I forgot to add same correction to the processing logic

zucchini-nlp · 2024-09-05T11:04:31Z

src/transformers/models/llava_next_video/diff_llava_next_video.py

+            img_token_not_enough = (input_ids == self.config.image_token_index).sum(
+                1
+            ).max() < self.config.image_seq_length
+            video_token_not_enough = (input_ids == self.config.video_token_index).sum(
+                1
+            ).max() < self.config.video_seq_length
+            inputs_not_expanded = (img_token_not_enough and pixel_values is not None) or (
+                video_token_not_enough and pixel_values_videos is not None


better check for multiple visual per prompt

zucchini-nlp · 2024-09-05T11:05:23Z

src/transformers/models/llava_next_video/processing_llava_next_video.py

+    # Copied from transformers.models.llava_next.processing_llava_next.LlavaNextProcessor._get_number_of_features
+    def _get_number_of_features(self, orig_height: int, orig_width: int, height: int, width: int) -> int:
+        image_grid_pinpoints = self.image_processor.image_grid_pinpoints


we forgot about image inputs for this model, added a test

zucchini-nlp · 2024-09-05T11:07:57Z

@amyeroberts this is ready for review now, the blocking PR is merged. For the tests, the failing ones are SDPA-related which is fixed in #32238 (needs a separate review, but not a blocker for current PR).

The tests are fixed so that we match the runner's output. I verified that the discrepancy appears after updating torch to the latest version and the test was passing in 2.3.0

amyeroberts

Thanks for working on this fix!

There's just one outstanding question I have about the iterator logic for image_sizes

tests/models/vipllava/test_modeling_vipllava.py

amyeroberts · 2024-09-05T14:46:51Z

src/transformers/models/llava_next/processing_llava_next.py

+                prompt_strings = []
+                for sample in text:
+                    while self.image_token in sample:
+                        image_size = next(image_sizes)


Is this right? The previous logic implies len(image_sizes) == len(text) however this is implying we have the same number of image_sizes as the number of image tokens per sample * number of samples.

Yes, I verified twice to be sure. The number of images == number of image sizes. The new added test multiimage_expansion fails with the previous logic

@amyeroberts do you have any other concerns regarding this PR?

amyeroberts

Thanks for double checking the logic!

* leave only half of the changes * fix tests * [run-slow] llava, llava_next, llava_next_video, vipllava, video_llava * fix tests, first try * [run-slow] llava, llava_next, llava_next_video, vipllava, video_llava * fix, second try * [run-slow] llava, llava_next, llava_next_video, vipllava, video_llava * fix * [run-slow] llava, llava_next, llava_next_video, vipllava, video_llava

zucchini-nlp requested a review from amyeroberts August 21, 2024 09:12

amyeroberts reviewed Aug 22, 2024

View reviewed changes

leave only half of the changes

70a70b1

zucchini-nlp force-pushed the vlm-fixes-after-refactor branch from 42042cc to 70a70b1 Compare August 30, 2024 14:01

zucchini-nlp mentioned this pull request Sep 3, 2024

[fix] LlavaNextProcessor '_get_unpadded_features' method #33263

Merged

5 tasks

zucchini-nlp added 2 commits September 3, 2024 10:50

merge main

4b1179d

fix tests

4fc07d0

zucchini-nlp added the run-slow label Sep 3, 2024

zucchini-nlp added 7 commits September 3, 2024 13:44

[run-slow] llava, llava_next, llava_next_video, vipllava, video_llava

a1d4434

fix tests, first try

255de78

[run-slow] llava, llava_next, llava_next_video, vipllava, video_llava

41dc04d

fix, second try

cb787c9

[run-slow] llava, llava_next, llava_next_video, vipllava, video_llava

cf35b01

fix

6a7a40e

[run-slow] llava, llava_next, llava_next_video, vipllava, video_llava

ead33ff

zucchini-nlp commented Sep 5, 2024

View reviewed changes

zucchini-nlp requested a review from amyeroberts September 5, 2024 11:08

amyeroberts reviewed Sep 5, 2024

View reviewed changes

zucchini-nlp mentioned this pull request Sep 8, 2024

Track progress for VLMs refactoring #33374

Open

16 tasks

amyeroberts approved these changes Sep 9, 2024

View reviewed changes

Merge branch 'huggingface:main' into vlm-fixes-after-refactor

663a354

zucchini-nlp merged commit 7d2d6ce into huggingface:main Sep 10, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VLM: fixes after refactor #32907

VLM: fixes after refactor #32907

zucchini-nlp commented Aug 21, 2024 •

edited

Loading

HuggingFaceDocBuilderDev commented Aug 21, 2024

amyeroberts left a comment

amyeroberts Aug 22, 2024

zucchini-nlp Aug 30, 2024

zucchini-nlp commented Aug 30, 2024

zucchini-nlp commented Aug 30, 2024

amyeroberts commented Aug 30, 2024

zucchini-nlp Sep 5, 2024

zucchini-nlp Sep 5, 2024

zucchini-nlp Sep 5, 2024

zucchini-nlp Sep 5, 2024

zucchini-nlp commented Sep 5, 2024

amyeroberts left a comment

amyeroberts Sep 5, 2024

zucchini-nlp Sep 6, 2024

zucchini-nlp Sep 9, 2024

amyeroberts left a comment

		patches_height = int(math.sqrt(self.num_image_tokens))
		patches_width = int(math.sqrt(self.num_image_tokens))

VLM: fixes after refactor #32907

VLM: fixes after refactor #32907

Conversation

zucchini-nlp commented Aug 21, 2024 • edited Loading

What does this PR do?

HuggingFaceDocBuilderDev commented Aug 21, 2024

amyeroberts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zucchini-nlp commented Aug 30, 2024

zucchini-nlp commented Aug 30, 2024

amyeroberts commented Aug 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zucchini-nlp commented Sep 5, 2024

amyeroberts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amyeroberts left a comment

Choose a reason for hiding this comment

zucchini-nlp commented Aug 21, 2024 •

edited

Loading