Skip to content

[blip-2] Fix dtype mismatch when keep in fp32 #37068

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 28, 2025

Conversation

zucchini-nlp
Copy link
Member

@zucchini-nlp zucchini-nlp commented Mar 28, 2025

What does this PR do?

Seems that BLIP2 keep_in_fp32_modules was not picking up correctly until 36722 (thus no errors up to today). Now that the query_tokens are actually kept in 32, we are getting dtype mismatch error in inference. (reported by @hmellor)

This PR fixes by casting inputs to correct dtypes, and keeping qformer in fp32 as well. The fix was tested with slow BLIP-2 tests and vLLM tests

@github-actions github-actions bot marked this pull request as draft March 28, 2025 09:43
Copy link
Contributor

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 Ready for review button (at the bottom of the PR page). This will assign reviewers and trigger CI.

@zucchini-nlp zucchini-nlp requested a review from hmellor March 28, 2025 09:43
@zucchini-nlp zucchini-nlp marked this pull request as ready for review March 28, 2025 09:44
Copy link
Member

@hmellor hmellor left a comment

Choose a reason for hiding this comment

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

LGTM!

I have confirmed that the vLLM test now passes on the latest commit of this PR 🚀

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Let's add a test please! 🤗

@zucchini-nlp
Copy link
Member Author

Test is already added, slow tests were failing on main from the above linked PR. The reason is that we don't usually run slow tests much 🥲

@ArthurZucker
Copy link
Collaborator

Let's merge then!

@zucchini-nlp zucchini-nlp merged commit 52cc204 into huggingface:main Mar 28, 2025
13 of 14 checks passed
@zucchini-nlp zucchini-nlp added the for patch Tag issues / labels that should be included in the next patch label Mar 28, 2025
ArthurZucker pushed a commit that referenced this pull request Mar 28, 2025
* fix fp32 BLIP2

* no need to reorder that

* check for `Noneness` as well before casting dtype
zucchini-nlp added a commit to zucchini-nlp/transformers that referenced this pull request May 14, 2025
* fix fp32 BLIP2

* no need to reorder that

* check for `Noneness` as well before casting dtype
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for patch Tag issues / labels that should be included in the next patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants