Skip to content

[generation] bring back tests on vision models - #38603

Merged
zucchini-nlp merged 2 commits into
huggingface:mainfrom
zucchini-nlp:blip-test-enable-back
Jun 6, 2025
Merged

[generation] bring back tests on vision models#38603
zucchini-nlp merged 2 commits into
huggingface:mainfrom
zucchini-nlp:blip-test-enable-back

Conversation

@zucchini-nlp

@zucchini-nlp zucchini-nlp commented Jun 5, 2025

Copy link
Copy Markdown
Member

What does this PR do?

As per title, now all vision models have their tests back. BLIP skipped intentionally due to complex manipulations in the custom generation. The model is very old and not worth fixing/refactoring

@zucchini-nlp
zucchini-nlp requested review from gante and ydshieh June 5, 2025 07:23
Comment on lines -1430 to -1432
@pytest.mark.generate
def test_generate_with_head_masking(self):
"""Test designed for encoder-decoder models to ensure the attention head masking is used."""

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.

head mask is not supported anymore by models, so we don't need a test (#35786)

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, ok , completely removed.

Comment on lines +191 to +193
self.decoder = DecoderConfig(
vocab_size=tgt_vocab_size, bos_token_id=eos_token_id, is_encoder_decoder=is_encoder_decoder
)

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.

currently we always check for the decoder text config when obtaining vocab_size or is_encoder_decoder. FSTM has a quasi-decoder config which I don't see being used anywhere, but I'm keeping it for BC anyways

Just duplicating the arg here for the tests and for modeling _common.py to use it correctly

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.

As it is not used anywhere, so OK for me.

(But why a decoder ever needs is_encoder_decoder? It should be always False no ? I know I'm naive ..)

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.

It is the internal helper get_text_config() which we now use in many places, to make sure the code works with VLMs. But for FSMT the text_config isn't really a text config but an unused self.decoder. This is creating issues when we want to know if the text backbone of model is encoder_decoder

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.

I don't see it being used anywhere except for generation, and the tests are passing. So we should be good

@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.

@ydshieh ydshieh 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.

LGTM, although I am not super familiar with the new stuff like get_text_config.

I trust/hope you run the tests to make sure they work as expected :-)

Comment thread tests/models/longt5/test_modeling_longt5.py
Comment on lines +191 to +193
self.decoder = DecoderConfig(
vocab_size=tgt_vocab_size, bos_token_id=eos_token_id, is_encoder_decoder=is_encoder_decoder
)

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.

As it is not used anywhere, so OK for me.

(But why a decoder ever needs is_encoder_decoder? It should be always False no ? I know I'm naive ..)

Comment on lines -1430 to -1432
@pytest.mark.generate
def test_generate_with_head_masking(self):
"""Test designed for encoder-decoder models to ensure the attention head masking is used."""

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, ok , completely removed.

@zucchini-nlp
zucchini-nlp enabled auto-merge (squash) June 6, 2025 08:10
@zucchini-nlp
zucchini-nlp merged commit dbfc79c into huggingface:main Jun 6, 2025
bvantuan pushed a commit to bvantuan/transformers that referenced this pull request Jun 12, 2025
* bring back geenration tests on VLMs

* remove head mask tests overwritten
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.

3 participants