Skip to content
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

Merged
merged 12 commits into from
Jun 14, 2021

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Jun 9, 2021

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 #4006

It also greatly simplifies the logic that we previously had (Thanks @fmassa for our discussion this morning!):

  • I removed @cpu_only that we don't need anymore
  • the entire logic is now centralized within the pytest_collection_modifyitems hook instead of being distributed among cpu_only, needs_cuda and cpu_and_gpu() which was confusing.
  • needs_cuda and cpu_and_gpu() have been greatly simplified to one-liners

This seems to be working as expected:


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:

  • tests that are either explicitly decorated with @needs_cuda
  • the CUDA parts of the tests that use cpu_and_gpu()

All the rest will be automatically skipped.

The same will happen for the internal Fbcode CI.

@NicolasHug NicolasHug marked this pull request as ready for review June 9, 2021 14:45
@NicolasHug NicolasHug changed the title NORMG Even better logic for ignoring CPU tests on GPU machines (etc.) Better logic for ignoring CPU tests on GPU machines Jun 9, 2021
Copy link
Contributor

@datumbox datumbox left a 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.

test/test_image.py Show resolved Hide resolved
test/test_image.py Show resolved Hide resolved
test/conftest.py Outdated Show resolved Hide resolved
Copy link
Member

@fmassa fmassa left a 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!

@NicolasHug NicolasHug merged commit 508b289 into pytorch:master Jun 14, 2021
@NicolasHug
Copy link
Member Author

Thanks both for the reviews!

facebook-github-bot pushed a commit that referenced this pull request Jun 15, 2021
Reviewed By: fmassa

Differential Revision: D29105975

fbshipit-source-id: 0f3446a61934e6b5ee3151c390e604e5b858d355
NicolasHug added a commit to NicolasHug/vision that referenced this pull request Jun 15, 2021
…rch#4025)

Reviewed By: fmassa

Differential Revision: D29105975

fbshipit-source-id: 0f3446a61934e6b5ee3151c390e604e5b858d355
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants