-
Notifications
You must be signed in to change notification settings - Fork 277
Enhance Graph.update() and add whole-graph update tests #1843
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
Merged
Merged
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
1633628
Reorganize graph test files for clarity
Andy-Jost d3c4ef4
Enhance Graph.update() and add whole-graph update tests
Andy-Jost 51cfcb0
Fix GraphDef test race condition; correct handle property annotations
Andy-Jost 72821c7
Split _graphdef.pyx into _graph_def/ subpackage for maintainability
Andy-Jost 17715f2
Merge remote-tracking branch 'origin/main' into graph-updates
Andy-Jost 60964fd
Fix stale _graphdef import paths after subpackage rename
Andy-Jost e924fde
Assert specific error code from cuGraphExecUpdate
Andy-Jost fe04a91
Handle all cuGraphExecUpdate error codes, not just update failure
Andy-Jost File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| from cuda.core._graph._graph_def._graph_def cimport Condition, GraphDef | ||
| from cuda.core._graph._graph_def._graph_node cimport GraphNode | ||
| from cuda.core._graph._graph_def._subclasses cimport ( | ||
| AllocNode, | ||
| ChildGraphNode, | ||
| ConditionalNode, | ||
| EmptyNode, | ||
| EventRecordNode, | ||
| EventWaitNode, | ||
| FreeNode, | ||
| HostCallbackNode, | ||
| IfElseNode, | ||
| IfNode, | ||
| KernelNode, | ||
| MemcpyNode, | ||
| MemsetNode, | ||
| SwitchNode, | ||
| WhileNode, | ||
| ) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Explicit CUDA graph construction — GraphDef, GraphNode, and node subclasses.""" | ||
|
|
||
| from cuda.core._graph._graph_def._graph_def import ( | ||
| Condition, | ||
| GraphAllocOptions, | ||
| GraphDef, | ||
| ) | ||
| from cuda.core._graph._graph_def._graph_node import GraphNode | ||
| from cuda.core._graph._graph_def._subclasses import ( | ||
| AllocNode, | ||
| ChildGraphNode, | ||
| ConditionalNode, | ||
| EmptyNode, | ||
| EventRecordNode, | ||
| EventWaitNode, | ||
| FreeNode, | ||
| HostCallbackNode, | ||
| IfElseNode, | ||
| IfNode, | ||
| KernelNode, | ||
| MemcpyNode, | ||
| MemsetNode, | ||
| SwitchNode, | ||
| WhileNode, | ||
| ) | ||
|
|
||
| __all__ = [ | ||
| "AllocNode", | ||
| "ChildGraphNode", | ||
| "Condition", | ||
| "ConditionalNode", | ||
| "EmptyNode", | ||
| "EventRecordNode", | ||
| "EventWaitNode", | ||
| "FreeNode", | ||
| "GraphAllocOptions", | ||
| "GraphDef", | ||
| "GraphNode", | ||
| "HostCallbackNode", | ||
| "IfElseNode", | ||
| "IfNode", | ||
| "KernelNode", | ||
| "MemcpyNode", | ||
| "MemsetNode", | ||
| "SwitchNode", | ||
| "WhileNode", | ||
| ] |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| from cuda.bindings cimport cydriver | ||
| from cuda.core._resource_handles cimport GraphHandle | ||
|
|
||
|
|
||
| cdef class Condition: | ||
| cdef: | ||
| cydriver.CUgraphConditionalHandle _c_handle | ||
| object __weakref__ | ||
|
|
||
|
|
||
| cdef class GraphDef: | ||
| cdef: | ||
| GraphHandle _h_graph | ||
| object __weakref__ | ||
|
|
||
| @staticmethod | ||
| cdef GraphDef _from_handle(GraphHandle h_graph) |
Oops, something went wrong.
Oops, something went wrong.
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.
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.
I used Cursor GPT-5.4 1M High to "comb" through this "very complex and very large" PR. It only found this one "High" item:
I think this would be a bit safer if it distinguished the graph-update failure case from ordinary driver errors, e.g.
Rationale:
cydriver.cuGraphExecUpdate(...)directly here makes sense, since the higher-level binding dropsresultInfoon non-success and would lose the detailed update reason entirely.resultInfoappears to be the structured explanation for the specificCUDA_ERROR_GRAPH_EXEC_UPDATE_FAILUREpath, not necessarily for every possible non-successCUresult.result_info.result == CU_GRAPH_EXEC_UPDATE_ERROR, the enum docs say the actual explanation is described by the function return value. The current code discardserr, so it may collapse distinct driver failures into the same genericresultInfo-based message.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.
According to the docs,
cuGraphExecUpdateonly returnsCUDA_SUCCESSorCUDA_ERROR_GRAPH_EXEC_UPDATE_FAILURE.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.
To be clear, nearly all of this change is refactoring and code movement. The graph tests were regrouped slightly and renamed. The huge
_graphdefmodule was split into three parts. The are not many functional changes 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.
Documentation tends to be imprecise, or become imprecise over time without anyone noticing.
The suggested change improves the quality of implementation at a very small cost.
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.
I checked the driver code and the docs are indeed incorrect.