VLMs: `patch_size` -> `num_image_tokens` in processing #33424

zucchini-nlp · 2024-09-11T06:48:19Z

What does this PR do?

Following #33374, we'll use num_image_tokens instead of patch_size VLM in processors. The reason is because some image backbones add a CLS token while other do not, and we cannot infer the number of tokens needed for an image from number of patches reliably.

Yes, as noted in some prev discussions, we'll have to assume for llava-next style models that the image is a square and obtain patch-width/patch-height from num_image_tokens. If I'm not mistaken, the backbones we have in the library and are used by VLMs are all square sized, and since llava-next class if not used with other backbones apart from CLIP afaik, we should be good to assume it that way.

Modified tests are passing locally for me

HuggingFaceDocBuilderDev · 2024-09-11T07:08:24Z

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

Thanks for opening this PR @zucchini-nlp!

I know this has been discussed elsewhere - but I can't remember exactly where - if you find it, could you link to it in this PRs description?

I still don't think num_image_tokens is a great solution -- I would have assumed the number of image tokens could be variable based on the input image size, which patch_size accounts for but num_image_tokens does not.

Assuming this is safe because of some of the backbones used with current checkpoints is an unreliable assumption. Part of the feature of a lot of these VLMs architectures is that different vision towers can swapped out easily and so it's very difficult to have strong assumptions about their properties. One option is to try and enforce these properties through validation.

I know part of the motivation is because patch_size calculations are also not reliable, although I don't think I've understood why adding the CLS token affects this? I would help to understand with an example to show where the issue is.

amyeroberts · 2024-09-12T17:17:34Z

src/transformers/models/llava/processing_llava.py

+        prompt_strings = text
+


repeated in the next lines

amyeroberts · 2024-09-21T00:26:27Z

src/transformers/models/llava/processing_llava.py

    image_processor_class = "AutoImageProcessor"
    tokenizer_class = "AutoTokenizer"

    def __init__(
        self,
        image_processor=None,
        tokenizer=None,
-        patch_size=None,


For backwards compatibility, we can't just remove accepting patch_size immediately. We need to deprecate it and not allow both patch_size and num_image_tokens to be specified.

You can use the @deprecate_kwarg decorator to handle this

amyeroberts · 2024-09-21T00:26:46Z

src/transformers/models/llava/processing_llava.py

        vision_feature_select_strategy=None,
        chat_template=None,
        image_token="<image>",  # set the default and let users change if they have peculiar special tokens in rare cases
        **kwargs,
    ):
-        self.patch_size = patch_size


Same here - we need to deprecate this property

amyeroberts · 2024-09-21T00:40:27Z

src/transformers/models/llava_next/processing_llava_next.py

        unpadded_features, newline_features = self._get_unpadded_features(
            orig_height, orig_width, patches_height, patches_width, scale_height, scale_width
        )
        # The base patch covers the entire image (+1 for the CLS)
-        base_features = patches_height * patches_width + 1
+        base_features = self.num_image_tokens


I don't think the numbers quite match here. Even if we assume the patches are square, this also assumes the image is square, which might not be the case.

Let's say the size of the image is 22x18 and a patch size is 4.

In the first case:

patches_height = 22 // 4 = 5 patches_width = 18 // 4 = 4 base_features = patches_height * patches_width + 1 = 21

If we then work backwards

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

So this conversion isn't backwards compatible and I think it's also less correct

Yes, right. but also I don't think this is possible in available llava-next checkpoints

zucchini-nlp · 2024-09-23T13:26:32Z

@amyeroberts

Yes, you're right that we are trying to make this work for current existing vision models and there may be some other models that do not have hard requirement for having a square image and thus square patches.

The problem with CLS is that num of image tokens is now computed as patches_height * patches_width for all cases and we add +1 by default to account for the CLS. But not all existing vision models add a cls token.

Since we have all VLMs on CLIP-like vision tower, I think we'll be safe in general either way we choose to implement. But apart from that, my main idea was that if some fancy architecture comes out where we won't be able to infer number of tokens from image/patch size. Then we can safely delegate setting correct number of image tokens for whatever vision model is used to users. I see this option as safer (imo) and more aligned with models like BLIP or Paligemma where we don't do special calculations to get how many image tokens are needed.

What do you think if we leave the patch_size as kwargs only for llava-next given that it is more quirky and requires to know how many patches each image is divided into? For all others we go with num_image_tokens to be on the safer side

amyeroberts · 2024-10-02T14:10:27Z

But apart from that, my main idea was that if some fancy architecture comes out where we won't be able to infer number of tokens from image/patch size. Then we can safely delegate setting correct number of image tokens for whatever vision model is used to users. I see this option as safer (imo) and more aligned with models like BLIP or Paligemma where we don't do special calculations to get how many image tokens are needed.

