[tests] fix flaky pattern in test_generate_continue_from_past_key_values#37724
Conversation
|
Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. The CI will be paused while the PR is in draft mode. When it is ready for review, please click the |
gante
changed the title
test_generate_continue_from_past_key_values
| @parameterized.expand([("offloaded",)]) # ("offloaded_static",) TODO: @raushan fixme in some models (eg T5) | ||
| @require_torch_accelerator | ||
| @pytest.mark.generate | ||
| def test_offloaded_cache_implementation(self, cache_implementation): |
There was a problem hiding this comment.
this test doesn't make sense? tests that two generate calls with the same flags have the same output
We have tests for the offloaded cache in tests/utils/test_cache_utils.py
There was a problem hiding this comment.
Right, this test was supposed to only check that calling genarate with all possible cache_implementation doesn't fail. There was some issue in the past, we lost some "cache_implementation" keys when refactoring 🥲
If the tests in cache_utils cover all caches and call generate with kwargs, I guess we're fine
There was a problem hiding this comment.
There are slow integration tests for all caches on main, this PR adds a fast test for all caches 🤗
| "max_new_tokens": self.max_new_tokens, | ||
| "use_cache": True, | ||
| "return_dict_in_generate": True, | ||
| "output_scores": True, |
There was a problem hiding this comment.
(the correct equivalence check needs the scores)
| "use_cache": True, | ||
| "do_sample": False, | ||
| "return_dict_in_generate": True, | ||
| "output_scores": True, |
There was a problem hiding this comment.
(same, the correct equivalence check needs the scores)
gante
changed the title
test_generate_continue_from_past_key_values test_generate_continue_from_past_key_values
|
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. |
| # TODO (joao, raushan): the correct line below is `if not hasattr(config.get_text_config(), "use_cache")`, | ||
| # but it breaks a few models. Fix and then apply `_check_similar_generate_outputs` pattern |
There was a problem hiding this comment.
prob it is same thing when we generate from inputs_embeds in VLMs and have to pop image inputs
There was a problem hiding this comment.
Yup 👍 I've pushed the actual fix into the future, in case the test is not (much) flaky
| @parameterized.expand([("offloaded",)]) # ("offloaded_static",) TODO: @raushan fixme in some models (eg T5) | ||
| @require_torch_accelerator | ||
| @pytest.mark.generate | ||
| def test_offloaded_cache_implementation(self, cache_implementation): |
There was a problem hiding this comment.
Right, this test was supposed to only check that calling genarate with all possible cache_implementation doesn't fail. There was some issue in the past, we lost some "cache_implementation" keys when refactoring 🥲
If the tests in cache_utils cover all caches and call generate with kwargs, I guess we're fine
3 participants
What does this PR do?
Fixes the flakiness pattern behind a common flaky test,
tests/models/kosmos2/test_modeling_kosmos2.py::Kosmos2ModelTest::test_generate_continue_from_past_key_valuesflakiness pattern TL;DR:
generate, tiny differences in the logits can lead to a different token, changing generation from that point onwardgenerateshouldn't naively look at the output sequences, but rather at the output sequences up to the point where they diverge, if they diverge 👉 corrects all equivalence checks using the right pattern (this pattern was already present in most tests, but a few ones still had the old pattern)