You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Reviewing files that changed from the base of the PR and between 45c9a18 and e4f844e.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
Summary
Summary by CodeRabbit
New Features
Added read-only IDs for graph definitions and executable graphs, along with graph-local and tooling IDs for graph nodes. These properties require a CUDA 13 build and a CUDA 13.1 or newer driver.
Added a kernel attribute indicating whether device-node updates are supported. It requires a CUDA 13 build and a CUDA 13.3 or newer driver.
Documentation
Updated the API reference and release notes with the new properties, version requirements, and graph-node navigation details.
The change adds graph and node identity properties and a kernel attribute for device-node-update support. The properties include CUDA build and driver-version checks. Tests and documentation cover the new APIs.
Changes: Graph identity and kernel capability APIs
Graph wrappers add id; graph nodes add local_id and tools_id. Access checks include build, driver-version, and object-validity checks. A test covers graph and node identity properties. The API reference and release notes document the properties.
KernelAttributes adds device_node_update_supported. Builds before CUDA 13 and drivers older than 13.3 raise NotImplementedError. A test checks the property across supported and unsupported configurations. The API reference documents the property and version requirement.
Assessment against linked issues
Objective
Addressed
Explanation
[#2360] Expose graph and executable graph IDs, node local and tooling IDs, and node-to-graph navigation
❓
The graph and node ID properties are implemented. The summaries mention node.graph in documentation but do not show an implementation change for node-to-graph navigation.
[#2360] Expose the device-node-update kernel attribute
✅
Possibly related issues
NVIDIA/cuda-python#2663 — The kernel capability property supports capability checks, but this change does not add the requested LaunchConfig setter.
Suggested reviewers:mdboom
Change: Feature
Merge Risk: Low · previous review 45c9a
The new properties require CUDA 13 builds, but some errors and documentation do not clearly state that requirement. These are bounded issues suitable for follow-up.
- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Tip
🎯 Customize your risk assessments
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
important: Assert unsupported node IDs on older drivers.
For a CUDA 13 build with a driver below 13.1, node.local_id and node.tools_id raise NotImplementedError, but this branch checks only definition.id and graph.id.
Suggested test fix
if driver_version() < (13, 1, 0):
with pytest.raises(NotImplementedError):
_ = definition.id
with pytest.raises(NotImplementedError):
_ = graph.id
+ with pytest.raises(NotImplementedError):+ _ = node.local_id+ with pytest.raises(NotImplementedError):+ _ = node.tools_id
return
Add _BINDINGS_MAJOR < 13 to this condition after the ID properties receive a CUDA 12 build guard. At the current head, the CUDA 12 build cannot compile the direct ID API references.
The reason will be displayed to describe this comment to others. Learn more.
Minor · Functional Correctness · Quick win
important: Clarify which kernel the capability describes.
The driver attribute reports whether this kernel can be the target of a device-side update. The current wording can also mean that this kernel can issue updates to other nodes. Clarify the target meaning here and in the API documentation. (docs.nvidia.com)
On a CUDA 12 build, the graph identity properties raise NotImplementedError even with a 13.1+ driver. Replace “CUDA 13.1+” with “CUDA 13 build and driver 13.1+” so the release note describes the available configuration. As per coding guidelines, “Keep user-facing behaviors coherent with docs and samples.” (raw.githubusercontent.com)
The reason will be displayed to describe this comment to others. Learn more.
Major · Functional Correctness · Quick win
important: Include the build major in the graph identity test gate.
On a CUDA 12 build with a 13.1+ driver, GraphDefinition.id raises NotImplementedError. test_graph_identity_attributes checks only driver_version() and instead asserts that the property returns an int. That supported test run fails. Update the test to check BUILD_CUDA_MAJOR as well. (raw.githubusercontent.com)
The reason will be displayed to describe this comment to others. Learn more.
nit: it would be better IMO to use the same message in either case when the feature is unavailable (5 instances)
IF CUDA_CORE_BUILD_MAJOR >= 13:
cdef unsigned int graph_id
if cy_driver_version() >= (13, 1, 0):
with nogil:
HANDLE_RETURN(cydriver.cuGraphExecGetId(as_cu(self._h_graph_exec), &graph_id))
return graph_id
raise NotImplementedError("Graph IDs require CUDA driver 13.1 or newer")
The reason will be displayed to describe this comment to others. Learn more.
Reading through the bot messages above, it seems that there is a difference here between the SDK and driver version: cuda12 builds can still get the values if the driver is new enough. If we trust the bot... I am happy to keep the blanket guards as they are (with the language fix you suggest), but thought you might want to consider this.
The reason will be displayed to describe this comment to others. Learn more.
Are the driver functions defined in CUDA 12? I don't see them listed in the CUDA 12.9 Documentation or cuda-bindings. Without the entry point, a newer driver won't help.
The reason will be displayed to describe this comment to others. Learn more.
Minor · Functional Correctness · Quick win
suggestion: State the CUDA 13 build requirement for these properties. On a CUDA 12 build, the ID properties raise NotImplementedError even with a 13.1-or-newer driver. device_node_update_supported also raises on that build even with a 13.3-or-newer driver. State the build requirement in both paragraphs and in the Graph.id declaration.
As per coding guidelines, “Keep user-facing behaviors coherent with docs and samples.” As per path instructions, “Check that public behavior changes have corresponding tests, documentation and release notes.”
Thanks @martindurant. I declined two of CodeRabbit's threads (the cuFuncGetAttribute detour and the CUDA 12 compatibility enum). These items remain:
Test gate. On a CUDA 12 build with a 13.1+ driver, every ID property raises NotImplementedError, so test_graph_identity_attributes fails. Gate on the build major as well, as test_green_context.py does:
Unsupported-path coverage. In that branch, also assert that node.local_id and node.tools_id raise NotImplementedError.
device_node_update_supported has no test. Add one that checks for a bool on a CUDA 13 build with a 13.3+ driver, and checks for NotImplementedError otherwise.
One error message. As in my earlier nit, use a single fall-through raise in each of the five properties. Word the message so it holds for both cases, for example "Graph IDs require a CUDA 13 build of cuda.core and CUDA driver 13.1 or newer". The current text is wrong on a CUDA 12 build with a new driver.
Docs. State "CUDA 13 build and driver 13.1+" (13.3+ for device_node_update_supported) in api.rst, the 1.3.0 release note, and the Graph.id docstring in both .pyx and .pyi. GraphNode.local_id and GraphNode.tools_id state no requirement at all.
Wording. In the device_node_update_supported docstring, say that the kernel can be the target of a device-side node update. The current text also reads as if the kernel issues updates to other nodes.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
Minor · 🎯 Functional Correctness · _graph_builder.pyx:1267-1276
cuda_core/cuda/core/graph/_graph_builder.pyx:1267-1276 Minor · Functional Correctness · Quick win
important: State both requirements in the Graph.id error.
When an open Graph is used on a CUDA 12 build with a CUDA 13.1-or-newer driver, the ELSE branch raises a message that names only the driver requirement. State that graph IDs require a CUDA 13 build of cuda.core and a CUDA 13.1-or-newer driver. GraphDefinition.id has the same incomplete message and needs the equivalent correction.
Suggested fix
- raise NotImplementedError("Graph IDs require CUDA driver 13.1 or newer")+ raise NotImplementedError("Graph IDs require a CUDA 13 build of cuda.core and CUDA driver 13.1 or newer")
Reviewing files that changed from the base of the PR and between 2439fd9 and 45c9a18.
Files selected for processing: 9
cuda_core/cuda/core/_module.pyi
cuda_core/cuda/core/_module.pyx
cuda_core/cuda/core/graph/_graph_definition.pyi
cuda_core/cuda/core/graph/_graph_definition.pyx
cuda_core/cuda/core/graph/_graph_node.pyi
cuda_core/cuda/core/graph/_graph_node.pyx
cuda_core/docs/source/release/1.3.0-notes.rst
cuda_core/tests/graph/test_graph_definition.py
cuda_core/tests/test_module.py
Files skipped from review as they are similar to previous changes: 1
cuda_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.
The reason will be displayed to describe this comment to others. Learn more.
Minor · Functional Correctness · Quick win
important: Include the CUDA 13 build requirement in the error.
On a CUDA 12 build with a 13.3 or newer driver, this error says a newer driver is required even though the driver meets that requirement. State that the property also requires a CUDA 13 build.
The reason will be displayed to describe this comment to others. Learn more.
Minor · Functional Correctness · Quick win
important: State the build requirement in the unsupported-build error.
On a CUDA 12 build, GraphDefinition.id raises an error that names only driver 13.1. A user with that driver still cannot use the property. Name both the CUDA 13 build and driver 13.1 requirements, as the property docstring does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cuda.coreEverything related to the cuda.core modulefeatureNew feature or request
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
closes #2360
Added read-only graph/node IDs and a kernel device-update capability flag, with CUDA version guards. The IDs become python ints.
Checklist