-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Mark CV-CUDA tests with needs_cuda #9305
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
base: main
Are you sure you want to change the base?
Conversation
…s-cuda-marks update my branch.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/9305
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit b195ef2 with merge base 96e7797 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
test/test_transforms_v2.py
Outdated
| marks=( | ||
| pytest.mark.skipif(not CVCUDA_AVAILABLE, reason="CVCUDA is not available"), | ||
| pytest.mark.needs_cuda, | ||
| ), |
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.
We'll need to use these two marks on every single CV-CUDA test, so instead of duplicating this everywhere, let's simply define a global mark on top of the file as
CV_CUDA_TEST = (
pytest.mark.skipif(not CVCUDA_AVAILABLE, reason="CVCUDA is not available"),
pytest.mark.needs_cuda,
)and then use marks=CV_CUDA_TEST for all of these.
Let's also update the previous test classes from @AntoineSimoulin:
vision/test/test_transforms_v2.py
Lines 6859 to 6861 in 96e7797
| @pytest.mark.skipif(not CVCUDA_AVAILABLE, reason="test requires CVCUDA") | |
| @needs_cuda | |
| class TestCVDUDAToTensor: |
and
vision/test/test_transforms_v2.py
Lines 6797 to 6799 in 96e7797
| @pytest.mark.skipif(not CVCUDA_AVAILABLE, reason="test requires CVCUDA") | |
| @needs_cuda | |
| class TestToCVCUDATensor: |
to use this mark as well.
I suspect we might be able to also remove the CVCUDA_AVAILABLE global variable now (not sure).
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.
Maybe we can create a @needs_cvcuda decorator (similar to @needs_cuda)? We will also have to modify the pytest_configure function to add this decorator. This way we can identify easily tests requiring cvcuda?
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 significantly reduces the boilerplate!
I have gone ahead and updated the branch implementing CV-CUDA for the ToDtype transform and I will preemptively update the others as well.
As a note, I defined CV_CUDA_TEST as a list so we can use it with the pytestmark class attribute for TestCVCUDAToTensor and TestToCVCUDATensor. Example here
test/test_transforms_v2.py
Outdated
| @pytest.mark.skipif(not CVCUDA_AVAILABLE, reason="test requires CVCUDA") | ||
| @needs_cuda | ||
| @needs_cvcuda | ||
| class TestCVDUDAToTensor: |
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.
Drive-by suggestion:
| class TestCVDUDAToTensor: | |
| class TestCVCUDAToTensor: |
|
|
||
| CVCUDA_AVAILABLE = _is_cvcuda_available() | ||
| if CVCUDA_AVAILABLE: | ||
| cvcuda = _import_cvcuda() |
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.
Some tests in TestToCVCUDATensor are failing because cvcuda is now removed.
It's good that it's removed, but we should update the parts in TestToCVCUDATensor to now use _import_cvcuda() instead
test/test_transforms_v2.py
Outdated
| CV_CUDA_TEST = ( | ||
| pytest.mark.needs_cvcuda, | ||
| pytest.mark.needs_cuda, | ||
| ) |
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.
Having the new needs_cvcuda is great, but I realize now we're in this odd situation where we really have to make all CV-CUDA tests also have the needs_cuda mark, otherwise we risk skipping them when we don't want to. That's reflected here by having to add both pytest.mark.needs_cvcuda and pytest.mark.needs_cuda, but also below when parametrizing an entire class, we have to add both marks:
@needs_cuda
@needs_cvcuda
class TestCVDUDAToTensor:I'd suggest instead to only use the needs_cvcuda mark for the cv-cuda test, and leave out the needs_cuda mark. That is:
pytest.param(
# ...,
marks=pytest.mark.needs_cuda,
)
# and:
# @needs_cuda # <-- removed!
@needs_cvcuda
class TestCVDUDAToTensor:In order to still mark the cv-cuda tests with needs_cuda, all we need to do is to add this logic in conftest.py:
needs_cvcuda = item.get_closest_marker("needs_cvcuda") is not None
if needs_cvcuda:
item.add_marker(pytest.mark.needs_cuda)
needs_cuda = item.get_closest_marker("needs_cuda") is not NoneLet me know if I can clarify anything!
NicolasHug
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.
That looks great. Thanks @zy1git !
Importantly we should make sure none of the CVCUDA tests are being skipped anymore. To check, we have to look at the logs https://github.com/pytorch/vision/actions/runs/20149139270/job/57837488003 and it's sometimes easier to check the "raw logs"
I did a manual check, things look good, but I encourage you to check on your own before merging! :)
Mark CV-CUDA tests with needs_cvcuda.