-
Notifications
You must be signed in to change notification settings - Fork 217
Improve #789: Remove cyclical dependency between cuda.bindings.{driver|runtime} and c.b.utils #840
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
Conversation
…e} and utils Rather than having bindings.utils._get_handle.pyx depend on driver and runtime and define the getters there, this flips things so driver and runtime register their own handlers.
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
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.
Pull Request Overview
This PR refactors the CUDA handle getter architecture to eliminate cyclical dependencies between the cuda.bindings.{driver|runtime}
and cuda.bindings.utils
modules. Instead of having _get_handle.pyx
depend on driver and runtime modules and centrally define all getters, the change flips the dependency so that driver and runtime modules register their own handle getters with the utils module.
- Removes the centralized
_get_handle.pyx.in
file that created cyclical imports - Moves handle getter registration to individual driver and runtime modules
- Introduces a registration mechanism in utils module for type-specific handle getters
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
cuda_bindings/cuda/bindings/utils/_get_handle.pyx.in |
Entire file removed - eliminated centralized handle getter definitions |
cuda_bindings/cuda/bindings/utils/__init__.py |
Added registration mechanism and moved get_cuda_native_handle function here |
cuda_bindings/cuda/bindings/runtime.pyx.in |
Added handle getter registration function and call to register runtime types |
cuda_bindings/cuda/bindings/nvrtc.pyx.in |
Added utils import (minimal change for dependency alignment) |
cuda_bindings/cuda/bindings/driver.pyx.in |
Added handle getter registration function and call to register driver types |
Comments suppressed due to low confidence (2)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
/ok to test |
from libcpp.vector cimport vector | ||
from cpython.buffer cimport PyObject_CheckBuffer, PyObject_GetBuffer, PyBuffer_Release, PyBUF_SIMPLE, PyBUF_ANY_CONTIGUOUS | ||
from cpython.bytes cimport PyBytes_FromStringAndSize | ||
from cuda.bindings import utils |
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.
it seems this is unused and can be purged
from cuda.bindings import utils |
This comment has been minimized.
This comment has been minimized.
/ok to test |
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, Mike! LGTM!
/ok to test |
/ok to test 15eeb07 |
@leofang: As far as workflow goes, should I merge this now or wait until the cython-gen changes that generated them are approved also? |
We often merge here before merging the cython-gen changes. I'd say go ahead. It's ideal to merge the cython-gen changes shortly before or after. I think you can even merge them without approval, although I generally feel most comfortable if I get an explicit approval there, too. |
Thanks, Mike! Getting approval is always good. I did not merge last week because I wanted to push out the patch release first, and we can't keep asking Peter to re-spin the test every time we update the binding code. We should keep the changes minimal. After we pushed it out this morning, we are un-freezed and resume merging. Keep in mind that we haven't backported #835 to the codegens, so right now after regenerating the output will deviate from what's in the public. I am very occupied today and likely tomorrow due to the remaining CuPy release works, I hope we can get it done soon. |
|
Rather than having bindings.utils._get_handle.pyx depend on driver and runtime and define the getters there, this flips things so driver and runtime register their own handlers.
While this doesn't resolve all cyclical imports as required by #789, this was a fairly straightforward change that improves import times by about 10%:
(This was measured using this pyperf benchmark, starting with a fresh Python process each time.)
This is generated based on changes to cython_gen submitted in the private repo: https://github.com/NVIDIA/cuda-python-private/pull/131