-
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. ❌ 1 New FailureAs of commit b67dfc4 with merge base 96e7797 ( NEW FAILURE - The following job has failed:
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
| @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
| 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!
Mark CV-CUDA tests with needs_cuda by:
marks=( pytest.mark.skipif(not CVCUDA_AVAILABLE, reason="CVCUDA is not available"), pytest.mark.needs_cuda, ),