-
Notifications
You must be signed in to change notification settings - Fork 31.3k
Reduce timing on CircleCI - part 1 (Use @slow for IntegrationTests) #42206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
[For maintainers] Suggested jobs to run (before merge) run-slow: aria, bark, hiera, imagegpt, jamba, llava, mamba, mbart, olmo2, plbart, rag, rt_detr, rt_detr_v2, speech_to_text, speecht5, tvp |
|
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. |
Cyrilvallez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes a lot of sense, except the imagegpt one! I think this is unrelated!
| # After #33632, this test still passes, but many subsequential tests fail with `device-side assert triggered`. | ||
| # We need to put `@slow` whenever `run_test_using_subprocess` is used. | ||
| @slow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this can can the case when this is run on the CI? Because it does not have any accelerator available so no device-side assert triggered
Probably the issue is only the @run_test_using_subprocess which should not be necesary here I think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is from my previous PR #34213, and I am sure I did the investigation at that time :-).
This test call the one in the parent class GenerationTesterMixin in the file tests/generation/test_utils.py and it does use torch_device.
Of course, when running on CircleCI, the torch_device is CPU and the comment doesn't apply in this case. But we still need put run_test_using_subprocess so the problem won't occur when it's running on GPU enviornment.
I can of course make run_test_using_subprocess more flexible to accept some conditions, but it won't be done in this PR .
Let me know your opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge now. Will address if there is further comment (which should be in a follow-up PR anyway)
What does this PR do?
Reduce timing on CircleCI.
Use
@slowfor some IntegrationTests.Note that, for IntegrationTest class that have
setupClass, even if a test method is fast, thesetupClassmight still take quite long time, if it downloads models. Even tokenizers may take 3 -5 seconds sometimes