-
Notifications
You must be signed in to change notification settings - Fork 31.2k
Fix: Guard against None num_query_tokens in Blip2Processor (to avoid TypeError) #42311
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
base: main
Are you sure you want to change the base?
Conversation
|
cc @zucchini-nlp! I think it's fine without the warning message, but deferring to her on what behaviour we want from processors 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Flakes342 thanks for the PR!
To support older models from the hub which weren't updated, I think we should update the default value of num_query_tokens.
The modeling code assumes that input_ids have special image placeholders and with self.num_query_tokens=None the placeholders will not be added in the input text. Therefore the model will not see an input image
Let's add the default value of 32 which is the only value used in all official checkpoints. Ping me for another review when ready :)
|
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. |
|
@zucchini-nlp thr pr is ready for your review |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: blip_2 |
|
run-slow: blip_2 |
| Number of tokens used by the Qformer as queries, should be same as in model's config. | ||
| """ | ||
|
|
||
| def __init__(self, image_processor, tokenizer, num_query_tokens=None, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I wasn't clear, I meant the default value in the signature here
|
This comment contains models: ["models/blip_2"] |
CI ResultsModel CI Report❌ Failed tests
|
What does this PR do?
Fixes #42203
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Fixed a bug where Blip2Processor assumes num_query_tokens is an int and does
max_length - self.num_query_tokenswhich raises TypeError whennum_query_tokensis None.This change:
Who can review?
@Rocketknight1