🚨 Add Blip2ForImageTextRetrieval #29261

jpizarrom · 2024-02-23T19:37:53Z

What does this PR do?

Add Blip2ForImageTextRetrieval, Blip2TextModelWithProjection, Blip2VisionModelWithProjection models to be able to get Image Text Matching scores, and extract text,image,multimodal features.

Fixes part of #25300 #25245

This is continuation of #25612, I tried to apply most of the feedback received in that PR.

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline,
Pull Request section?
Was this discussed/approved via a Github issue or the forum? Please add a link
to it if that's the case.
Did you make sure to update the documentation with your changes? Here are the
documentation guidelines, and
here are tips on formatting docstrings.
Did you write any new necessary tests?

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.

@ArthurZucker @amyeroberts

src/transformers/models/blip_2/modeling_blip_2.py

ArthurZucker · 2024-02-27T06:23:14Z

cc @NielsRogge and @younesbelkada if one of you want to review on @jpizarrom makes the CIs go green!

jpizarrom · 2024-03-09T06:46:32Z

cc @NielsRogge and @younesbelkada if one of you want to review on @jpizarrom makes the CIs go green!

Hi, what could I do to makes the CIs go green! shall I just merge to upstream/main, or rebase to it?

amyeroberts · 2024-03-11T12:17:07Z

@jpizarrom It's preferable for you to rebase onto main. To see how to make the CIs green, you'll need to click on details and look at the output error logs from circleci. I'd suggest doing this after rebasing so see which errors are coming from this branch.

amyeroberts

Thanks for adding this! Overall looks great, just a few small comments

Once they're addressed we can move the checkpoints to be under the salesforce org

amyeroberts · 2024-03-27T12:06:48Z

src/transformers/models/blip_2/configuration_blip_2.py

+    @classmethod
+    def from_vision_qformer_configs(
+        cls,
+        vision_config: Blip2VisionConfig,
+        qformer_config: Blip2QFormerConfig,
+        **kwargs,
+    ):
+        r"""
+        Instantiate a [`Blip2Config`] (or a derived class) from a BLIP-2 vision and Q-Former model configurations.
+
+        Returns:
+            [`Blip2Config`]: An instance of a configuration object
+        """
+
+        return cls(
+            vision_config=vision_config.to_dict(),
+            qformer_config=qformer_config.to_dict(),
+            **kwargs,
+        )


I don't think it's necessary to add a separate method here. We can just make text_config optional in from_vision_qformer_text_config

from_vision_qformer_configs was removed

src/transformers/models/blip_2/modeling_blip_2.py

amyeroberts · 2024-04-04T12:49:00Z

src/transformers/models/blip_2/modeling_blip_2.py

+        if self.device != torch.device("cpu"):
+            with torch.cuda.amp.autocast(dtype=torch.float16):
+                vision_outputs = self.vision_model(
+                    pixel_values=pixel_values,
+                    output_attentions=output_attentions,
+                    output_hidden_states=output_hidden_states,
+                    return_dict=return_dict,
+                )
+        else:
+            vision_outputs = self.vision_model(
+                pixel_values=pixel_values,
+                output_attentions=output_attentions,
+                output_hidden_states=output_hidden_states,
+                return_dict=return_dict,
+            )


Autocasting and typing should be handled outside of the model definition

Suggested change

if self.device != torch.device("cpu"):

with torch.cuda.amp.autocast(dtype=torch.float16):

vision_outputs = self.vision_model(

pixel_values=pixel_values,

output_attentions=output_attentions,

output_hidden_states=output_hidden_states,

return_dict=return_dict,

)

else:

vision_outputs = self.vision_model(

pixel_values=pixel_values,

output_attentions=output_attentions,

output_hidden_states=output_hidden_states,

return_dict=return_dict,

)

vision_outputs = self.vision_model(

pixel_values=pixel_values,

output_attentions=output_attentions,

output_hidden_states=output_hidden_states,

return_dict=return_dict,

)