This seems to me like we're engineering for a problem which doesn't yet exist.

For all others we go with num_image_tokens to be on the safer side

My main reservation with this is that yes, we can safely delegate the number of tokens, but we can no longer safely calculate patch_size.

Would it be possible to have both arguments? Where if both are set we verify they're compatible?

As I'm off I'll delegate this decision to @qubvel. I'll be happy with whatever you both decide!

zucchini-nlp · 2024-10-08T08:59:46Z

Hi @qubvel !

Can you look at the last discussion we had with Amy when you have time :)

qubvel · 2024-10-08T14:45:29Z

I tried to summarize the pros and cons of existing options, feel free to add if I missed anything

1. `patch_size` and default `+1` for CLS token

+ Support for dynamic image size
- Not all architectures have CLS token added, we cannot reliably infer num_image_tokens one from patch_size
- [Potential] There might be an architecture where we can't infer num_image_tokes based on patch_size and CLS token (examples?)

2. `num_image_tokens`

+ Do not depend on CLS token and can support custom num_image_tokens
- Still need patch_size for some processors, we have the same problem - we cannot reliably infer one from another
- From user's point of view it might be not obvious how to get num_image_tokens while patch size can be taken directly from the config

So, both of them are not ideal 🥲 Are there any other options? Maybe we can add to configs smth like num_special_tokens, wdyt?

zucchini-nlp · 2024-10-09T10:46:45Z

@qubvel right, patch_size gave us flexibility to work with non-square images but we can't reliably get number of image tokens in some cases. I did a small overview of what types of VLMs are out there and

we can categorize VLMs image backbones into three types:

perceiver-like models have a fixes image seq length (Ideifcs, BLIP) so we are good here
models that use simple ViT architecture and optionally anyres splitting/pixel shuffle. We can be sure abuot image seq length in all cases if position interpolation is not on. AFAIK none of existing VLM papers use interpolation, but they rather try anyres/multiple encoders etc. for high resolutions. We can also use patch_size with same level of certainty and might need extra kwargs for specific models (convnext, internViT) to account for donwsampling
flexible image size models where the image is resized smartly like in Qwen2-VL. These models resize so that it fits in one of the possible shapes and preserved max of the original image. We can't use image seq length for them, and we should infer number of tokens from each image's size and other params like downsample ratio or merge size

This means we can't use one version of processing for all VLMs to get number of image tokens. And I am still more inclined to num_image_tokens because

we already use image_seq_length in other VLMs like Paligemma, Idefics, Chameleon and BLIP instead of trying to get number of tokens using heuristics
regarding the above concerns with non-square images, it seems like we try to accoun for smth that the model checkpoints don't support. In other words, non-square can be the case for llava only when positions in ViT are interpolated which is not used in any of the models
we still will have one model that needs special treatment. Qwen2-VL has dynamic seq length for each image depending on its size, so we leave it as is
yeah, from user perspective seems easier to pass in patch_size and hope everything works as magic. We are actually going to add self._get_image_features for all VLMs to make it more modular. Maybe users who want to tinker can rely on that method later if they decide to change ViT patch sizes?

At the end we'll do num_image_tokens or image_seq_length (to be aligned with existing code) for models where pixel shapes are fixed based on crop-size/resize. And for those with dynamic resize, we'll need to also dynamically calculate image seq length . WDYT?

Unfortunately I can't come up with a better solution that will work out of the box for all possible VLMs in all cases, but imo we don't have to. As long as the current approach works with the current model, and is consistent library-wise

qubvel · 2024-10-10T15:08:21Z

Thanks for the very detailed clarification! Indeed it looks like processors' logic can vary significantly across models, the questions here are

how to uniformize processors logic (would be easier for users and pipelines integration)
make a reliable method to compute image_tokens/patch_size

My suggestion was to use num_special_tokens or num_additinal_tokens, which could be used instead of +1. In that case, we have to provide two arguments patch_size and num_special_tokens (it could be 0/1/2/... and can be a property of a model config, to be computed dynamically based on enabled cls token for example):

# for square images
num_image_tokens = (image_size / patch_size) ** 2 + num_special_tokens

For such a case we might dynamically compute the number of image tokens based on the resulting image size and patch_size to support interpolation. TBH, I'm not sure how it's scaling up to all architectures, it's just an idea to take into consideration 😄

I'm happy to go with your option too! And maybe just more examples/docs will be enough to make users comfortable!

zucchini-nlp · 2024-10-11T09:39:17Z

how to uniformize processors logic (would be easier for users and pipelines integration)

Unfortunately relying on only one way to ave uniform/same kwargs is impossible at the moment since models have different requirements for vision backbone. Though we can use the patch_size + num_additional_tokens for VLMs with ViT backbone and image_seq_length for others where the length is fixed

