cuda.core: add graph definition node updates - #2395
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. |
|
/ok to test |
|
4ce2862 to
fffa9fd
Compare
mdboom
left a comment
There was a problem hiding this comment.
The general organization and approach looks fine to me.
One small suggestion (that I can't really evaluate because I don't deeply understand the use cases).
6bafe1a to
84bcfab
Compare
|
/ok to test |
84bcfab to
f500f26
Compare
|
/ok to test f500f26 |
|
/ok to test |
| new_graph = case.graph_def.instantiate() | ||
| case.assert_exec_uses(old_graph, case.original) | ||
| case.assert_exec_uses(new_graph, case.replacement) | ||
|
|
There was a problem hiding this comment.
| assert case.original != case.replacement | |
| assert old_graph != new_graph |
| case = definition_update_case | ||
|
|
||
| assert case.invalid_update is not None | ||
| assert case.invalid_exception is not None | ||
| with pytest.raises(case.invalid_exception): | ||
| case.invalid_update() | ||
|
|
||
| case.assert_current(case.original) | ||
| graph = case.graph_def.instantiate() | ||
| case.assert_exec_uses(graph, case.original) |
There was a problem hiding this comment.
| case = definition_update_case | |
| assert case.invalid_update is not None | |
| assert case.invalid_exception is not None | |
| with pytest.raises(case.invalid_exception): | |
| case.invalid_update() | |
| case.assert_current(case.original) | |
| graph = case.graph_def.instantiate() | |
| case.assert_exec_uses(graph, case.original) | |
| case = definition_update_case | |
| old_graph = case.graph_def.instantiate() | |
| assert case.invalid_update is not None | |
| assert case.invalid_exception is not None | |
| with pytest.raises(case.invalid_exception): | |
| case.invalid_update() | |
| case.assert_current(case.original) | |
| new_graph = case.graph_def.instantiate() | |
| case.assert_exec_uses(new_graph, case.original) | |
| assert case.original != case.replacement | |
| assert old_graph == new_graph |
There was a problem hiding this comment.
I don't think old_graph == new_graph is correct here? Because they are separate instances.
There was a problem hiding this comment.
Ok, so there is no equality comparison for graph instances?
There was a problem hiding this comment.
There should be an equality based on the underlying CUgraphExec handle, but not a structural equality.
| assert_exec_uses=assert_exec_uses, | ||
| invalid_update=invalid_update, | ||
| invalid_exception=ValueError, | ||
| invalid_argument_update=None, |
There was a problem hiding this comment.
| invalid_argument_update=None, | |
| invalid_argument_update=lambda: None, |
| def _host_callback_ctypes_case(device): | ||
| """Use ctypes callbacks and copied payloads to distinguish each exec.""" | ||
| callback_type = ctypes.CFUNCTYPE(None, ctypes.c_void_p) | ||
| called = [] |
There was a problem hiding this comment.
| called = [] | |
| invalid_callback_type = ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_void_p) | |
| called = [] |
| @callback_type | ||
| def replacement_fn(data): | ||
| called.append((replacement_fn, read_byte(data))) | ||
|
|
There was a problem hiding this comment.
| @invalid_callback_type | |
| def invalid_fn(data): | |
| return 0 |
| assert called == [(expected[0], expected[1][0])] | ||
|
|
||
| def invalid_update(): | ||
| node.update(lambda: None, user_data=b"not valid for a Python callback") |
There was a problem hiding this comment.
| node.update(lambda: None, user_data=b"not valid for a Python callback") | |
| node.update(invalid_fn, user_data=b"not valid for a Python callback") |
| unexpected_dst = replacement_dst if expected["dst"] is original_dst else original_dst | ||
| assert list(as_bytes(unexpected_dst)) == [0] * 4 | ||
|
|
||
| def cleanup(): |
There was a problem hiding this comment.
I am surprised to see all this explicit cleanup. Is it necessary and cuda.python applications need to do this as well?
There was a problem hiding this comment.
You're right, it should not be needed. This was added to work around a teardown error (below). I will continue working on that.
Warning: mr.deallocate() failed during Buffer destruction: CUDA_ERROR_INVALID_CONTEXT: This most frequently indicates that there is no context bound to the current thread. This can also be returned if the context passed to an API call is not a valid handle (such as a context that has had cuCtxDestroy() invoked on it). This can also be returned if a user mixes different API versions (i.e. 3010 context with 3020 API calls). See cuCtxGetApiVersion() for more details. This can also be returned if the green context passed to an API call was not converted to a CUcontext using cuCtxFromGreenCtx API.
|
|
||
| def invalid_update(): | ||
| if replace == "kernel": | ||
| node.update(kernel=replacement_kernel) |
There was a problem hiding this comment.
Shouldn't we also test updating a kernel with wrong args?
For this case, empty_kernel does not take any args, so I would expect updating with kernel=empty_kernel, args=original_args to also fail.
There was a problem hiding this comment.
cuda.core does not currently validate kernel arguments for ordinary launches either. cuKernelGetParamInfo, which enables count and size validation, requires CUDA 12.4. This should be addressed consistently across launch, graph construction, and update paths as separate work rather than only testing updates here.
| if c_dst_type == cydriver.CU_MEMORYTYPE_HOST: | ||
| c_dst = <cydriver.CUdeviceptr><uintptr_t>( | ||
| params.memcpy.copyParams.dstHost) | ||
| elif c_dst_type != cydriver.CU_MEMORYTYPE_ARRAY: |
There was a problem hiding this comment.
Would be safer to just make this else (same on line 824 below). Doesn't make a difference at the moment because of line 810. Otherwise follow the elif ...: with
else:
raise NotImplementedError("implement me")
| cdef cydriver.CUdeviceptr c_dst | ||
| cdef unsigned int c_value | ||
| cdef unsigned int c_element_size | ||
| cdef size_t c_width | ||
| cdef size_t c_height | ||
| cdef size_t c_pitch | ||
| cdef OpaqueHandle dst_attachment_owner | ||
| cdef GraphHandle h_graph | ||
| cdef cydriver.CUgraphNode node = as_cu(self._h_node) | ||
| cdef cydriver.CUcontext ctx = NULL | ||
| cdef cydriver.CUDA_MEMSET_NODE_PARAMS current | ||
| cdef cydriver.CUgraphNodeParams params | ||
| cdef object queried | ||
|
|
||
| if dst is None and dst_owner is not None: | ||
| raise ValueError("dst_owner requires dst") | ||
| if (dst is None and value is None and width is None and | ||
| height is None and pitch is None): | ||
| return | ||
|
|
||
| c_memset(¶ms, 0, sizeof(params)) | ||
| params.type = cydriver.CU_GRAPH_NODE_TYPE_MEMSET | ||
| with nogil: | ||
| HANDLE_RETURN(cydriver.cuGraphMemsetNodeGetParams( | ||
| node, ¤t)) | ||
| if _check_node_get_params(): | ||
| queried = handle_return(driver.cuGraphNodeGetParams( | ||
| <uintptr_t>node)) | ||
| ctx = <cydriver.CUcontext><uintptr_t>int(queried.memset.ctx) | ||
| else: | ||
| with nogil: | ||
| HANDLE_RETURN(cydriver.cuCtxGetCurrent(&ctx)) | ||
| params.memset.dst = current.dst | ||
| params.memset.value = current.value | ||
| params.memset.elementSize = current.elementSize | ||
| params.memset.width = current.width | ||
| params.memset.height = current.height | ||
| params.memset.pitch = current.pitch | ||
| params.memset.ctx = ctx | ||
|
|
||
| c_dst = params.memset.dst | ||
| c_value = params.memset.value | ||
| c_element_size = params.memset.elementSize | ||
| c_width = params.memset.width | ||
| c_height = params.memset.height | ||
| c_pitch = params.memset.pitch | ||
|
|
||
| if dst is None: | ||
| h_graph = graph_node_get_graph(self._h_node) | ||
| HANDLE_RETURN(graph_get_attachment( | ||
| h_graph, node, | ||
| &dst_attachment_owner, NULL)) | ||
| else: | ||
| dst_attachment_owner = _resolve_memcpy_operand( | ||
| dst, dst_owner, "dst", &c_dst) | ||
|
|
||
| if value is not None: | ||
| c_value, c_element_size = _parse_fill_value(value) | ||
| if width is not None: | ||
| c_width = width | ||
| if height is not None: | ||
| c_height = height | ||
| if pitch is not None: | ||
| c_pitch = pitch | ||
|
|
||
| params.memset.dst = c_dst | ||
| params.memset.value = c_value | ||
| params.memset.elementSize = c_element_size | ||
| params.memset.width = c_width | ||
| params.memset.height = c_height | ||
| params.memset.pitch = c_pitch | ||
|
|
There was a problem hiding this comment.
| cdef cydriver.CUdeviceptr c_dst | |
| cdef unsigned int c_value | |
| cdef unsigned int c_element_size | |
| cdef size_t c_width | |
| cdef size_t c_height | |
| cdef size_t c_pitch | |
| cdef OpaqueHandle dst_attachment_owner | |
| cdef GraphHandle h_graph | |
| cdef cydriver.CUgraphNode node = as_cu(self._h_node) | |
| cdef cydriver.CUcontext ctx = NULL | |
| cdef cydriver.CUDA_MEMSET_NODE_PARAMS current | |
| cdef cydriver.CUgraphNodeParams params | |
| cdef object queried | |
| if dst is None and dst_owner is not None: | |
| raise ValueError("dst_owner requires dst") | |
| if (dst is None and value is None and width is None and | |
| height is None and pitch is None): | |
| return | |
| c_memset(¶ms, 0, sizeof(params)) | |
| params.type = cydriver.CU_GRAPH_NODE_TYPE_MEMSET | |
| with nogil: | |
| HANDLE_RETURN(cydriver.cuGraphMemsetNodeGetParams( | |
| node, ¤t)) | |
| if _check_node_get_params(): | |
| queried = handle_return(driver.cuGraphNodeGetParams( | |
| <uintptr_t>node)) | |
| ctx = <cydriver.CUcontext><uintptr_t>int(queried.memset.ctx) | |
| else: | |
| with nogil: | |
| HANDLE_RETURN(cydriver.cuCtxGetCurrent(&ctx)) | |
| params.memset.dst = current.dst | |
| params.memset.value = current.value | |
| params.memset.elementSize = current.elementSize | |
| params.memset.width = current.width | |
| params.memset.height = current.height | |
| params.memset.pitch = current.pitch | |
| params.memset.ctx = ctx | |
| c_dst = params.memset.dst | |
| c_value = params.memset.value | |
| c_element_size = params.memset.elementSize | |
| c_width = params.memset.width | |
| c_height = params.memset.height | |
| c_pitch = params.memset.pitch | |
| if dst is None: | |
| h_graph = graph_node_get_graph(self._h_node) | |
| HANDLE_RETURN(graph_get_attachment( | |
| h_graph, node, | |
| &dst_attachment_owner, NULL)) | |
| else: | |
| dst_attachment_owner = _resolve_memcpy_operand( | |
| dst, dst_owner, "dst", &c_dst) | |
| if value is not None: | |
| c_value, c_element_size = _parse_fill_value(value) | |
| if width is not None: | |
| c_width = width | |
| if height is not None: | |
| c_height = height | |
| if pitch is not None: | |
| c_pitch = pitch | |
| params.memset.dst = c_dst | |
| params.memset.value = c_value | |
| params.memset.elementSize = c_element_size | |
| params.memset.width = c_width | |
| params.memset.height = c_height | |
| params.memset.pitch = c_pitch | |
| cdef OpaqueHandle dst_attachment_owner | |
| cdef GraphHandle h_graph | |
| cdef cydriver.CUgraphNode node = as_cu(self._h_node) | |
| cdef cydriver.CUcontext ctx = NULL | |
| cdef cydriver.CUDA_MEMSET_NODE_PARAMS current | |
| cdef cydriver.CUgraphNodeParams params | |
| cdef object queried | |
| if dst is None and dst_owner is not None: | |
| raise ValueError("dst_owner requires dst") | |
| if (dst is None and value is None and width is None and | |
| height is None and pitch is None): | |
| return | |
| c_memset(¶ms, 0, sizeof(params)) | |
| params.type = cydriver.CU_GRAPH_NODE_TYPE_MEMSET | |
| with nogil: | |
| HANDLE_RETURN(cydriver.cuGraphMemsetNodeGetParams( | |
| node, ¤t)) | |
| if _check_node_get_params(): | |
| queried = handle_return(driver.cuGraphNodeGetParams( | |
| <uintptr_t>node)) | |
| ctx = <cydriver.CUcontext><uintptr_t>int(queried.memset.ctx) | |
| else: | |
| with nogil: | |
| HANDLE_RETURN(cydriver.cuCtxGetCurrent(&ctx)) | |
| cdef cydriver.CUdeviceptr c_dst = current.dst | |
| cdef unsigned int c_value = current.value | |
| cdef unsigned int c_element_size = current.elementSize | |
| cdef size_t c_width = current.width | |
| cdef size_t c_height = current.height | |
| cdef size_t c_pitch = current.pitch | |
| if dst is None: | |
| h_graph = graph_node_get_graph(self._h_node) | |
| HANDLE_RETURN(graph_get_attachment( | |
| h_graph, node, | |
| &dst_attachment_owner, NULL)) | |
| else: | |
| dst_attachment_owner = _resolve_memcpy_operand( | |
| dst, dst_owner, "dst", &c_dst) | |
| if value is not None: | |
| c_value, c_element_size = _parse_fill_value(value) | |
| if width is not None: | |
| c_width = width | |
| if height is not None: | |
| c_height = height | |
| if pitch is not None: | |
| c_pitch = pitch | |
| params.memset.dst = c_dst | |
| params.memset.value = c_value | |
| params.memset.elementSize = c_element_size | |
| params.memset.width = c_width | |
| params.memset.height = c_height | |
| params.memset.pitch = c_pitch | |
| params.memset.ctx = ctx | |
| commit_status = graph_commit_child_graph_update( | ||
| prepared, &h_replacement) | ||
| finally: | ||
| if h_replacement: |
There was a problem hiding this comment.
| if h_replacement: | |
| if commit_status == CUDATA_SUCCESS: |
There was a problem hiding this comment.
graph_commit_child_graph_update returns CUDA errors through the status, but C++ errors are reported by throwing, so commit_status is not correct here.
There was a problem hiding this comment.
Hm, the h_replacement handle is not initialized on line 919. Why does this work, i.e., why would the if h_replacement: path not always be taken?
The function throws when a CUDA error occurred here, btw.
Use the generic node setter with failure-atomic attachment replacement, establishing the shared path for definition-level parameter mutation.
Extend definition-level mutation to event waits and both Python and ctypes host callbacks while preserving old executable state and attachment ownership.
Report unsupported driver or binding versions before preparing mutation attachments or calling the generic node setter.
Allow partial memset parameter replacement while preserving graph-owned destination lifetimes and previously instantiated graph behavior.
Support partial copy parameter replacement while preserving independent source and destination ownership across graph instantiations.
Support independent launch configuration and argument replacement while requiring explicit arguments when changing kernels.
Replace embedded child hierarchies while preserving attachment metadata, invalidating stale views, and keeping existing executables independent.
Describe supported mutation methods, CUDA 12.2 requirements, and executable graph behavior in the API and release notes.
Use type-erased shared ownership for prepared child updates so extension loading does not depend on a hidden C++ deleter symbol.
Reflect shared ownership for the opaque child update transaction in the generated stub.
Make memcpy and memset mutation calls explicit and unambiguous before the public API freezes.
Preserve memory-node contexts, reject unsupported node forms, and fail clearly when child graph metadata cannot be updated.
Resolve the CUDA 13.2 graph parameter getter dynamically so CUDA 12 binding builds remain compilable.
Clarify parameter handling and cover host/device memory transitions while exposing context-sensitive test teardown for follow-up.
Cover the public invalid-node state to ensure parameter updates fail cleanly without restoring graph membership.
fe48941 to
1b0fdbd
Compare
|
/ok to test |
Summary
Add subclass-specific
update()methods for mutable graph definition nodes. The new API supports kernel, memcpy, memset, child-graph, event-record, event-wait, and host-callback nodes while leaving inspection properties read-only.Updates replace one complete CUDA parameter structure through
cuGraphNodeSetParams. Optional keyword arguments preserve existing values where coherent, coupled values such as kernels and arguments are replaced together, and existing executable graphs continue using their previous parameters and retained resources. This feature requires CUDA driver andcuda.bindingsversions 12.2 or newer.Changes
update()methods forKernelNode,MemcpyNode,MemsetNode,ChildGraphNode,EventRecordNode,EventWaitNode, andHostCallbackNode.args=()for a no-argument kernel.Review guide
cuda_core/cuda/core/graph/_subclasses.pyxand_subclasses.pyifor the public update signatures and node-specific parameter construction._set_definition_node_paramsfor the common CUDA 12.2 gate and attachment transaction ordering.resource_handles.cpp/.hppfor staged child-hierarchy metadata replacement and shared clone/rekey primitives.test_graph_node_update.pyfor old-versus-new executable behavior and failure preservation, thentest_graph_definition_lifetime.pyfor child-view invalidation and attachment lifetime coverage.Related Work