-
Notifications
You must be signed in to change notification settings - Fork 243
CollectiveX: label backend maturity and correct the low-latency docs / CollectiveX:标注后端成熟度并修正低延迟文档 #2449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
CollectiveX: label backend maturity and correct the low-latency docs / CollectiveX:标注后端成熟度并修正低延迟文档 #2449
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
98ef7f6
CollectiveX: label backend maturity and correct the low-latency docs
Oseltamivir 6713de0
CollectiveX: record that NVSHMEM_DISABLE_IB does not fix B300 low lat…
Oseltamivir ce469ba
CollectiveX: fix the stale methodology Matrix note and trim the label…
Oseltamivir File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🟡 The Matrix section of docs/methodology.md (lines 87-95, not touched by this PR's diff) still says NCCL EP's LOW_LATENCY algorithm 'carries no ll_backends row on any SKU' and that 'the cells stay out of the matrix until a fixed wheel ships' (citing NVIDIA/nccl#2303). This is now false: configs/platform_config.json lists nccl-ep under ll_backends for all six NVIDIA SKUs, this PR's own README.md edit corrects the identical claim, and tests/test_matrix.py::test_nccl_ep_rollout_shape asserts the same coverage.
Extended reasoning...
The bug.
docs/methodology.md's Matrix section (lines 87-95) still reads:The sentence just above it (line 88) also lists only DeepEP V2, MoRI, and UCCL-EP as carrying an
ll_backendsrow, omitting NCCL EP entirely. Both statements are now false.Why it's false. Three artifacts this exact PR either edits or depends on say the opposite:
configs/platform_config.jsonlists"ll_backends": {"nccl-ep": [8]}for all six NVIDIA SKUs (h100-dgxc, h200-dgxc, b200-dgxc, b300, gb200, gb300).README.mdedit corrects the parallel claim in the README's low-latency paragraph, changing it from the same "NCCL EP has no low-latency row on any SKU" wording to: "NCCL EP EP8 on all six NVIDIA SKUs, restored once the single-handle fix removed the [[Issue]: OW_LATENCY dispatch/combine receives time out on GB200/GB300 NVL72 NVIDIA/nccl#2303] signal aliasing that had wedged them."tests/test_matrix.py::test_nccl_ep_rollout_shapeasserts LOW_LATENCY is runnable EP8 on every one of those six SKUs, and its own comment explains the fix was a single-handle rebind in the adapter (ep_nccl.py's_ensure_handle/_rebind), landed without a wheel bump — directly contradicting methodology.md's "stays out ... until a fixed wheel ships."Why the existing diff doesn't catch this. The PR does touch
docs/methodology.md— it adds theimplementation.maturitybullet in the Result Artifact section (lines ~207-211) — so the file is not untouched by the PR as a whole. But that edit is unrelated to the NCCL EP low-latency claim, and the Matrix-section paragraph a hundred-odd lines earlier was simply missed. Nothing enforces the README and methodology.md low-latency descriptions staying in sync; they are two independent prose copies of the same fact, and this PR fixed one copy (README) while leaving the other (methodology.md) stale — the exact class of drift the PR's stated purpose is to eliminate.Concrete proof. A reader who opens
docs/methodology.mdtoday to determine whether B300, GB200, or GB300 have any low-latency decode coverage will read "NCCL EP ... carries noll_backendsrow on any SKU ... until a fixed wheel ships" and conclude those three SKUs have zero low-latency rows. Butplatform_config.json(ll_backends.nccl-ep: [8]on b300/gb200/gb300) and the passingtest_nccl_ep_rollout_shapetest both say NCCL EP EP8 LL is runnable there today, and per the PR description NCCL EP is precisely the only low-latency row on those three SKUs. So the primary methodology doc gives a reader the exact wrong answer to the exact question this PR says matters most.The fix is mechanical: replace the stale sentence (lines 87-95) with wording matching the corrected README passage — NCCL EP LOW_LATENCY carries an EP8
ll_backendsrow on all six NVIDIA SKUs, restored by the single-handle fix that removed the NVIDIA/nccl#2303 signal aliasing — and add NCCL EP to the enabled-backends list on line 88.Severity: documentation-only, nothing breaks at runtime, so this is a nit rather than a blocking issue — but it directly contradicts the registry, the tests, and this PR's own corrected README on the specific fact the PR exists to fix, so it's worth cleaning up in the same PR.
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.
Fixed in ce469ba. You were right and it was the same claim in a second place — the README passage I corrected and this one both descended from #2407 changing only
platform_config.json. The Matrix section now reads "NCCL EP EP8 on all six NVIDIA SKUs", notes the rows were dropped while the LL legs wedged on NVIDIA/nccl#2303 and restored once the single-handle adapter removed the aliasing, and states the B300/GB200/GB300 decode gap there rather than leaving it to the README.中文:已在 ce469ba 中修复。你的判断是对的,而且同一处错误描述出现在两个地方 —— 我此前更正的 README 段落与这一处,都源自 #2407 只改了
platform_config.json。Matrix 章节现在写明「NCCL EP EP8 覆盖全部六个 NVIDIA SKU」,说明这些行曾因 LL leg 卡死在 NVIDIA/nccl#2303 而被移除、并在单 handle 版 adapter 消除别名问题后恢复,同时把 B300/GB200/GB300 的解码缺口直接写在该处,而不是留给 README。