this was done because in the original model the autocast was applied only to the vision layers, don't know yet how to do this in a different way.

https://github.com/salesforce/LAVIS/blob/ac8fc98c93c02e2dfb727e24a361c4c309c8dbbc/lavis/models/blip2_models/blip2_qformer.py#L423-L424

cc @amyeroberts

it was removed, as discussed in #29261 (comment)

tests/models/blip_2/test_modeling_blip_2.py

src/transformers/models/blip_2/modeling_blip_2.py

amyeroberts · 2024-04-04T13:19:00Z

src/transformers/models/blip_2/modeling_blip_2.py

+        if config.use_qformer_text_input:
+            self.embeddings = Blip2TextEmbeddings(config)


Instead of using this config argument to conditionally call and create this layer, I'd suggest instead call self.embeddings if input_ids is not None

Suggested change

if config.use_qformer_text_input:

self.embeddings = Blip2TextEmbeddings(config)

self.embeddings = Blip2TextEmbeddings(config)

when this layer is created_always_, I got this type of errors, don't know how to fix them.
Some Blip2 models do not use this bert based embeddings, they use opt or flan-t5 to create the query_embeds. Maybe I could try to refactor the code to move the Blip2TextEmbeddings outside of Blip2QFormerModel and pass always query_embeds. what do you think?

FAILED tests/models/blip_2/test_modeling_blip_2.py::Blip2ForConditionalGenerationDecoderOnlyTest::test_training_gradient_checkpointing - AssertionError: False is not true : qformer.embeddings.word_embeddings.weight in Blip2ForConditionalGeneration has no gradient!

FAILED tests/models/blip_2/test_modeling_blip_2.py::Blip2ModelTest::test_training_gradient_checkpointing - AssertionError: False is not true : qformer.embeddings.word_embeddings.weight in Blip2ForConditionalGeneration has no gradient!

I did a refactor, embeddings were removed from Blip2QFormerModel, and place them into Blip2ForImageTextRetrieval and Blip2TextModelWithProjection, but to do so i needed to add query_length param to Blip2QFormerModel.forward.

amyeroberts · 2024-04-04T13:19:24Z

src/transformers/models/blip_2/modeling_blip_2.py

        # past_key_values_length
        past_key_values_length = (
            past_key_values[0][0].shape[2] - self.config.query_length if past_key_values is not None else 0
        )

        query_length = query_embeds.shape[1] if query_embeds is not None else 0

-        embedding_output = self.layernorm(query_embeds)
+        if self.config.use_qformer_text_input:


Suggested change

if self.config.use_qformer_text_input:

if input_ids is not None:

this is outdated, because embeddings were removed from Blip2QFormerModel

amyeroberts · 2024-04-04T13:19:56Z

src/transformers/models/blip_2/modeling_blip_2.py

+        # TODO: maybe have a cleaner way to cast the input (from `Blip2Processor` side?)
+        expected_dtype = self.dtype
+        if encoder_hidden_states is not None and encoder_hidden_states.dtype != expected_dtype:
+            encoder_hidden_states = encoder_hidden_states.to(expected_dtype)


Is this even necessary?

Should not be necessary indeed given that modeling code is by default in torch.float32

it was removed

github-actions · 2024-04-30T08:05:02Z

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

NielsRogge · 2024-05-01T14:45:36Z

src/transformers/models/blip_2/modeling_blip_2.py

+        )
+
+        if self.device != torch.device("cpu"):
+            with torch.cuda.amp.autocast(dtype=torch.float16):


As far as I can tell we don't add torch.cuda.amp.autocast code to modeling files, they are just in float32 by default. This was discussed on the original BLIP-2 model addition PR from what I remember. It's up to users to call something like torch.cuda.amp.autocast themselves if they wish to load the model in a different precision than the default one (cc @younesbelkada).

Hence in the conversion script I casted both the original weights and my BLIP-2 implementation to float32 in order to verify the conversion.

