Skip to content

BLIP: enable generation tests - #34174

Merged
zucchini-nlp merged 16 commits into
huggingface:mainfrom
zucchini-nlp:blip-tests
Nov 1, 2024
Merged

zucchini-nlp merged 16 commits into
huggingface:mainfrom
zucchini-nlp:blip-tests

Conversation

@zucchini-nlp

@zucchini-nlp zucchini-nlp commented Oct 15, 2024 •

Copy link
Copy Markdown
Member

What does this PR do?

Enables generation tests for BLIP models, except BLIP-1 (turned out to be a bit harder). I changed the generation tests to use modelTest.input_name as BLIP is the only model that uses pixel values as main input and thus checking generated text length's will always fail.

I tried to get rid of custom generate for these models, but that opened a Pandora box so I think better not waste time on an old model and maintain it for a while, until the model gets deprecated. But still I did some changes so we don't need to add extra bos at the beginning and now the decoder-based BLIP models return full text at output. Encoder-decoder based models return only generated text, which is consistent with what an LLM should do

@zucchini-nlp zucchini-nlp mentioned this pull request Oct 15, 2024
14 of 16 tasks
@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

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
zucchini-nlp requested review from ArthurZucker and gante and removed request for gante October 15, 2024 12:50
@gante

gante commented Oct 16, 2024 •

Copy link
Copy Markdown
Contributor

changed the generation tests to use modelTest.input_name as BLIP is the only model that uses pixel values as main input and thus checking generated text length's will always fail.

I'd like very much to avoid this change -- extra logic for all tests to handle a niche corner case. Let's brainstorm alternatives! main_input in the generative tests is used to check the shapes. Perhaps we always want to look for input_ids or inputs_embeds in the input dictionary? 🤔

@zucchini-nlp

zucchini-nlp commented Oct 16, 2024 •

Copy link
Copy Markdown
Member Author

@gante I tried to force input_ids always and I found another corner case with Whisper which expects input_features to be the main input for shape checking. I would very much love to make BLIP standard and maybe I'll make so in v5, because it will break a whole lot of things.

OMG, I found an option while writing this reply, whisper and the other audio model are encoder-decoder so we can make it work by getting main input in decoder-only models. Just before the check happens, in the same indent block :)
If you agree, I'll make the change hehe

@ArthurZucker ArthurZucker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AH actually we might need / want to force return_dict to TRUE, to avoid all the if elses

@gante

gante commented Oct 17, 2024

Copy link
Copy Markdown
Contributor

OMG, I found an option while writing this reply, whisper and the other audio model are encoder-decoder so we can make it work by getting main input in decoder-only models. Just before the check happens, in the same indent block :)
If you agree, I'll make the change hehe

if it works, sounds good! (make sure to leave a comment)

@zucchini-nlp

Copy link
Copy Markdown
Member Author

@gante requesting re-review, since the input-name was merged as a separate PR. I rebased main and ran tests again

@ArthurZucker ArthurZucker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for cleaning up

logits = outputs.logits if return_dict else outputs[1]
loss = outputs.loss
logits = outputs.logits
outputs = outputs.to_tuple() if not return_dict else outputs

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

outputs will have loss and logits twice there

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(this was probably already a bug)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yep, in general i don't like that we return it as this and would better return unwrapped lm outputs. But we can't prob just delete it for BC reasons

Comment on lines +640 to +654
models_without_standard_cache = (
"ctrl",
"fsmt",
"gptbigcode",
"mega",
"reformer",
"jamba",
"mamba",
"xlnet",
"zamba",
)
has_standard_cache = not any(
model_name in config.__class__.__name__.lower() for model_name in models_without_standard_cache
)
if has_standard_cache:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

when we overwrite we don't need that no?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah, will remove unnecessary part

@zucchini-nlp
zucchini-nlp merged commit 4cc0813 into huggingface:main Nov 1, 2024
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
* blip2 tests

* instructblips

* copies

* fix slow tests

* fix

* uncomment this

* clean up after rebase

* should be model main input

* fix overwritten tests

* oops len should be multiple of frame number

* style

* fix some tests
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