-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Better logic for ignoring CPU tests on GPU machines #4025
Better logic for ignoring CPU tests on GPU machines #4025
Conversation
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.
Looks good @NicolasHug, just a couple of nits for your consideration. Let me know what you think.
…vision into even_better_needs_cuda_logic
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 looks awesome, thanks a lot Nicolas!
Thanks both for the reviews! |
Reviewed By: fmassa Differential Revision: D29105975 fbshipit-source-id: 0f3446a61934e6b5ee3151c390e604e5b858d355
…rch#4025) Reviewed By: fmassa Differential Revision: D29105975 fbshipit-source-id: 0f3446a61934e6b5ee3151c390e604e5b858d355
This PR allows all CPU-only tests to be skipped on the CircleCI GPU machines, and does something similar for the internal fbcode CI.
unittest_linux_gpu
now runs in about 475 seconds, which is a 425 seconds (= 7 minutes) improvement over the 900 seconds of some previous runs, e.g. this one from 2 days ago. We should expect the same improvement on the Windows GPU workflows but they're currently toasted #4006It also greatly simplifies the logic that we previously had (Thanks @fmassa for our discussion this morning!):
@cpu_only
that we don't need anymorepytest_collection_modifyitems
hook instead of being distributed amongcpu_only
,needs_cuda
andcpu_and_gpu()
which was confusing.needs_cuda
andcpu_and_gpu()
have been greatly simplified to one-linersThis seems to be working as expected:
unittest_linux_gpu
https://app.circleci.com/pipelines/github/pytorch/vision/8875/workflows/bd0fce58-8045-4d57-a6e1-fb7a691451a0/jobs/645558:cpu_and_gpu()
are skipped, and their cuda siblings are properly run. Look e.g. fortest_segmentation_model
.test_detection_model_validation
needs_cuda
decorator are run, as e.gtest_fasterrcnn_switch_devices
Some more context:
Up to yesterday, the CircleCI GPU machines were running all tests, including the CPU-only tests. This was a waste of resource as the CPU-only tests are already extensively tested on the CircleCI CPU workflows.
Up to this morning (with #4002 merged), the CircleCI GPU machines were running all tests, except the CPU tests that were explicitly decorated with
@cpu_only
. Only a few of them were using this decorator, so we weren't skipping as many tests as we could.Now, we don't need
@cpu_only
at all, and CircleCI GPU workflows will only run:@needs_cuda
cpu_and_gpu()
All the rest will be automatically skipped.
The same will happen for the internal Fbcode CI.