[Store] Fix current-stream readiness for DummyClient CUDA IPC tensor writes - #3303
[Store] Fix current-stream readiness for DummyClient CUDA IPC tensor writes#3303mo-ke-ke wants to merge 3 commits into
Conversation
4ddc1ea to
530e50e
Compare
|
@stmatengss @zxpdemonio @ykwd, when you have a chance, could one of you please take a look at this narrow follow-up to #3234? It fixes a reproduced PyTorch current-stream readiness race in the DummyClient CUDA IPC tensor-write path without changing the wire/RPC schema; the pre-fix case reproduced at 0/20 versus 20/20 here, and CI is green. |
There was a problem hiding this comment.
I reviewed this change with emphasis on API compatibility, failure atomicity, CUDA/non-CUDA portability, batch fallback, and data exposure. I found no blocking issues.
Why our earlier testing missed this race: our performance scripts explicitly called torch.cuda.synchronize() after tensor initialization and again before every put_tensor. That made every source tensor host-ready before CUDA IPC export, so those runs measured ready-tensor throughput but did not exercise PyTorch current-stream ordering. The earlier dummy tensor regression also used CPU tensors. This PR closes that gap by asserting that the producer event is still pending at the call boundary after current.wait_stream(producer), for both single and batch writes.
I also validated commit 530e50e6 on an NVIDIA A10 / CUDA 12.8 test host:
- CUDA
store,mooncake_client, andmooncake_mastertargets built successfully. - The two new stream-readiness tests passed: 2/2.
- Existing dummy tensor operations and mixed
put/put_tensorconcurrency tests passed: 2/2. - Changed-line formatting and
git diff --checkpassed. - The upstream CI matrix is green, including non-CUDA and ROCm builds.
The public Python signatures and RPC/wire schema remain unchanged. CPU, zero-sized, mixed CPU/CUDA, and non-exportable batches retain the existing whole-batch staging fallback. Stream validation/synchronization completes before RPC submission, so a failure cannot publish a partial Store write; the new error logs also do not expose keys, pointers, or raw exception text.
Residual, non-blocking risks: the committed regression relies on private torch.cuda._sleep and will skip where it is unavailable; forced CUDA stream-failure injection is not covered.
|
Follow-up after reproducing the test command exactly: the production fix still looks correct, but I found two issues in the new regression test that should be closed before relying on it as a gate. Validation on NVIDIA A10 / CUDA 12.8 at
Two test changes are needed:
I found no additional blocker in the production implementation: current-stream capture happens under the GIL, device/stream validation and synchronization happen before RPC submission, partial writes are not published on synchronization failure, and the public API/wire schema are unchanged. |
|
@zxpdemonio Fixed both test-gate issues in e47e341: only a top-level missing I reran the exact two-test invocation on B300 without preloading PyTorch: 2 passed, 4 subtests passed, 0 skipped. The updated head's CI gate is currently tripped by an unrelated repo-wide typos finding on current main at |
|
@XucSh @YiXR @stmatengss @ykwd The updated head is now green on the current main (24 successful, 1 skipped, 0 failed), and the two test-gate follow-ups from the existing review are addressed in e47e341. When one of you has availability, could you provide the required Store CODEOWNERS review? It is the only remaining merge gate. |
Description
The DummyClient CUDA IPC tensor-write fast path can expose a tensor allocation to the receiver before work queued on the caller's PyTorch current stream has completed. For example, after
current.wait_stream(producer), the dependency exists oncurrent, but the existing path exports the memory handle and starts the RPC without observing that dependency. The receiver can therefore consume stale or partially written data.This change:
(device_id, stream_handle)pairs and synchronizes only streams associated with requests that will be sent;Callers remain responsible for joining work from other producer streams into the current stream, for example with
current.wait_stream(producer).This is deliberately narrow: CPU tensors and the staging fallback are unchanged, and the patch adds no device-wide synchronization, wire-format field, RPC endpoint, serialized ABI change, or capability negotiation.
Related work: #3234 introduced the DummyClient CUDA IPC tensor path fixed here. #1946 targets registered Store buffers and the local-replica GPU/CPU transfer path, while #3159 targets CUDA IPC upsert. Neither covers PyTorch current-stream readiness for the existing
put_tensor, batch, and TP write path. This PR does not modify either path or any wire/RPC schema.Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Test environment: NVIDIA B300, CUDA 12.8, PyTorch 2.11.0+cu128.
Test commands:
Correctness results:
main(a6b4db4)A separate synthetic matrix using a non-default current stream covered async/control and single/batch cases at 20 trials each: 80/80 correct with zero payload mismatches.
Fresh CUDA and no-CUDA configurations built successfully, and the Python extension imported successfully from both builds.
Ready-tensor performance:
Pre-commit:
Every changed-file hook except
ruff-formatpassed.ruff checkreports zero findings on bothorigin/mainand this branch.ruff format --checkhas the same whole-file failure on both revisions, so that hook was skipped to avoid an unrelated full-file reformat.Test results:
Checklist
./scripts/code_format.shpre-commit run --all-filesand all hooks passAI Assistance Disclosure
OpenAI Codex assisted with source inspection, CUDA stream-ordering analysis, test design, implementation, validation, and review. The submitter remains responsible for understanding and defending the change.