-
Notifications
You must be signed in to change notification settings - Fork 74
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
Add some basic test coverage for cuda.core.experimental #153
Conversation
/ok to test |
Let's also hook up the code samples with the test system. We can do something like this: Collect the test files in-process, and loop over them. |
@ksimpson-work heads up, we've done the move to the |
1d5f425
to
fc6d176
Compare
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.
With this review I'm done looking at all the files. I'm sorry for all the nits, and thank you for handling the ones I've already shared! 🙏
Let's get the formatting feedbacks addressed quickly and move on. I've opened #195 to track an automated solution so that we don't need to spend time on reviewing formatting issues, which could sometimes be distracting/frustrating 🙂 |
Thankyou @vzhurba01 for your review. I have addressed all the comments and implemented all your nits as I agreed with all. Thanks @leofang for your review as well. I believe all comments have been addressed as of the most recent push |
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.
LGTM
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.
Thanks, Keenan/Vlad, for all the hard work! Unless I am missing something I don't think we have "relational tests" added yet (ex: Device.create_stream()
returns a Stream
; Stream.record()
returns an Event
, etc), but let's add them in the next PR.
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.
@ksimpson-work do you see these warnings locally? This is concerning and making me wonder if we need to fix #141 as part of beta 1:
============================================================== warnings summary ===============================================================
<frozen importlib._bootstrap_external>:1297
<frozen importlib._bootstrap_external>:1297: DeprecationWarning: The cuda.cuda module is deprecated and will be removed in a future release, please switch to use the cuda.bindings.driver module instead.
<frozen importlib._bootstrap_external>:1297
<frozen importlib._bootstrap_external>:1297: DeprecationWarning: The cuda.cudart module is deprecated and will be removed in a future release, please switch to use the cuda.bindings.runtime module instead.
<frozen importlib._bootstrap_external>:1297
<frozen importlib._bootstrap_external>:1297: DeprecationWarning: The cuda.nvrtc module is deprecated and will be removed in a future release, please switch to use the cuda.bindings.nvrtc module instead.
tests/test_memory.py::test_buffer_initialization
tests/test_memory.py::test_buffer_copy_to
tests/test_memory.py::test_buffer_copy_from
/local/home/leof/miniforge3/envs/cuda_py_cuda126/lib/python3.12/site-packages/_pytest/unraisableexception.py:85: PytestUnraisableExceptionWarning: Exception ignored in: <function Buffer.__del__ at 0x7f3fb7ce1ee0>
Traceback (most recent call last):
File "/local/home/leof/dev/cuda-python/cuda_core/cuda/core/experimental/_memory.py", line 35, in __del__
self.close(default_stream())
File "/local/home/leof/dev/cuda-python/cuda_core/cuda/core/experimental/_memory.py", line 41, in close
self._mr.deallocate(self._ptr, self._size, stream)
File "/local/home/leof/dev/cuda-python/cuda_core/tests/test_memory.py", line 96, in deallocate
handle_return(cuda.cuMemFreeHost(ptr))
File "/local/home/leof/dev/cuda-python/cuda_core/cuda/core/experimental/_utils.py", line 57, in handle_return
_check_error(result[0], handle=handle)
File "/local/home/leof/dev/cuda-python/cuda_core/cuda/core/experimental/_utils.py", line 29, in _check_error
raise CUDAError(f"{name.decode()}: {desc.decode()}")
cuda.core.experimental._utils.CUDAError: CUDA_ERROR_INVALID_VALUE: invalid argument
warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
...
Could you check if explicitly calling Buffer.close()
by the end of each test would make it go away? If so we should add it for now and circle back right after beta 1 (#141 needs a long-term solution).
Confirmed locally that this is the way to go. |
Tracked in #196. @vzhurba01 @ksimpson-work merge? |
@ksimpson-work feel free to merge! |
Close #78.
This change adds a minimal testsuite to the repo so we can establish pipeline, and develop the tests alongside the cuda.core object model. It uses a pytest tree structure.
Example: