Repository navigation
[Fix] nccl: Disable graph buffer registration whenever a captured graph can hold a collective - #43216
Conversation
…can capture a collective Both guards key on a feature flag rather than on the aliasing they protect against, so a plain multi-node TP deployment -- no DP attention, no memory saver -- falls through and keeps NCCL_GRAPH_REGISTER on. Reproduced with two TP16 engines across two H200 nodes each, decode batch 128, CUDA graph on: every TP rank hits the 300s scheduler watchdog in the same second, blocked in cuEventSynchronize under process_batch_result_decode. Running the same workload with NCCL_GRAPH_REGISTER=0 stayed clean for 5h (76k requests, 70M tokens, 148 pause/resume cycles). Throughput A/B at batch 128, 10 min each: 2,060,061 tokens with the registration on versus 2,066,577 with it off, so the guard can track the hazard instead of enumerating the shapes that expose it. The replaced test asserted the variable stays unset without the TP LM-head all-to-all, which this deliberately changes; the isolating cases for the pausable pool, a single rank, and a disabled decode graph take its place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… phase A decode-only check drops the case the pausable-pool guard used to cover: with --enable-memory-saver and the decode capture off, a prefill capture still puts its collectives in that pool, and the registration stayed on. Keying on any non-disabled phase restores it and keeps the single condition. Adds the regression case; it fails on the previous revision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zhihengy
changed the title
Oasis-Git
left a comment
Oasis-Git
left a comment
There was a problem hiding this comment.
[Codex]: Three non-blocking suggestions from source inspection. I did not run local tests or E2E validation.
| ), | ||
| **kwargs, | ||
| ) | ||
| fields.update(kwargs) |
There was a problem hiding this comment.
[Codex]: Could we add a regression case with attn_dp_size=1, tp_size=2, enable_tp_lm_head_all_to_all=False, and enable_memory_saver=False, asserting that an initially unset NCCL_GRAPH_REGISTER becomes 0? The helper defaults to attention DP, so the new cases without LM-head all-to-all still do not directly cover the plain TP deployment that motivates this change.
| capture into a pausable pool, which the pool guard used to cover. | ||
|
|
||
| Errs toward True. Later passes can still narrow cuda_graph_config, and | ||
| turning the registration off has no measured throughput cost, so a false |
There was a problem hiding this comment.
[Codex]: Could we qualify this docstring as a conservative workaround and limit the throughput statement to the measured deployment? The PR description says the internal NCCL root cause remains open and the performance comparison has one run per configuration. The aliasing explanation above reads as an established mechanism, and that measurement does not establish the cost of disabling registration across all deployment shapes covered by this guard.
| view = resolving_view(server_args) | ||
| if max(view.tp_size, view.ep_size, view.dp_size) <= 1: | ||
| return False | ||
| config = getattr(view, "cuda_graph_config", None) |
There was a problem hiding this comment.
[Codex]: Could this use direct access to view.cuda_graph_config, retaining the None check if needed? The resolution pipeline initializes this field before handle_data_parallelism, and that handler already accesses it directly in the attention-DP branch. The getattr fallback appears unnecessary and can hide a missing-field contract violation.
This comment has been minimized.
This comment has been minimized.
zhihengy
changed the title
|
[Codex]: Use /tag-and-rerun-ci to trigger base CI, as requested. |
|
/tag-and-rerun-ci |
|
[Codex]: Base CI attempt 2 for 8c615a8 was blocked before tests ran: the check-maintenance gate requires rebasing onto current main containing 6a39ddf. This is a rebase prerequisite, not a test failure. Base run, attempt 2 · Failing gate. The standard /tag-and-rerun-ci handler also retried other unsuccessful workflows, including Extra; no Extra opt-in was requested or added by this command. |
Jiminator
added
the
bypass-fail-fast
…ph can hold a collective (sgl-project#43216) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Jimmy Shong <69131491+Jiminator@users.noreply.github.com> (cherry picked from commit 786cdfc) (cherry picked from commit 6c22672fd6ebefe21e735b341653e2300d3a0eff)
… / qsaring / hicachectrl), header bump to qwennext-stack-295-2026-10-28, tip 44134b3, CAVEATS re-pin table + sgl-project#43216 boot-needle bullet + sgl-project#33029 NOT-NEEDED bullet + merge-sgl-project#5 owed refresh 21 picks, ZERO conflicts at the integrator; per-wave file-set and byte-parity proofs, entry-209 subsumption map (290), the QSA remedy adjudication with the layout-equivalence proof (285/286/287), the entry-199 non-interference machine check (294), the flag-collision record (295) and the full battery attribution are in the entries. Nothing pushed.
… CAVEATS + wanted_stack) -- the 'no explicit-override warning / operator setting silently wins' note is FALSE against the landed code: parallel_hook.py:445-449 logs a warning when NCCL_GRAPH_REGISTER was set explicitly, and that block PREDATES the pick. Recorded what actually moved (the two narrow triggers -> _graph_may_capture_collectives, our entry-sgl-project#38936-era _graph_pool_is_pausable helper subsumed and deleted, no dangling callers) and turned the warning line into the live boot needle.
… header STACK/tip updated, 5 owed-paragraphs marked HISTORY, CAVEATS top bullet = MERGE-sgl-project#5 ERA (PROD BOOT FORBIDDEN until B3: the 209 clamp + 218 ordering rule with the trailing_endpoint 17P->8P/9F proof-of-life-when-restored), wanted_stack Section C consumed (C0a CLOSED with the four arrival shas, C1-C6 ARRIVED per the merge-message D-list, C9 OPT-IN-verified, C10 sgl-project#45090 taken + the operator-owed /root-harness check, 33-trailer families spot-verified IN-RANGE, sgl-project#43216 DEDUPED); plus the kernels kvcache store_cache_fp8 registry graft found by the damage battery (kernels_namespace 107P/0F restored); battery damage map recorded in full in entry 310
Problem
NCCL_GRAPH_REGISTERis on by default and registers the send/recv buffers of every graph-captured collective for the lifetime of the graph. Those buffers are graph-pool temporaries, and the pool hands the same addresses to other tensors, so a replay can find its registration pointing at memory that now belongs to something else.Both existing guards key on a feature flag rather than on that aliasing:
enable_tp_lm_head_all_to_alladditionally requires PD-decode, DP attention,dp_size > 1andtp_size == dp_sizeenable_memory_saverplusSGLANG_MEMORY_SAVER_CUDA_GRAPH, which is off by defaultA plain multi-node TP deployment matches neither, so the registration stays on and the hang is reachable.
Features: more likely to appear under large batch size (> 128); not directly related to torch_memory_saver; currently does not appear in PREFILL stage but we still use any stage as gate to not regressing from the previous gate.
Reproduction
Two TP16 engines spanning two H200 nodes each, decode batch 128, CUDA graph on, radix cache on,
max-running-requests 128, a 501B MoE model. Load is 160 concurrent 4096-token generations per engine plus a pause/flush/continue cycle every 120s.enable_tp_lm_head_all_to_allandenable_memory_saverare both false in every run below.Every TP rank hits the 300s scheduler watchdog within the same second:
NCCL_GRAPH_REGISTER00Run 4 is why the dcf4e18 cherry-pick alone is not enough: it is in the tree, neither of its conditions matches, so the variable is never set and the hang is unchanged.
Runs 6 and 7 rule out a stale NCCL as the cause. 2.32.3 carries the fix for NVIDIA/nccl#2389, which has a similar signature, and the hang still reproduces with the registration on and still disappears with it off.
Two further conditions were tested and are not required:
NCCL_PXN_DISABLE=1still hangs (7 min), and all of the above runs have the TP LM-head all-to-all and the memory saver off. Disabling GDR (NCCL_NET_GDR_LEVEL=0) does hold for 50 min with the registration on, which points at the network registration path, but that is not something this layer can detect at argument-resolution time.Cost
Same engine, same tree, same load, 10 minutes each:
No measurable throughput difference. Single run per configuration, not repeated for variance. Worth noting that the non-registered path is the one every deployment used before the default flipped, so it is not a new or lightly-exercised code path.
Change
One condition replaces both:
max(tp_size, ep_size, dp_size) > 1with any graph phase still enabled means a capture can hold a collective, which is exactly when a registration can outlive the pool's claim on the address. It errs toward disabling — later passes can still narrowcuda_graph_config, and a false positive costs nothing measurable.Keying on any phase rather than decode alone keeps the pausable-pool case that dcf4e18 added: with the decode capture off and the memory saver on, a prefill capture still lands in that pool.
The test that asserted the variable stays unset without the TP LM-head all-to-all encoded the old contract and now asserts the new one. Added alongside it: the pausable pool in isolation, a prefill-only capture, a single rank, and a fully disabled graph.
Draft while the deployment-shape coverage gets a second opinion — happy to narrow the condition if there is a configuration where the registration is known to be safe and worth keeping. The root cause is still open: these runs establish which conditions reproduce it and which do not, not the mechanism inside NCCL.
CI States
Latest PR Test (Base): ✅ Run #39685648032
Latest PR Test (Extra): ❌ Run #39685648027
Latest PR Test (AMD ROCm 10): ❌ Run #39685648048