Skip to content

Conversation

@NielsRogge
Copy link
Contributor

@NielsRogge NielsRogge commented Mar 3, 2022

What does this PR do?

The library is called HuggingFace Transformers, I know.. but we recently have additions of non-Transformer based models, that don't use attention (namely ConvNeXT and PoolFormer). Soon, we'll also have ResNet in the library (which will back Transformer-based models such as DETR).

As these models don't use attention, they need to overwrite 3 tests:

  • test_attention_outputs
  • test_retain_grad_hidden_states_attentions
  • test_model_outputs_equivalence

This PR adds a has_attentions attribute to ModelTesterMixin which can be set to False for such models. It will then make sure these tests are properly tested, without taking into account attention.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 3, 2022

The documentation is not available anymore as the PR was closed or merged.

Copy link
Contributor

@FrancescoSaverioZuppichini FrancescoSaverioZuppichini left a comment

Choose a reason for hiding this comment

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

Thanks! I am not super familiar with this codebase so I am not going to approve it but LGTM!

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @NielsRogge

Pinging @ydshieh as well for second review

@LysandreJik
Copy link
Member

You'll need to fix the failing tests as well.

@LysandreJik LysandreJik requested a review from ydshieh March 8, 2022 09:20
@ydshieh
Copy link
Collaborator

ydshieh commented Mar 8, 2022

Thanks for this work @NielsRogge !

I left a review comment (question) about test_forward_signature.

And then a minor question here:

https://github.com/huggingface/transformers/blob/114b62cc9927d85804540d46a6f85e57ba13ea51/tests/test_modeling_common.py#L1046-L1049

It is a bit strange (to me) to have config.output_attentions = True without any condition like if self.has_attentions:. WDYT?

@NielsRogge
Copy link
Contributor Author

@LysandreJik I've fixed the failing tests, failing test seems unrelated

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

LGTM!

@LysandreJik
Copy link
Member

Feel free to merge if every test passes after a rebase

@NielsRogge NielsRogge force-pushed the add_attentions_option branch from 2192fef to 904d957 Compare March 10, 2022 09:29
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.

5 participants