Let me then add the num_additional_tokens, looks good to me and maybe less breaking (even though we didn't update model checkpoints yet)

zucchini-nlp · 2024-10-15T08:05:51Z

Slow tests for expansion are passing all locally

qubvel

Thanks, looks good to me!
It'd be great to add some docs regarding where should users look for num_additional_image_tokens param. Should we add it as a config property/param?

zucchini-nlp · 2024-10-29T15:42:03Z

yea, docs will be much needed given we already had some users asking about how to suppress the warning. I don't know if there is a place we can add info about VLMs in more details so I am thinking to add a WARNING section in model doc page for each touched model. Glad there are only ~5 models

zucchini-nlp · 2024-10-30T07:42:43Z

Done, added the docs. LMK if it looks good to you and I'll request review from core maintainer. I hope after this PR we can update hub files and remove extra logic by v4.50 (deprecation target will be modified in #34245)

qubvel

LGTM, thanks!

ArthurZucker

I am a bit out of loop, so needs some clairiication!

ArthurZucker · 2024-10-30T09:15:11Z

src/transformers/models/video_llava/processing_video_llava.py

-                "with `processor.patch_size = {{patch_size}}` and processor.vision_feature_select_strategy = {{vision_feature_select_strategy}}`. "
+                "Please add `patch_size`, `num_additional_image_tokens` and `vision_feature_select_strategy` to the model's processing config or set directly "
+                "with `processor.patch_size = {{patch_size}}`, `processor.num_additional_image_tokens = {{num_additional_image_tokens}}` "
+                "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.44."


this was released a long time ago we can probably ignore it now no?

There is a PR from @gante (#34245) that moves deprecation target a few more releases up, so we have time to update configs on the hub and for users to get used to new format

Imo we need at least 1 release to remove it totally

ArthurZucker · 2024-10-30T09:16:43Z

src/transformers/models/llava_next_video/processing_llava_next_video.py

-                "with `processor.patch_size = {{patch_size}}` and processor.vision_feature_select_strategy = {{vision_feature_select_strategy}}`. "
+                "Please add `patch_size`, `num_additional_image_tokens` and `vision_feature_select_strategy` to the model's processing config or set directly "
+                "with `processor.patch_size = {{patch_size}}`, `processor.num_additional_image_tokens = {{num_additional_image_tokens}}` "
+                "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."


we are in 4.47

ArthurZucker · 2024-10-30T09:16:55Z

src/transformers/models/llava_next/processing_llava_next.py

-                    "with `processor.patch_size = {{patch_size}}` and processor.vision_feature_select_strategy = {{vision_feature_select_strategy}}`. "
+                    "Please add `patch_size`, `num_additional_image_tokens` and `vision_feature_select_strategy` to the model's processing config or set directly "
+                    "with `processor.patch_size = {{patch_size}}`, `processor.num_additional_image_tokens = {{num_additional_image_tokens}}` "
+                    "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."


same here (unless this was not released!)

ArthurZucker · 2024-10-31T16:52:55Z

src/transformers/models/llava/processing_llava.py

@@ -70,12 +79,14 @@ def __init__(
        image_processor=None,
        tokenizer=None,
        patch_size=None,
+        num_additional_image_tokens=0,


should probably be placed at the end for BC as well 😉

also it was set to 1 (i mean num_image_tokens -= 1 now it will use 0

this one is important

oke, will fix it and merge after making sure tests pass

ArthurZucker · 2024-10-31T16:53:50Z

src/transformers/models/llava/processing_llava.py

@@ -158,8 +171,9 @@ def __call__(
            else:
                logger.warning_once(


this will be in 4.47

ArthurZucker · 2024-10-31T16:56:33Z

src/transformers/models/llava/processing_llava.py

@@ -51,6 +51,9 @@ class LlavaProcessor(ProcessorMixin):
            The tokenizer is a required input.
        patch_size (`int`, *optional*):
            Patch size from the vision tower.
+        num_additional_image_tokens (`int`, *optional*, defaults to 0):
+            Number of additional tokens added to the image embeddings, such as CLS (+1). If the backbone has no CLS or other


they are added where (to which image embedding and why) also I am not even sure I understand the usage of this myself. As it will be in all processors, can you explain a bit more why someone should set this?

Yes, the very first reason for this PR is that some ViT backbones can add a CLS token to image patches, while other do not. Therefore our current processors can't work with SigLIP because we hardcoded the CLS token addition in code.

We had two options to fix it:

what I proposed initially was to simply use num_image_tokens arg and let users specify any amount so we don't have to infer how many patches there will be from patch_size and image_size. Some processor already do that like Paligemma

it had drawbacks though that we cannot work with non-square images in that case, because in case of llava-next the padding/unpadding is needed. That depends on patch-size and image-size. I realize we have no models that accept non-square images, but that was the strongest objection from core maintainer against the num-image-tokens

So after discussions with Pavel, we decided to make as less changes as possible and still support SigLIP by adding num_additional_tokens. It will be 1 if the vision tower adds CLS, and otherwise 0. It can be more than 1 in case there is any vision tower with extra tokens added on top of image patches

ok thanks for explaning!

ArthurZucker

just 1 place to fix

zucchini-nlp · 2024-11-18T12:16:53Z

Cool, merging soon and I'll update hub configs this week for llava-hf models + open PRs for BLIP models. We've had the deprecation warning for 3-4 releases and should be fine with updating configs

…3424) * use num additional tokens * fix copies + docs * another fix copies :) * add docs * move order for BC

zucchini-nlp changed the title ~~use num_image_tokens~~ VLMs: patch_size -> num_image_tokens in processing Sep 11, 2024

zucchini-nlp requested a review from amyeroberts September 11, 2024 06:49

amyeroberts reviewed Sep 21, 2024

View reviewed changes

zucchini-nlp mentioned this pull request Oct 4, 2024

Track progress for VLMs refactoring #33374

Open

16 tasks

zucchini-nlp added 2 commits October 15, 2024 10:04

use num additional tokens

af34e5c

Merge remote-tracking branch 'upstream/main' into vlms-continuatino

95b5e9d

zucchini-nlp force-pushed the vlms-continuatino branch from 1e3751b to 95b5e9d Compare October 15, 2024 08:05

zucchini-nlp requested a review from qubvel October 15, 2024 08:05

zucchini-nlp added 2 commits October 18, 2024 12:27

fix copies + docs

850a8c7

another fix copies :)

5dc84ee

zucchini-nlp mentioned this pull request Oct 18, 2024

🧼 remove v4.44 deprecations #34245

Merged

This was referenced Oct 27, 2024

Image_token mismatch when applying SigLIP to Llava & LlavaNext. #34447

Closed

LLaVA: latency issues #34460

Merged

qubvel approved these changes Oct 29, 2024

View reviewed changes

zucchini-nlp added 2 commits October 30, 2024 07:48

add docs

8dde9f4

Merge remote-tracking branch 'upstream/main' into vlms-continuatino

4aea55e

qubvel approved these changes Oct 30, 2024

View reviewed changes

zucchini-nlp requested a review from ArthurZucker October 30, 2024 08:44

ArthurZucker reviewed Oct 31, 2024

View reviewed changes

ArthurZucker approved these changes Nov 4, 2024

View reviewed changes

zucchini-nlp added 3 commits November 4, 2024 14:53

move order for BC

0ce015d

Merge remote-tracking branch 'upstream/main' into vlms-continuatino

4d5ca86

Merge branch 'main' into vlms-continuatino

77aac79

zucchini-nlp merged commit 1646ffb into huggingface:main Nov 18, 2024
18 checks passed

Scyther-07 mentioned this pull request Dec 2, 2024

LLaVA OV: fix unpadding precision #34779

Merged

jp1924 mentioned this pull request Dec 3, 2024

Add: num_additional_image_tokens to models #35052

Merged

5 tasks

BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024

VLMs: patch_size -> num_image_tokens in processing (huggingface#3…

81ac59f

…3424) * use num additional tokens * fix copies + docs * another fix copies :) * add docs * move order for BC

VLMs: patch_size -> num_image_tokens in processing #33424

VLMs: patch_size -> num_image_tokens in processing #33424

Conversation

zucchini-nlp commented Sep 11, 2024 • edited Loading

What does this PR do?

HuggingFaceDocBuilderDev commented Sep 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zucchini-nlp commented Sep 23, 2024

amyeroberts commented Oct 2, 2024

zucchini-nlp commented Oct 8, 2024

qubvel commented Oct 8, 2024 • edited Loading

1. patch_size and default +1 for CLS token

2. num_image_tokens

zucchini-nlp commented Oct 9, 2024

qubvel commented Oct 10, 2024

zucchini-nlp commented Oct 11, 2024

zucchini-nlp commented Oct 15, 2024

qubvel left a comment

Choose a reason for hiding this comment

zucchini-nlp commented Oct 29, 2024

zucchini-nlp commented Oct 30, 2024

qubvel left a comment

Choose a reason for hiding this comment

ArthurZucker 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurZucker left a comment

Choose a reason for hiding this comment

zucchini-nlp commented Nov 18, 2024

VLMs: `patch_size` -> `num_image_tokens` in processing #33424

VLMs: `patch_size` -> `num_image_tokens` in processing #33424

zucchini-nlp commented Sep 11, 2024 •

edited

Loading

qubvel commented Oct 8, 2024 •

edited

Loading

1. `patch_size` and default `+1` for CLS token

2. `num_image_tokens`