ok, so this means that i need to remove maybe_autocast from https://github.com/NielsRogge/LAVIS/blob/blip2_float32/lavis/models/blip2_models/blip2_image_text_matching.py#L57-L58, right?

Yes that's right

it was removed, a PR on your fork was opened to also remove the autocast from the ITM model NielsRogge/LAVIS#1

NielsRogge · 2024-05-01T14:50:18Z

src/transformers/models/blip_2/modeling_blip_2.py

@@ -84,6 +84,99 @@ def to_tuple(self) -> Tuple[Any]:
        )


+@dataclass
+class Blip2ImageTextMatchingModelOutput(ModelOutput):


Not sure if feasible, but it'd be nice to match the output class of CLIP, which is also an image-text matching model. It consists of the following keys:

loss

logits_per_image (this I assume is the itm_score)

logits_per_text (this I assume is the itm_score transposed)

and some other keys which are CLIP-specific.

Making sure that Blip2ForImageTextRetrieval matches this would allow it to be added to the zero-shot image classification pipeline, which relies on this output key:

transformers/src/transformers/pipelines/zero_shot_image_classification.py

Line 143 in bbaa8ce

"logits": outputs.logits_per_image,

Otherwise we will have a hard time adding BLIP-2 support to the zero-shot image classification pipeline.

Hi @NielsRogge, i updated the output to match CLIP output, but this PR is not being updated with my latest commits

NielsRogge

Thanks for your work! Would request some changes however in order to be able to make BLIP-2 compatible with the zero-shot image classification pipeline.

src/transformers/models/blip_2/modeling_blip_2.py

NielsRogge · 2024-05-01T15:01:24Z

src/transformers/models/blip_2/modeling_blip_2.py

+        input_ids: Optional[torch.FloatTensor] = None,
+        position_ids: Optional[torch.LongTensor] = None,
+        query_embeds: Optional[torch.FloatTensor] = None,
+        past_key_values_length: int = 0,


Suggested change

past_key_values_length: int = 0,

past_key_values are not used I assume

it was removed. thanks

…t_retrieval_model

NielsRogge · 2024-08-11T12:03:01Z

tests/models/blip_2/test_modeling_blip_2.py

