Skip to content

Conversation

@younesbelkada
Copy link
Contributor

What does this PR do?

Fixes the current failing instructblip slow test

This commit: 2230d14 was responsible of the test failure however the commit above sillently fixed an issue.

The PR #25105 introduced the correct way of quantizing composable models (blip2, instructblip) and models on the Hub. Before that commit the lm_head of the language model was converted in 8bit, which in fact is wrong. The lm_head should always stay in fp32 for numerical stability and also for consistency with other causal LM models in the library (we keep all lm_head in fp32). Therefore this PR fixes the expected logits and generations

Tested the fix in the latest docker image huggingface/transformers-all-latest-gpu on a 2xNVIDIA T4 and the test pass

cc @ydshieh @amyeroberts

@younesbelkada younesbelkada requested a review from ydshieh August 31, 2023 12:23
@younesbelkada younesbelkada changed the title [InstructBlip] Fix instructblip test [InstructBlip] FINAL Fix instructblip test Aug 31, 2023
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 31, 2023

The documentation is not available anymore as the PR was closed or merged.

@ydshieh
Copy link
Collaborator

ydshieh commented Aug 31, 2023

So #25105 fixed (among other things) that lm_head was in 8bit, right?

@ydshieh
Copy link
Collaborator

ydshieh commented Aug 31, 2023

Confirmed it passes when running the single test, but failed when running the 2 integration tests together.

@amyeroberts I think we can merge, and I will try to figure out why this strange situation happens. WDYT?

@younesbelkada
Copy link
Contributor Author

So #25105 fixed (among other things) that lm_head was in 8bit, right?

Yes, specifically for models that has submodules that are causalLM models, mainly instructblip and blip2

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good on my side. The strange dependency could be addressed separately, but if @younesbelkada is keen to figure it out, also works for me.

@ydshieh ydshieh requested a review from amyeroberts August 31, 2023 13:20
Copy link
Contributor

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into this and the detailed explanation!

@younesbelkada younesbelkada merged commit 9c5acca into huggingface:main Aug 31, 2023
@younesbelkada younesbelkada deleted the fix-instructblip-final branch August 31, 2023 15:01
parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants