Repository navigation
Conversation
…erand boundary Follow-up to NVIDIA#3029. The release note said Graph.update() applies any replaced operand of a captured memcpy node; the driver accepts only a device-to-device replacement in an executable update, and a replacement with host memory takes effect in a new instantiation. A test pins that boundary, and ExecutableMemcpyNode.update() records why it may read the memory types from the definition node. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Andy-Jost
added
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe changes document executable memcpy update handling, clarify captured operand behavior in the release notes, and add a test for replacing a captured device operand with pinned host memory. ChangesCaptured memcpy updates
Assessment against linked issues
Suggested reviewers: Priority: ➖ Normal Change: Other Merge Risk: 🔵 Low · up to The new test leaves a focused gap in coverage for direct executable-node updates from device to host memory. The PR is otherwise documentation, comments, and tests, so it is mergeable with that limitation tracked.
Comment |
|
CI showed that Windows TCC drivers accept a host buffer as the replacement operand of a captured memcpy node in an executable update, while Linux drivers reject it. The test now accepts either outcome and checks the copy when the update is accepted, and the release note says the driver decides. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cuda_core/tests/graph/test_graph_node_update.py (1)
931-932: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winsuggestion: Instantiate
otherbefore updating the definition node.
_capture_device_memcpycaptures a device-to-device copy. Sinceotheris created afternode.update(src=host_src), its node-level update supplies the host source it already uses. The earlier executable only exercisesinstantiated_before.update(graph_def), the whole-graph update path.Suggested fix
@@ -899,6 +899,7 @@ # The builder is unused; it keeps the captured graph alive. builder, graph_def, node, dst, src = _capture_device_memcpy(init_cuda, stream) instantiated_before = graph_def.instantiate() + other = graph_def.instantiate() host_src = LegacyPinnedMemoryResource().allocate(64) ctypes.memset(int(host_src.handle), 0xA5, 64) @@ -928,7 +929,6 @@ else: assert "PARAMETERS_CHANGED" in str(rejected) - other = graph_def.instantiate() rejected = _cuda_error_from(lambda: other[node].update(dst=dst, src=host_src, size=64))
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
f917a584-4241-4e7c-965a-8e62ad5e5600
📒 Files selected for processing (2)
cuda_core/docs/source/release/1.3.0-notes.rstcuda_core/tests/graph/test_graph_node_update.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
juenglin
left a comment
juenglin
left a comment
There was a problem hiding this comment.
Agree with CodeRabbit,
One new test does not cover the node-level executable update path it intends to check; adjusting the instantiation order would fix that.
A diagnostic run across Linux, Windows MCDM, and Windows TCC rows showed that the executable-update outcome follows the allocation kind, not the platform: the driver compares the memory class of pool-backed and virtual-memory operands and rejects a replacement by host memory or by a cuMemAlloc buffer, while plain cuMemAlloc operands accept it. The TCC runners report no memory-pool support, so the default memory resource is not pool-backed there, which is why the update was accepted on them. The release note and the test docstring now say so. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This branch has not been deployed
2 participants
Summary
Follow-up to #3029, which fixed #2649. The review there found that the release note overstated what
Graph.update()accepts once a captured memcpy node's operand is replaced, and asked for a test of that boundary and a comment in the executable update path. This PR makes those three changes and nothing else.Changes
Graph.update(). A replacement with host memory always takes effect in a new instantiation; whether an executable update accepts it depends on the driver, and Linux drivers reject it.test_memcpy_update_captured_node_host_operandreplaces a captured node's source with a pinned host buffer and checks that a fresh instantiation copies from it. ForGraph.update()and the executable view it accepts either outcome: aCUDAError(withPARAMETERS_CHANGEDforGraph.update()), or a successful update whose launch copies from the host buffer. The first CI run showed why: Linux and Windows WDDM/MCDM rows reject the update, Windows TCC rows accept it.ExecutableMemcpyNode.update()records in a comment that it reads the memory types from the definition node, which is correct only whileMemcpyNode.update()keeps unified operands unified.builder.Related Work
#3029, #2649, and the review comment that requested these changes: #3029 (comment)
🤖 Generated with Claude Code