@@ -704,6 +716,16 @@ class Blip2ModelTest(ModelTesterMixin, PipelineTesterMixin, GenerationTesterMixi
    test_attention_outputs = False
    test_torchscript = False

+    # TODO: Fix the failed tests


To be addressed?

Not on this PR, I don't believe it is related to the changes of this PR, Blip2ForConditionalGeneration fails there, but i will verify that.

I wanted to make the test pass, and leave a comment about this, as i saw similar comments on other models.

this is the error i was getting, it also occurs on the main branch

FAILED tests/models/blip_2/test_modeling_blip_2.py::Blip2ModelTest::test_pipeline_visual_question_answering_fp16 - RuntimeError: "LayerNormKernelImpl" not implemented for 'Half'

@jpizarrom Could you open an issue to track this to make sure this isn't lost?

Refer to this comment on why the test fails. You're probably running this on CPU.

src/transformers/models/blip_2/modeling_blip_2.py

…t_retrieval_model

jpizarrom · 2024-08-13T19:21:05Z

src/transformers/models/blip_2/modeling_blip_2.py

@@ -304,7 +401,13 @@ class Blip2PreTrainedModel(PreTrainedModel):
    config_class = Blip2Config
    base_model_prefix = "blip"
    supports_gradient_checkpointing = True
-    _no_split_modules = ["Blip2Attention", "T5Block", "OPTDecoderLayer"]
+    _no_split_modules = [


@NielsRogge I updated _no_split_modules, i hope this will fix the slow multi-gpu tests that were failing.

jpizarrom · 2024-08-15T06:31:08Z

Hi @amyeroberts, now the slow tests are passing. please let me know if I need to make any more changes.

atomwalk12 · 2024-08-20T18:31:03Z

Please excuse me as I am not OP. However, I have an inquiry about this feature as I would really like to be able to use it. Generally, how much time does it take once a feature has been merged into the main branch to be made available in the next release?

amyeroberts · 2024-08-22T14:08:42Z

@jpizarrom Was an issue created to track the failing test c.f. this comment: #29261 (comment)

jpizarrom · 2024-08-22T16:04:26Z

@jpizarrom Was an issue created to track the failing test c.f. this comment: #29261 (comment)

Not yet, I can do it, but I don't have my computer with me until the first week of September.

* add Blip2ForImageTextRetrieval * use one line and remove unnecessary space in tests Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com> * use value from the config, rather than hardcoded * change order of params in Blip2QFormerModel.forward * update docstring * fix style * update test_inference_opt * move embeddings out of Blip2QFormerModel * remove from_vision_qformer_configs * remove autocast float16 in Blip2QFormerModel * rename fiels into vision_projection,text_projection,use_image_text_matching_head * use CLIPOutput for Blip2ImageTextMatchingModelOutput * remove past_key_values_length from Blip2TextEmbeddings * fix small typo in the CLIPOutput docstring * add Blip2ForImageTextRetrieval to Zero Shot Image Classification mapping * update docstring and add require_torch_fp16 * rollback test_inference_opt * use use_image_text_matching_head=True in convert * skip test_model_get_set_embeddings * fix create_rename_keys error on new itm fields * revert to do scale after dot product between "query" and "key" * fix ValueError on convert script for blip2-opt-2.7b * update org of paths to Salesforce * add is_pipeline_test_to_skip for VisualQuestionAnsweringPipelineTests * [run_slow] blip_2 * removed Blip2ForImageTextRetrieval from IGNORE_NON_AUTO_CONFIGURED * fix docstring of Blip2ImageTextMatchingModelOutput * [run_slow] blip_2 * fix multi-gpu tests * [run_slow] blip_2 * [run_slow] blip_2 --------- Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

KilianKW · 2024-10-09T07:03:10Z

@amyeroberts @jpizarrom Thanks a lot for adding this feature! I've noticed that the relevant model weights hosted here are still missing some license information. Do you need a dedicated ticket for that or is this post enough?

jpizarrom · 2024-10-13T09:00:15Z

@amyeroberts @jpizarrom Thanks a lot for adding this feature! I've noticed that the relevant model weights hosted here are still missing some license information. Do you need a dedicated ticket for that or is this post enough?

Hi, i am not sure, other blip2 models like Salesforce/blip2-opt-2.7b-coco show MIT, but in the repo LAVIS there is BSD 3-Clause License

jpizarrom marked this pull request as draft February 23, 2024 19:41

jpizarrom commented Feb 23, 2024

View reviewed changes

src/transformers/models/blip_2/modeling_blip_2.py Outdated Show resolved Hide resolved

jpizarrom marked this pull request as ready for review February 23, 2024 20:14

jpizarrom changed the title ~~WIP Add Blip2ForImageTextRetrieval~~ Add Blip2ForImageTextRetrieval Feb 23, 2024

jpizarrom changed the title ~~Add Blip2ForImageTextRetrieval~~ 🚨 Add Blip2ForImageTextRetrieval Mar 2, 2024

jpizarrom force-pushed the add_blip2_image_text_retrieval_model branch from 0e82065 to 9aa9a15 Compare March 22, 2024 15:16

jpizarrom requested a review from amyeroberts March 22, 2024 15:43

amyeroberts reviewed Apr 4, 2024

View reviewed changes

jpizarrom and others added 4 commits May 1, 2024 08:43

add Blip2ForImageTextRetrieval

e106340

use one line and remove unnecessary space in tests

164d824

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

use value from the config, rather than hardcoded

715f6fa

change order of params in Blip2QFormerModel.forward

da0cc83

jpizarrom changed the title ~~🚨 Add Blip2ForImageTextRetrieval~~ WIP 🚨 Add Blip2ForImageTextRetrieval May 1, 2024

jpizarrom force-pushed the add_blip2_image_text_retrieval_model branch from 05327aa to da0cc83 Compare May 1, 2024 06:57

jpizarrom added 4 commits May 1, 2024 09:29

update docstring

02a0e08

fix style

360f537

update test_inference_opt

5e7764f

move embeddings out of Blip2QFormerModel

7e7135a

NielsRogge reviewed May 1, 2024

View reviewed changes

NielsRogge requested changes May 1, 2024

View reviewed changes

remove from_vision_qformer_configs

258f349

NielsRogge reviewed May 1, 2024

View reviewed changes

src/transformers/models/blip_2/modeling_blip_2.py Outdated Show resolved Hide resolved

NielsRogge reviewed May 1, 2024

View reviewed changes

src/transformers/models/blip_2/modeling_blip_2.py Outdated Show resolved Hide resolved

NielsRogge reviewed May 1, 2024

View reviewed changes

remove autocast float16 in Blip2QFormerModel

cf42e9b

jpizarrom added 5 commits August 9, 2024 21:51

update org of paths to Salesforce

2534ca3

Merge remote-tracking branch 'upstream/main' into add_blip2_image_tex…

af53840

…t_retrieval_model

add is_pipeline_test_to_skip for VisualQuestionAnsweringPipelineTests

f841c59

[run_slow] blip_2

1720ba4

removed Blip2ForImageTextRetrieval from IGNORE_NON_AUTO_CONFIGURED

11f0579

jpizarrom requested a review from NielsRogge August 11, 2024 09:07

NielsRogge reviewed Aug 11, 2024

View reviewed changes

src/transformers/models/blip_2/modeling_blip_2.py Outdated Show resolved Hide resolved

jpizarrom added 7 commits August 11, 2024 16:49

fix docstring of Blip2ImageTextMatchingModelOutput

5e63cdb

[run_slow] blip_2

95232f8

fix multi-gpu tests

04bb860

[run_slow] blip_2

5c66e50

Merge remote-tracking branch 'upstream/main' into add_blip2_image_tex…

2419c85

…t_retrieval_model

Merge remote-tracking branch 'upstream/main' into add_blip2_image_tex…

53abc06

…t_retrieval_model

[run_slow] blip_2

d32e889

jpizarrom commented Aug 13, 2024

View reviewed changes

amyeroberts merged commit 7591ca5 into huggingface:main Aug 27, 2024
25 checks passed

NielsRogge mentioned this pull request Aug 28, 2024

[Zero-shot image classification pipeline] Remove tokenizer_kwargs #33174

Open

100 mentioned this pull request Sep 6, 2024

🚨 Fix torch.jit.trace for interpolate_pos_encoding in all vision models #33226

Merged

5 tasks

jpizarrom deleted the add_blip2_image_text_retrieval_model branch October 13, 2024 09:05

		if config.use_qformer_text_input:
		self.embeddings = Blip2TextEmbeddings(config)

	if self.config.use_qformer_text_input:
	if input_ids is not None:

🚨 Add Blip2ForImageTextRetrieval #29261

🚨 Add Blip2ForImageTextRetrieval #29261

Conversation

jpizarrom commented Feb 23, 2024 • edited Loading

What does this PR do?

Before submitting

Who can review?

ArthurZucker commented Feb 27, 2024

jpizarrom commented Mar 9, 2024

amyeroberts commented Mar 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

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

github-actions bot commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpizarrom May 1, 2024 • edited Loading

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

NielsRogge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpizarrom May 18, 2024 • edited Loading

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

jpizarrom commented Aug 15, 2024

atomwalk12 commented Aug 20, 2024

amyeroberts commented Aug 22, 2024

jpizarrom commented Aug 22, 2024

KilianKW commented Oct 9, 2024

jpizarrom commented Oct 13, 2024

jpizarrom commented Feb 23, 2024 •

edited

Loading

jpizarrom May 1, 2024 •

edited

Loading

jpizarrom May 18, 2024 •

edited

Loading