Repository navigation
CollectiveX: add NCCL EP as a fourth pluggable EP backend #2331
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,11 +33,14 @@ | |
| # caller-prequantized dispatch (DeepEP blockwise e4m3fn; MoRI per-SKU e4m3 tensor cast). | ||
| # A precision the backend does not list is gated out at generation (no case emitted). | ||
| BACKEND_PRECISIONS = { | ||
| "deepep-v2": ("bf16", "fp8"), | ||
| "mori": ("bf16", "fp8"), | ||
| "uccl-ep": ("bf16", "fp8"), | ||
| # NCCL EP is BF16-only this release: its FP8 machinery exists upstream but RELEASE.md | ||
| # lists it unsupported/untested, so no FP8 case is emitted (see bench/ep_nccl.py). | ||
| "nccl-ep": ("bf16",), | ||
| } | ||
| # Short shard-ID slug per non-normal mode. Normal-mode shard IDs carry no mode | ||
|
Check warning on line 43 in experimental/CollectiveX/sweep_matrix.py
|
||
|
Comment on lines
36
to
43
Contributor
There was a problem hiding this comment. 🟡 This PR adds nccl-ep as a BF16-only backend (sweep_matrix.py:41) with new backend/ll_backends rows in platform_config.json, but doesn't refresh README.md or docs/methodology.md the way the UCCL-EP PR (#2288) did. As a result, README.md:16 / methodology.md:24 now falsely claim FP8 dispatch happens 'on every backend', and README.md's backend scope table (lines 66-70) plus the ll_backends prose (lines 28-30) omit nccl-ep entirely. This is a pre-existing-pattern gap the author already flagged and offered to fix here or in a follow-up. Extended reasoning...What's stale/false: Two related documentation problems land with this PR because it changes backend capability without touching
Why nothing catches this today: These are prose/table facts in Markdown, not code assertions — there is no test that cross-checks README/methodology text against Precedent: The prior UCCL-EP PR (#2288), which added a new backend in exactly the same shape (new adapter + platform_config rows + precision entry), refreshed both Step-by-step proof of the false claim:
Fix: Update
Collaborator
Author
There was a problem hiding this comment. Fixed in
|
||
| # segment so existing references stay valid; a low-latency shard adds "-ll". | ||
| _MODE_SLUG = {"low-latency": "ll"} | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 PR description confirms every low-latency nccl-ep attempt on h100-dgxc, h200-dgxc, b200-dgxc, and b300 wedges (a lagging rank times out its dispatch receive against the ~100s EP timeout and the kernel traps with cudaErrorLaunchFailure/719), yet platform_config.json still enables
ll_backends: {"nccl-ep": [8]}on all four SKUs. Since sweep_matrix.py's_ll_runnablereads this map directly andtest_nccl_ep_rollout_shapelocks the rows in asrunnable, merging as-is schedules guaranteed-failing low-latency EP8 legs on real GPU clusters every timenccl-eporallis swept. The author explicitly asks for a reviewer call on whether to drop or gate these four config rows; recommend gating them out (or dropping thenccl-eplow-latency rows plus the matching test assertion) in this PR rather than merging a coverage row that is known-broken.Extended reasoning...
What the bug is.
configs/platform_config.jsonadds"ll_backends": {..., "nccl-ep": [8]}toh100-dgxc(diff line ~13),h200-dgxc(~line 37),b200-dgxc(~line 57), and adds a brand-newll_backendsblock containingnccl-ep: [8]tob300, which previously had noll_backendsentry at all. These four rows tell the sweep generator that the low-latency (decode) NCCL EP path is runnable at EP8 on every x86 SKU NCCL EP is offered on.How it is dispatched.
sweep_matrix.py::_ll_runnableis a one-line lookup:return ep in platform.get('ll_backends', {}).get(backend, []).resolve_matrixcalls this to decide whether to fan a cell out into a low-latency case, and any cell where it returns True is recorded withdisposition: "runnable"— meaning it is picked up bycollectivex-sweep.ymland actually dispatched to the GPU cluster whenever the workflow is run withbackend: nccl-eporbackend: all. The new testtest_nccl_ep_rollout_shapeintests/test_matrix.pygoes further and assertsll == {(sku, 8) for sku in {h100-dgxc, h200-dgxc, b200-dgxc, b300}}, which locks this broken low-latency scheduling in as expected/passing behavior rather than flagging it as unsupported.Why nothing prevents the failure. The PR's own description, in the 'Known issue — the low-latency path wedges on x86' section, states plainly: 'every low-latency attempt on h100-dgxc, b300 and b200-dgxc wedges: one rank falls behind, its peers time out their dispatch receives against the compile-time ~100s EP timeout, and the kernel traps (cudaErrorLaunchFailure, 719)'. The root cause is identified as the stale-signal/pipeline-desync race in NCCL EP's LL count/flag protocol, filed upstream as NVIDIA/nccl#2303 and fixed by NVIDIA/nccl#2306 — a fix that is NOT present in the pinned
nccl4py[cu13]==0.3.1wheel this tree installs (COLLX_NCCL4PY_SPECinruntime/common.sh). Nothing inep_nccl.py, the launcher backend-selection logic, or the sweep generator gates this LL path off — the config rows are the only thing standing between the sweep and a guaranteed crash, and they currently say 'go'.Why this is actionable within this tree. Unlike the upstream wheel bug itself (which cannot be fixed here), dropping or gating the four
ll_backends: {"nccl-ep": [8]}rows (and the corresponding assertion intest_nccl_ep_rollout_shape) is a purely local, one-PR change. The author is explicit about this being an open decision: 'Reviewer decision requested: drop or gate thosell_backendsrows in a follow-up, or leave them pending a fixed wheel.' Leaving the rows in place as submitted answers that question implicitly (and wrongly) in favor of dispatching the broken path. This also breaks with the precedent already established elsewhere in this same config for known-broken cells — e.g. cross-node EP16 on the same SKUs is recorded as anunsupportedcoverage row (never dispatched) precisely because it faults reproducibly atnccl_ep.cc:2884; the low-latency EP8 legs deserve the same treatment instead of being marked runnable.Step-by-step proof. (1) An operator triggers
collectivex-sweep.ymlwithbackend: nccl-ep(orall) and defaultmodes(blank = all modes). (2) Thesetupjob runssweep_matrix.py, which iterates SKUs and forh100-dgxc/h200-dgxc/b200-dgxc/b300findsll_backends["nccl-ep"] == [8], so_ll_runnable(platform, "nccl-ep", 8)returns True and a low-latency EP8 case is added torequested_caseswithdisposition: "runnable"and folded into an executable shard. (3) Thesweepjob matrix picks up that shard, allocates real GPU nodes via Slurm, builds/activates thenccl4py==0.3.1wheel, and runsbench/run_ep.py --backend nccl-epwithmode=low-latency. (4) Per the PR's own validation notes, one rank in that run falls behind due to the stale-signal race in the wheel's LL protocol; its peers block on their dispatch receive until the ~100s EP timeout fires, and the kernel traps withcudaErrorLaunchFailure(error 719). (5) The shard leg fails, burning a real multi-minute GPU allocation and a red CI leg — reproducibly, every time this cell is swept, because the wheel's fix (NVIDIA/nccl#2306) is not present.Suggested fix. Drop the four
"nccl-ep": [8]entries fromll_backends(leavingbackendsfor normal/HT mode intact, since HT validated green), and drop or adjusttest_nccl_ep_rollout_shape's low-latency assertion accordingly — the same pattern the config already uses to scope out the EP16 cross-node cells asunsupportedrather than dispatching known-broken work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already fixed —
d58d56566(pushed after this review ran) drops all four"nccl-ep": [8]rows fromll_backends, so_ll_runnableno longer schedules those legs, andtest_nccl_ep_rollout_shapenow assertsself.assertEqual(ll, set()). On the current head the onlyll_backendsentries aredeepep-v2/uccl-epon the three x86 SKUs andmorion the three CDNA ones.Root cause for the record: the wedge is not an x86-only quirk. The LL parity-buffer signals carry no generation tag, so at a fixed workload a lapped rank consumes a two-calls-old value and the pipeline desyncs — filed as NVIDIA/nccl#2303, fix in #2306. The rows come back when a fixed wheel ships;
78e595c92documents the gap in the meantime.