[whisper] small changes for faster tests #38236
Conversation
| @require_torchaudio | ||
| class WhisperModelIntegrationTests(unittest.TestCase): | ||
| def setUp(self): | ||
| self._unpatched_generation_mixin_generate = transformers.GenerationMixin.generate |
There was a problem hiding this comment.
this was only used in one test, and that test should use the base generate. This overcomplicates things.
idk why it was added in the first place 🤔
There was a problem hiding this comment.
It is linked to #29312, but the change here is only test. So good for me if it works.
There was a problem hiding this comment.
That is implicitly tested: the output checked in the test is different if num_beams is not respected :)
(just like in all other beam search integration tests: we check the output, which is sensible enough to detect bad usage of flags)
There was a problem hiding this comment.
It's definitely way over the top for what it tried. So yea let's keep it simple.
|
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. |
| transformers.GenerationMixin.generate = self._unpatched_generation_mixin_generate | ||
|
|
||
| @cached_property | ||
| def default_processor(self): |
| torch_device = "cpu" | ||
| set_seed(0) |
There was a problem hiding this comment.
could you run with flakefinder to see if we need seed or not?
There was a problem hiding this comment.
RUN_SLOW=1 py.test tests/models/whisper/test_modeling_whisper.py -k test_tiny_en_generation --flake-finder --flake-runs 100 yields no failures
(set_seed(0) comes from the original whisper commit. However, AFAIK, Whisper has no random components -- in fact, many output-checking tests in this file don't set a seed)
ydshieh
left a comment
ydshieh
left a comment
There was a problem hiding this comment.
LGTM, but it would be good to check a few things.
tearDownClass is a good practice to cleanup the stuff.
|
@ydshieh regarding dataset loading: I've remembered that we had the same issue in the pipeline tests, so I've pushed a new commit which copies the pattern from the pipeline tests The pattern we added before doesn't have a teardown method -- do we need to add it everywhere the pattern is used? |
|
@gante Thank you for checking and further improvement. Regarding (Those loaded objects assigned to class attributes will remain forever until the python process exit. At some point, it might be a problem if we have many test classes) |
vasqu
left a comment
vasqu
left a comment
There was a problem hiding this comment.
LGTM, thanks. Was only concerned about the seeds but seems to be a non-issue.
| @require_torchaudio | ||
| class WhisperModelIntegrationTests(unittest.TestCase): | ||
| def setUp(self): | ||
| self._unpatched_generation_mixin_generate = transformers.GenerationMixin.generate |
There was a problem hiding this comment.
It's definitely way over the top for what it tried. So yea let's keep it simple.
4 participants
What does this PR do?
whisper has a very slow set of tests.
This PR applies low-hanging fruit changes to get faster tests (slow tests: 7m47s -> 6m59s on my machine)