[1/5] feat: NVSHMEM symmetric window for GPU AFD transport - #332
specture724 wants to merge 2 commits into
Conversation
A one-sided transport with no control plane: a rank writes a whole slot into a peer's symmetric window -- header, routing indices, weights, payload -- and stamps a flag last, on the same stream, so a peer that sees the flag sees the payload. The flag protocol is what makes the window CUDA-graph-capturable, and that fixes its shape. A captured stream wait compares against a value baked in at capture time, so a sender cannot signal with a fresh sequence number every replay. It signals with a constant marker instead, and the reader resets the flag in-band once it has consumed the slot, which makes every replay identical. `cuda_rt` and `nvshmem_rt` are thin ctypes bindings: stream memory ops for the in-stream wait, and NVSHMEM's symmetric allocation plus peer pointer lookup. No AFD concepts appear anywhere in this layer -- it is a wire format and the driver calls under it, and the connector that uses it comes next. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: specture724 <specture724@gmail.com>
316bbbb to
6509abd
Compare
hsliuustc0106
left a comment
There was a problem hiding this comment.
Consolidated review of 6509abd (static review; pre-commit/DCO green). No blockers; five P2s:
[P2] Validate the symmetric-heap layout across ranks — afd_plugin/connectors/gpu/symm_window.py:328
__init__ never exchanges total_bytes/capacities over the group, and the receiver only checks magic/version, so config drift between role deployments produces unequal heaps (deep transport crash) or silently clamped partial reads (_view(...)[: sizes[0]] at :472 and :735-755) with no error. All-gather the capacity tuple in __init__ and raise on mismatch, naming the differing parameter.
[P2] Fence the constructor zero-reset against earlier peers — afd_plugin/connectors/gpu/symm_window.py:329
A peer returning from its own collective malloc can issue its first fabric write while this rank's zero_() is still queued behind stream work; the zero erases the arriving flag and the receiver spins forever. End __init__ with a group.barrier() after the synchronize, or state the required post-construction collective as a class-level contract that #333's handshake must provably provide.
[P2] 1,191 of 1,336 added lines are unreachable at this head — afd_plugin/connectors/gpu/symm_window.py:303
Only the test file imports symm_window; the factory registry and connectors/gpu/__init__.py are untouched. Acceptable as part 1/5 if #333/#334 land immediately, but if the stack stalls, main carries dead transport whose ctypes ABI assumptions (struct sizes, (1<<16)+sizeof versioning, libnvshmem_host.so.3 discovery) have zero executable evidence. The series should not be called complete before a GPU-gated test or E2E exercises the real window.
[P2] Justify the module-level mutable globals — afd_plugin/connectors/gpu/cuda_rt.py:31
_lib/_wait_value32/_checked_devices are new process-wide mutable state without the justification AGENTS.md requires; nvshmem_rt.py:110 already models the expected comment.
[P2] Update the connector design page — docs/design/module/connector_contracts.md
afd_plugin/connectors/**/*.py is a primary path of this page: the NVSHMEM substrate needs an evidence-table row (window/rt files ↔ test_symm_window.py) plus a limitations line for the NVSHMEM host library, NVLink-reachable PEs, and stream-mem-ops device gate. Backend-mode/topology rows can ride with #333.
Validation gaps: no Buildkite test-ready run in the rollup; no GPU-marked test anywhere in the series yet; the docstring perf numbers (hot-spin vs backoff TTFT, launch-latency percentages) need linked artifacts when #334's DSV4 runs land.
Review follow-up on #332. A sender writes into a peer's window using its own idea of the slot geometry, and the receiver only checks magic and version, so a configuration difference between the two role deployments never surfaces as an error: unequal heaps fault inside the transport, and a smaller capacity silently clamps a partial read. All-gather the geometry before allocating and raise naming the one parameter that differs. The constructor's zero-reset also needed a fence. The collective malloc orders the allocation, not the reset: a peer returning from its own malloc first can land a dispatch while this rank's zero_() is still queued, and the zero erases the arriving flag, leaving that peer waiting on a reply for a slot this rank never saw. Also document why cuda_rt caches the driver handle process-wide, and give the NVSHMEM substrate an evidence row plus a limitations paragraph in connector_contracts.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — all five addressed in Symmetric-heap layout across ranks. Constructor zero-reset fence. You were right that the collective malloc does not order this: it orders the allocation, and a peer that returns first can land a dispatch while the Module-level mutable globals. Documented, in the shape Design page. Unreachable at this head. No change — this is inherent to a 1/5 PR, and Validation gaps noted — the GPU-marked test and the artifacts behind the docstring's perf numbers are still outstanding. |
First of a five-PR stack that brings up
GpuAsyncAFDConnectorand runsDeepSeek-V4 on it. Review and merge in order; each PR is based on the one
before it.
What this adds
A one-sided transport with no control plane: a rank writes a whole slot into a
peer's symmetric window — header, routing indices, weights, payload — and stamps
a flag last, on the same stream, so a peer that sees the flag sees the payload.
The flag protocol is what makes the window CUDA-graph-capturable, and that fixes
its shape. A captured stream wait compares against a value baked in at capture
time, so a sender cannot signal with a fresh sequence number every replay. It
signals with a constant marker instead, and the reader resets the flag in-band
once it has consumed the slot, which makes every replay identical.
cuda_rtandnvshmem_rtare thin ctypes bindings: stream memory ops for thein-stream wait, and NVSHMEM symmetric allocation plus peer pointer lookup.
Scope
No AFD concepts appear in this layer at all — it is a wire format and the
driver calls under it, and it imports nothing from the rest of the plugin. That
is why it is a separate PR: it can be reviewed by itself.
Testing
tests/unit/connectors/gpu/test_symm_window.pypins the slot layout and theheader codec (field offsets, what a header does and does not carry, corrupt
magic, shutdown flag round trip). Runs on CPU, no GPU needed.
Full unit suite is unchanged against
main: same 13 pre-existing failures,none added.
pre-commitclean over the PR range.🤖 Generated with Claude Code