[dmvevents]

Fixes #726.

get_nccl_comm_handle's reuse branch trusts backend._comm_ptr() blindly, but PyTorch creates NCCL communicators lazily — _comm_ptr() is a passive accessor that returns 0 when the ProcessGroup+device has no communicator yet (comms materialize at the first collective on that group+device; eager creation only happens when init_process_group received device_id=...).

A consumer that constructs ElasticBuffer before any collective has run on the EP group gets NCCLCommHandle(0), and calculate_elastic_buffer_size reaches get_physical_domain_size → ncclTeamWorld(nullptr) → comm->nRanks null-deref. vLLM's expert-parallel serve path is a live reproduction: deterministic segfault on all 16 ranks (DP16/EP16) at serve init — full chain in #726.

Change: only take the borrowed-comm path when the pointer is non-null; otherwise fall through to the existing create-own-comm path. On a cold group the fallback's all_gather_object is itself a collective on that group, so it lazily materializes torch's comm as a side effect and every rank takes the same branch — no rank divergence on the cold-boot case.

Why the test suite doesn't catch it: tests/utils init_dist passes device_id=... to init_process_group, so torch eager-creates comms and _comm_ptr() is always valid there. The null case is only reachable from lazy-init consumers.

Validated E2E on the vLLM path (Qwen3-30B-A3B-FP8, DP16/EP16, 2 nodes over EFA): with the create-own-comm path taken (behaviorally identical to this guard's fall-through on a cold group), serve comes up and sustains a c=1→64 load sweep with 0 errors.

Update: the guard is now an explicit all-rank-consistent decision — every rank gathers every rank's communicator-nullness before any rank branches, so reuse happens only when all ranks hold a live communicator and no rank can be left behind in a collective. Covered by tests/utils/test_comm_reuse.py.

[ds-review-bot]

🤖 ds-review-bot Code Review

v6

该修改修复了所有 rank 都处于冷启动状态的空指针崩溃，但本地分支决定可能导致部分 rank 提前返回、其余 rank 卡在集合通信中。

v5

该 MR 修复 #726：get_nccl_comm_handle 的重用分支此前无条件信任 backend._comm_ptr()，但 PyTorch 的 NCCL 通信器是惰性创建的——在该 group+device 上未跑过任何集合通信（且 init_process_group 未传 device_id=...）时，_comm_ptr() 返回 0。空句柄会被包装成 NCCLCommHandle(0)，随后在 C++ 侧 get_physical_domain_size → ncclTeamWorld(nullptr) → comm->nRanks 空指针解引用，导致确定性段错误（vLLM EP serve 路径 16/16 rank 复现）。修复方式：仅当 comm_ptr != 0 时才走借用路径，否则落入原有的自建 comm 路径；冷启动时所有 rank 均看到 0 并走同一分支，且 fallback 的 all_gather_object 会顺带物化 torch 的 comm。变更范围小（+10/−2，单文件）、方向正确；新增注释准确解释了惰性语义与崩溃路径，提交信息也说明了测试盲区成因（tests/utils 的 init_dist 传 device_id 使 comm eager 创建，_comm_ptr() 恒非空）。整体建议合入，下方为两条需作者跟进的改进建议。

v4p

该 MR 修复 #726：get_nccl_comm_handle 的复用分支此前无条件信任 backend._comm_ptr()，而 PyTorch 对 NCCL 通信子是惰性创建的——在冷启动（EP group 尚未执行任何 collective）时 _comm_ptr() 返回 0，导致下游 NCCLCommHandle(0) 在 C++ 侧（如 ncclTeamWorld 解引用 comm->nRanks）发生空指针崩溃。变更在复用前增加非空判断，冷启动时回退到既有的自建通信子路径，整体实现正确、改动最小且符合既有代码模式。

Files reviewed: 1
Issues found: 🟡 2 warning | 🔵 1 suggestion
Inline comments posted: 3

[dmvevents]

Pushed e888ef2 (merged current main, so this is 0 behind) adding two test cases, and converted the file to the plain test-function style the rest of tests/ uses (so it runs as python tests/utils/test_comm_reuse.py).

Both gaps were found by mutating the guard rather than by reading it:

mutation	before	after
all(have_comm) → True (remove the guard)	caught	caught
drop the all_gather_object (per-rank decision)	caught	caught
all(...) → any(...)	caught	caught
int(os.getenv('EP_REUSE_NCCL_COMM','1')) → True	not caught	caught

The last row is the reason for this push: the escape hatch had no coverage, because the cases pinned the variable to '1' and never set '0'.

One thing worth flagging about the comment in get_nccl_comm_handle — it says the decision collective "is reached unconditionally", which is true only after a cache miss. The cache check returns above it, so if ranks' cache state ever diverges, cached ranks return while the others block in all_gather_object. That needs a non-uniform caller to happen, so I've pinned it as a documented constraint in test_cache_hit_bypasses_the_decision_collective rather than changing behaviour. Happy to reword the comment if you'd prefer.

yapf 0.40.2 and ruff 0.6.5 from requirements-lint.txt are both clean on this file. Note format.sh still reports a diff on an unmodified tree because deep_ep/utils/comm.py has pre-existing [None, ] * n lines that yapf rewrites — I left those alone to keep this diff test-only. Happy to send that as a separate formatting PR if it's wanted.

One note on the ds-review-bot review above: its footer reads Files reviewed: 1 and +10/−2, and all three of its inline comments are on comm.py — so it ran against the first commit only and hasn't seen a6f4b48, which made the reuse decision group-uniform. Its rank-divergence finding was correct and is fixed there; the three threads are resolved.

For what it's worth on whether the tests earn their place: with main's current deep_ep/utils/comm.py checked out under them they fail with an AssertionError, and with this branch's they pass — so they do distinguish the two rather than passing either way. All four mutations above were re-run on the merged tree, not just on the branch.