-
Notifications
You must be signed in to change notification settings - Fork 301
cuda.core: Cythonize GraphBuilder and Graph with handle-layer cleanup #2008
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
base: main
Are you sure you want to change the base?
Changes from all commits
0b63c00
035b09d
ae974af
4e71a1e
aad9f6d
343ecca
9465c76
3c6c387
7823068
564a2be
9c02485
1aec3e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| # 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 GraphExecHandle, GraphHandle, StreamHandle | ||
| from cuda.core._stream cimport Stream | ||
|
|
||
|
|
||
| cdef class GraphBuilder: | ||
| cdef: | ||
| GraphHandle _h_graph | ||
| StreamHandle _h_stream | ||
| int _kind | ||
| int _state | ||
| Stream _stream # cached to avoid reconstruction from _h_stream handle | ||
| object __weakref__ | ||
|
|
||
|
|
||
| cdef class Graph: | ||
| cdef: | ||
| GraphExecHandle _h_graph_exec | ||
| object __weakref__ | ||
|
|
||
| @staticmethod | ||
| cdef Graph _init(cydriver.CUgraphExec graph_exec) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,8 @@ from cuda.core._stream import Stream | |
| from cuda.core._utils.cuda_utils import driver | ||
| from cuda.core.graph._graph_definition import GraphCondition, GraphDefinition | ||
|
|
||
| _BuilderKind = int | ||
| _CaptureState = int | ||
|
|
||
| @dataclass | ||
| class GraphDebugPrintOptions: | ||
|
|
@@ -106,23 +108,19 @@ class GraphBuilder: | |
|
|
||
| """ | ||
|
|
||
| class _MembersNeededForFinalize: | ||
| __slots__ = ('conditional_graph', 'graph', 'is_join_required', 'is_stream_owner', 'stream') | ||
|
|
||
| def __init__(self, graph_builder_obj: GraphBuilder, stream_obj: Stream | None, is_stream_owner: bool, conditional_graph, is_join_required: bool) -> None: | ||
| ... | ||
|
|
||
| def close(self) -> None: | ||
| ... | ||
| __slots__ = ('__weakref__', '_building_ended', '_mnff') | ||
| def __init__(self): | ||
| ... | ||
|
|
||
| def __init__(self) -> None: | ||
| def __dealloc__(self): | ||
| ... | ||
|
|
||
| @classmethod | ||
| def _init(cls, stream: Stream | None, is_stream_owner: bool, conditional_graph: object=None, is_join_required: bool=False) -> GraphBuilder: | ||
| @staticmethod | ||
| def _init(stream: Stream): | ||
| ... | ||
|
|
||
| def close(self): | ||
| """Destroy the graph builder.""" | ||
|
|
||
| @property | ||
| def stream(self) -> Stream: | ||
| """Returns the stream associated with the graph builder.""" | ||
|
|
@@ -155,7 +153,7 @@ class GraphBuilder: | |
| def end_building(self) -> GraphBuilder: | ||
| """Ends the building process.""" | ||
|
|
||
| def complete(self, options: GraphCompleteOptions | None=None) -> 'Graph': | ||
| def complete(self, options: GraphCompleteOptions | None=None) -> Graph: | ||
| """Completes the graph builder and returns the built :obj:`~graph.Graph` object. | ||
|
|
||
| Parameters | ||
|
|
@@ -245,9 +243,6 @@ class GraphBuilder: | |
| A condition variable for controlling conditional execution. | ||
| """ | ||
|
|
||
| def _cond_with_params(self, node_params: object) -> tuple[GraphBuilder, ...]: | ||
| ... | ||
|
|
||
| def if_then(self, condition: GraphCondition) -> GraphBuilder: | ||
| """Adds an if condition branch and returns a new graph builder for it. | ||
|
|
||
|
|
@@ -335,15 +330,7 @@ class GraphBuilder: | |
|
|
||
| """ | ||
|
|
||
| def close(self) -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drive-by comment to test the theory that the I think dropping
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I stand corrected, I see this just got moved a few lines above.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, good. I was worried for a moment! |
||
| """Destroy the graph builder. | ||
|
|
||
| Closes the associated stream if we own it. Borrowed stream | ||
| object will instead have their references released. | ||
|
|
||
| """ | ||
|
|
||
| def embed(self, child: GraphBuilder) -> None: | ||
| def embed(self, child: GraphBuilder): | ||
| """Embed a previously-built :obj:`~graph.GraphBuilder` as a child node. | ||
|
|
||
| Parameters | ||
|
|
@@ -392,21 +379,7 @@ class Graph: | |
|
|
||
| """ | ||
|
|
||
| class _MembersNeededForFinalize: | ||
| __slots__ = 'graph' | ||
|
|
||
| def __init__(self, graph_obj: Graph, graph: driver.CUgraphExec) -> None: | ||
| ... | ||
|
|
||
| def close(self) -> None: | ||
| ... | ||
| __slots__ = ('__weakref__', '_mnff') | ||
|
|
||
| def __init__(self) -> None: | ||
| ... | ||
|
|
||
| @classmethod | ||
| def _init(cls, graph: driver.CUgraphExec) -> Graph: | ||
| def __init__(self): | ||
| ... | ||
|
|
||
| def close(self) -> None: | ||
|
|
@@ -457,5 +430,5 @@ class Graph: | |
| """ | ||
| __all__ = ['Graph', 'GraphBuilder', 'GraphCompleteOptions', 'GraphDebugPrintOptions'] | ||
|
|
||
| def _instantiate_graph(h_graph, options: GraphCompleteOptions | None=None) -> 'Graph': | ||
| def _instantiate_graph(h_graph, options: GraphCompleteOptions | None=None) -> Graph: | ||
| ... | ||
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.
Design question, not a blocker: this is the only one of the graph-family boxes that holds no extra state —
GraphBoxtracksh_parent,GraphNodeBoxtracksh_graph, butGraphExecBoxhas just the rawCUgraphExec. That's semantically correct today: aftercuGraphInstantiateWithParams, the exec is independent of its sourceCUgraph, andcuGraphExecUpdatetakes the new source at call time, so nothing needs to be held.But the PR description calls this "groundwork for step 3 of #1330 (graph updates)". If that work ends up wanting to remember which
GraphBuilder/CUgraphan exec was last updated from (for diagnostics, error messages, or to extend the source's lifetime through the exec), this Box is the natural place to grow aGraphHandle h_source_graph;. Worth either a one-line// TODO:noting that, or a confirmation that no such reference is planned and removing the asymmetry from any reviewer's mental model.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.
Agreed: no source-graph ref on
GraphExecBoxis planned. The exec is independent post-instantiate andGraph.update()supplies the source at call time. Clarification: step 3 lays the foundation for source graph updates; exec graph updates are step 5.