has_attentions - consistent test skipping logic and tf tests #17495
Conversation
|
|
||
| else: | ||
| config, inputs_dict = self.model_tester.prepare_config_and_inputs_for_common() | ||
| seq_len = getattr(self.model_tester, "seq_length", None) |
There was a problem hiding this comment.
I'm not sure why the diff is so confusing for this file (possibly blank lines?). The only change is:
- Removing the if / else statement
- Un-indenting the logic in the else block
Apologies for the difficulty this introduces when reviewing.
There was a problem hiding this comment.
Use a GUI tool for review is OK here.
|
The documentation is not available anymore as the PR was closed or merged. |
|
@NielsRogge, @ydshieh, @patrickvonplaten adding you to cover: git blame ownership, test ownership, some git blame and general transformers ownership. I hope that's OK. Feel free to remove yourselves and/or add others you think are more suitable. |
|
|
||
| else: | ||
| config, inputs_dict = self.model_tester.prepare_config_and_inputs_for_common() | ||
| seq_len = getattr(self.model_tester, "seq_length", None) |
There was a problem hiding this comment.
Use a GUI tool for review is OK here.
| tuple_inputs = self._prepare_for_class(inputs_dict, model_class, return_labels=True) | ||
| dict_inputs = self._prepare_for_class(inputs_dict, model_class, return_labels=True) | ||
| check_equivalence(model, tuple_inputs, dict_inputs, {"output_attentions": True}) | ||
| if self.has_attentions: |
There was a problem hiding this comment.
What happens for this test before this PR? It failed? I mean previously we didn't have condition if self.has_attentions:
There was a problem hiding this comment.
Pretty much - a tensorflow model would fail when equivalent tests to a PyTorch model without attention layers were implemented. The same logic I'm adding here are in test_modeling_common.py. I found this when working on the ResNet port. All of the models this affects only have PyTorch implementations apart from convnext which uses unittest.skip in its tests, so there's no failing tests at the moment on main.
There was a problem hiding this comment.
All yes (I somehow think unconsciously all the models have TF version) 😄
|
Thanks a lot for cleaning this up @amyeroberts ! |
|
@NielsRogge I decide not to remove |
What does this PR do?
Two linked changes regarding the control of tests being run:
PyTorch and TF consistency:
has_attentionsflag is used intest_modeling_common.pyto control some of the logic in tests that are run, only applying them if the model has attention(s). This PR adds equivalent logic to tests intest_modeling_tf_common.pySkipping tests consistency:
unittest.skipis used to skip entire tests if they cannot/do not apply to that model e.g. for input embedding test in ConvNext. Fortest_attention_outputsthis was controlled with an if-else statement. This was changed to be controlled withunittest.skipinstead for two reasons: 1) consistency with the rest of the code base 2) prevent confusing pytest outputs i.e. models without attention are shown to skiptest_attention_outputsinstead of passing it.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.