-
Notifications
You must be signed in to change notification settings - Fork 30.6k
[tests] fix blip2 edge case #40699
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
[tests] fix blip2 edge case #40699
Conversation
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. |
run-slow: blip_2 |
This comment contains run-slow, running the specified jobs: models: ['models/blip_2'] |
"I will remove it now.\n\n" | ||
"See https://github.com/pypa/pip/issues/5466 for details.\n" | ||
).format(stale_egg_info) | ||
f"Warning: {stale_egg_info} exists.\n\n" |
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.
make fixup corrected this 👀
pass | ||
|
||
@parameterized.expand(TEST_EAGER_MATCHES_SDPA_INFERENCE_PARAMETERIZATION) | ||
@unittest.skip("Won't fix: Blip2 + T5 backbone needs custom input preparation for this test") |
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.
can be fixed, but I don't think it's worth it 👀
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.
agreed, let's assume that t5 passes the test so we are fine :)
run-slow: blip_2 |
This comment contains run-slow, running the specified jobs: models: ['models/blip_2'] |
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.
Thanks, it is much better to have it in model config level. Can you also verify how Florence2 works with this PR? Those are the only two VLMs with encoder-decoder backbone afaik
|
||
if model.config.get_text_config(decoder=True).is_encoder_decoder: | ||
if model.config.is_encoder_decoder: | ||
self.assertTrue(output_generate.sequences.shape[1] == self.max_new_tokens + 1) |
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.
cool thanks, can you check if Florence2 model works with these changes? It is same as blip and uses Bart as backbone
pass | ||
|
||
@parameterized.expand(TEST_EAGER_MATCHES_SDPA_INFERENCE_PARAMETERIZATION) | ||
@unittest.skip("Won't fix: Blip2 + T5 backbone needs custom input preparation for this test") |
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.
agreed, let's assume that t5 passes the test so we are fine :)
[0, 3, 7, 152, 2515, 11389, 3523, 1], | ||
"san francisco", # TODO: check if this is ok | ||
), | ||
("cuda", None): ([0, 3, 7, 152, 2515, 11389, 3523, 1], "san francisco"), |
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.
i believe the CUDA expectation incorrect from the beginning
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.
yes, I agree 👀
run-slow: florence2 |
This comment contains run-slow, running the specified jobs: models: ['models/florence2'] |
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.
Forgot to ✅
@zucchini-nlp florence2 slow tests were mostly green (there were 3 FA2 failures, which are unrelated to this PR) ✅ In florence2, |
[For maintainers] Suggested jobs to run (before merge) run-slow: blip_2 |
What does this PR do?
(Carved from #40553, which is becoming messy)
Our tests have the
model.config.get_text_config(decoder=True).is_encoder_decoder
pattern, which doesn't make sense -- we're pulling the decoder if it exists, and then checking if it is encoder-decoder.The pattern exists because of
blip2
, which wasn't settingis_encoder_decoder
correctly -- if its LLM is an encoder-decoder model, then it is also an encoder-decoder model.✅ blip 2 slow tests are passing