Repository navigation
Ship _v2 lowpp API for the driver module - #3095
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
github-actions
Bot
added
cuda.bindings
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test cd9a1d8 |
|
/ok to test 027a11e |
|
|
/ok to test |
|
/ok to test |
|
/ok to test 650a8ab |
|
/ok to test 0c7b2a7 |
|
/ok to test |
|
/ok to test dd186df |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ⚪ Pending latest changes Change: Feature Merge Risk: 🟡 Moderate · up to
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments: 1
cuda_bindings_13/tests/test_cuda.py: 1
500-518: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winsuggestion: Destroy
graphExecbefore cleanup.
graphExecremains live until module context teardown. No later graph-memory assertion uses this context, so the measurable cross-test impact is not established.+ driver.graph_exec_destroy(graphExec) driver.graph_destroy(graph) driver.stream_destroy_v2(stream)
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
08008359-6944-48d9-bf2e-acce3fd6e63d
📒 Files selected for processing: 32
.pre-commit-config.yamlcuda_bindings_13/AGENTS.mdcuda_bindings_13/cuda/bindings/_v2/driver.pxdcuda_bindings_13/cuda/bindings/_v2/driver.pyxcuda_bindings_13/cuda/bindings/_v2/nvrtc.pxdcuda_bindings_13/cuda/bindings/_v2/nvrtc.pyxcuda_bindings_13/cuda/bindings/cudla.pyxcuda_bindings_13/cuda/bindings/cufile.pyxcuda_bindings_13/cuda/bindings/driver.pyxcuda_bindings_13/cuda/bindings/nvfatbin.pyxcuda_bindings_13/cuda/bindings/nvjitlink.pyxcuda_bindings_13/cuda/bindings/nvml.pyxcuda_bindings_13/cuda/bindings/nvvm.pyxcuda_bindings_13/docs/source/api.rstcuda_bindings_13/docs/source/module/driver_v2.rstcuda_bindings_13/docs/source/module/nvrtc_v2.rstcuda_bindings_13/docs/source/module/v2_migration.rstcuda_bindings_13/docs/source/release/13.5.0-notes.rstcuda_bindings_13/tests/conftest.pycuda_bindings_13/tests/legacy_api/conftest.pycuda_bindings_13/tests/legacy_api/nvml/test_legacy_nvml_cuda.pycuda_bindings_13/tests/legacy_api/test_legacy_cuda.pycuda_bindings_13/tests/legacy_api/test_legacy_cufile.pycuda_bindings_13/tests/legacy_api/test_legacy_interoperability.pycuda_bindings_13/tests/nvml/test_cuda.pycuda_bindings_13/tests/test_cuda.pycuda_bindings_13/tests/test_cufile.pycuda_bindings_13/tests/test_interoperability.pycuda_bindings_13/tests/test_kernelParams.pycuda_bindings_13/tests/test_nvrtc.pycuda_core/cuda/core/system/exceptions.pycuda_core/docs/source/release/1.3.0-notes.rst
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
|
Migration guide: are the suffixed legacy driver functions intentionally omitted? The guide says the legacy module "exposes only the unsuffixed name" for versioned C symbols. But 8 suffixed legacy functions exist in This can mislead: Deliberate, or should the tables add rows for the suffixed names? |
|
Naming nit on
|
juenglin
left a comment
juenglin
left a comment
There was a problem hiding this comment.
(Posting this first point in the review body: GitHub won't attach an inline comment to the 43k-line generated cuda_bindings_13/cuda/bindings/_v2/driver.pyx.)
_v2/driver.pyx TensorMap.opaque (~L10281) and the other view.array getters — use-after-free.
These getters do arr = cython.view.array(..., allocate_buffer=False); arr.data = &self._ptr[0].<member>; return _numpy.asarray(arr). The returned array's base is the cython array holding the raw pointer, with no strong reference back to self. Once the wrapper is garbage-collected the backing memory is freed while the view lives on (for TensorMap the struct is inline in the Python object).
Repro (no GPU needed):
from cuda.bindings._v2 import driver
import gc
views = []
for i in range(500):
tm = driver.TensorMap()
tm.opaque = [i] * 16
views.append(tm.opaque) # keep the view, drop the wrapper
del tm
gc.collect()
flood = [driver.TensorMap() for _ in range(500)] # reuse the freed 128B slots
for t in flood:
t.opaque = [9999] * 16
print(sum(int(v[0]) == 9999 for v in views)) # -> 500: every view reads reused memorySame pattern in .address_mode, .border_color, .max_threads_dim, .max_grid_size, and the two data_ getters. The recarray-backed structs already tie lifetime via owner=obj in from_ptr; these should too. A "view outlives wrapper" regression test would help — the existing test_array_member_assignment_and_aliased_dealloc only covers the raw-address+count members.
| - | ||
| * - ``nvrtcInstallBundledHeaders`` | ||
| - ``install_bundled_headers`` | ||
| - ``flags`` takes ``InstallHeadersFlag`` values. Returns the optional error message (``bytes`` or ``None``). |
There was a problem hiding this comment.
Doc vs. code mismatch. These rows say the bundled-header helpers return None on success, but the implementation returns b"" and the tests assert it: _v2/nvrtc.pyx:1054, :1081, :1115 and tests/test_nvrtc.py:43, :52, :54 (same for get_lowered_name at nvrtc.pyx:881). Please reconcile doc and code — a caller following the docs and writing if err is None would be wrong.
There was a problem hiding this comment.
Good catch. Updated the docs to match the behavior in bdeddb0
| InsufficientSizeError = nvml.InsufficientSizeError | ||
| InsufficientPowerError = nvml.InsufficientPowerError | ||
| DriverNotLoadedError = nvml.DriverNotLoadedError | ||
| NvmlTimeoutError = nvml.TimeoutError |
There was a problem hiding this comment.
This rename doesn't actually stop the builtin shadowing. Here (and in the matching nvml.pyx) the old names are kept as aliases: cuda.bindings.nvml still defines TimeoutError = NvmlTimeoutError / MemoryError = NvmlMemoryError and has no __all__, and cuda.core.system re-exports both old names (this line plus MemoryError below, both added to __all__). So from ... import * still rebinds the builtin TimeoutError/MemoryError, which is the thing the release note says the rename fixes.
Broader question: the NVML rename and the regeneration of nvml/cufile/cudla/nvvm/nvjitlink/nvfatbin plus the legacy driver seem unrelated to the _v2 driver API work. Is bundling them here intentional, or would it be cleaner as a separate PR? They carry user-visible changes not called out in the description.
There was a problem hiding this comment.
Unfortunately, we can't remove the old names without breaking API. Nor can we actually create deprecation warnings about their use (since they are used in a try/except and not actually called as functions). So the best we can do is add the new names as aliases now and declare them deprecated in the CHANGELOG, so we can remove the old names in the 14.x release.
An additional wrinkle is that cuda-core has to work with the latest /released/ version of cuda-bindings 13.4.x (not only the version here on main), so it has to continue to use the old names until 13.5.x is released and we can bump cuda-core's version floor on cuda-bindings.
Broader question: the NVML rename and the regeneration of nvml/cufile/cudla/nvvm/nvjitlink/nvfatbin plus the legacy driver seem unrelated to the _v2 driver API work. Is bundling them here intentional, or would it be cleaner as a separate PR? They carry user-visible changes not called out in the description.
These are due to bugs that were found during the course of the development of _v2.driver that also apply to these existing modules. (Bugs that only are found because the test coverage on driver is so much larger than any of the other modules). It would add complexity to the generator to generate in different ways for different modules, so we should be consistent.
However, it is a good callout that these changes (which are all backward-compatible) are not called out in the release notes. I have added them in e82804d.
| (``cuStreamCreate`` becomes ``stream_create``). Driver functions keep the version suffix of | ||
| the C symbol (``mem_alloc_v2``), whereas the legacy module drops it. | ||
| - Parameters and struct members use ``snake_case``. Names that would clash with a Python keyword or | ||
| builtin, or with a name reserved by the generated classes, get a trailing underscore (``input_``, ``flags_``). |
There was a problem hiding this comment.
Rule doesn't match the generated names. This says names clashing with a Python builtin get a trailing underscore (input_, flags_), but several _v2.driver parameters keep builtin names: type (x11), object (x4), len (x2), bytes (x2), input (x2), format, map (input_ doesn't actually appear in driver.pyx). Since renaming later would be breaking, worth settling before the API firms up — adjust either the rule or the generator.
There was a problem hiding this comment.
Yes, the note is wrong -- it's not about shadowing Python builtins, it's about avoiding shadowing Numpy methods that would shadow a field name. It turns out we don't /actually/ need to do this -- it's a bug -- but fixing it would breaking existing API, so it needs to be fixed by adding aliases to the old name, deprecating and then removing. For that reason, I think we should address it separately, and I will make an issue for the generator for that.
isVoid
left a comment
isVoid
left a comment
There was a problem hiding this comment.
My agent didn't find anything outside of the generated files region. I do have a small trivia below.
| Enumerations | ||
| ------------ | ||
|
|
||
| Enumeration classes are ``int`` subclasses. Functions accept either the enumeration members or plain integers. | ||
| Their members expose a description of the value automatically, so the individual members | ||
| are no longer documented one by one. |
There was a problem hiding this comment.
I ran into similar agent design decision when doing rust bindings. Semantically C enums are closer to plain integers instead of python/rust enum. I find that interesting how different language bindings landed at the same design.
No, that's just an omission in the migration guide. Fixed in 1e81c7e.
This was a good catch. My agent did this because it
This is describing an existing bug in all of the other cybind modules that has existed for a long time. It's a real issue, but may require some care and complexity to fix in an API-backward-compatible way. Since no one has actually reported the bug as an issue, I think this is lower priority and shouldn't hold up merging this PR. @juenglin already filed #3193 for this, so I think we just work on that separately. |
No description provided.