Do not remove half seq length in generation tests #30016
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. |
| @@ -752,7 +752,7 @@ def test_attention_outputs(self): | |||
|
|
|||
| def _check_encoder_attention_for_generate(self, attentions, batch_size, config, seq_length): | |||
| block_len = getattr(self.model_tester, "block_len", None) | |||
| encoder_expected_shape = (batch_size, 1, config.num_attention_heads, block_len, 3 * block_len) | |||
| encoder_expected_shape = (batch_size, 2, config.num_attention_heads, block_len, 3 * block_len) | |||
There was a problem hiding this comment.
is this because of the input_ids shape change?
There was a problem hiding this comment.
yes, I guess we can hardcode it this way since previously "1" was also hardcoded
There was a problem hiding this comment.
You mean bringing back "1"? That causes test failures...
|
@zucchini-nlp Can you trigger all tests, to double-check? (i.e. commit message = "[test_all] ...") |
Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com>
|
All tests are passing, except for the attached PR and another which seemingly failed to download files. |
| @@ -691,40 +667,39 @@ def test_model_parallel_beam_search(self): | |||
| new_model.generate( | |||
| input_ids, | |||
| attention_mask=attention_mask, | |||
| max_length=max_length, | |||
| max_new_tokens=self.max_new_tokens, | |||
There was a problem hiding this comment.
everywhere we call model.generate, if we pas this max_new_token, why not in the setup update the model.generation_config.max_new_tokens?
There was a problem hiding this comment.
Hmm, you mean the "setup" in each model's test file, so that we already have the "max_new_tokens" linked to model? That means we need to initialize model from config when calling _get_input_ids_and_config and then set model.generation_config.max_new_tokens.
I can do it, seems like it will not cause any errors. But I am not sure if that's what you mean
Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
* remove seq length from generation tests * style and quality * [test_all] & PR suggestion Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com> * Update tests/generation/test_utils.py Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com> * [test all] remove unused variables --------- Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com> Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
What does this PR do?
Generation tests divided input ids into half, feeding only half-seq-length for generation. But this strategy may cause difficulties for multimodal models, which have dependency between images count and input ids. For ex, Llava models include special image tokens in input ids, and removing half-seq-length might result in failing test.
This is part of a work to add GenerationTesterMixin in multimodal models, which are not covered with tests now.
This PR:
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@gante