Skip to content

[Runtime] Preflight full intranode P2P matrix - #730

Open
0z5a wants to merge 3 commits into
deepseek-ai:mainfrom
0z5a:fix/p2p-preflight
Open

[Runtime] Preflight full intranode P2P matrix#730
0z5a wants to merge 3 commits into
deepseek-ai:mainfrom
0z5a:fix/p2p-preflight

Conversation

@0z5a

@0z5a 0z5a commented Aug 17, 2026

Copy link
Copy Markdown

Fixes #584.

Problem

DeepEP currently reaches CUDA peer setup before validating that every participating intranode GPU can access every other GPU. On partial-P2P hosts this fails on the first unsupported pair, after initialization has already progressed, and does not show the complete incompatible topology.

Changes

  • Gather stable Linux boot IDs and physical GPU UUIDs before buffer construction; CUDA logical ordinals are never exchanged across processes.
  • Map UUIDs into each source process's visible CUDA ordinals, with an NVML READ/WRITE fallback for peers hidden by per-rank CUDA_VISIBLE_DEVICES masks.
  • Query every required directed intranode P2P edge and reject duplicate rank assignments to one physical GPU.
  • Aggregate and deterministically report all unsupported rank/device pairs on every rank.
  • Run the preflight before legacy _C.Buffer allocation and before elastic communicator/buffer setup.
  • Document the full directed-P2P requirement and the alternative-backend guidance.
  • Add host-only regression tests, including the 8-GPU four-pair topology from deep_ep.cpp:200 init fails on partial CUDA peer access; preflight and report unsupported device pairs (cudaErrorPeerAccessUnsupported) #584 (48/56 unsupported directed pairs).

Validation

8-rank failure-path benchmark

Environment: 8x NVIDIA GeForce RTX 5060 Ti, CUDA 13, one NCCL rank per GPU. This PCIe host exposes no off-diagonal CUDA P2P edges, so the real topology is 56/56 unsupported directed pairs. Results use 10 warmups and 100 measured iterations; each iteration reports the slowest rank.

Measurement Result
Full preflight median 1.717 ms
Full preflight p95 / max 2.908 / 3.082 ms
Equivalent two-object-gather baseline median 1.670 ms
Median increment over gather baseline 0.047 ms
Error agreement identical on all 8 ranks
Reported unsupported edges 56 entries, 56/56
Failed 1 GiB legacy buffer probe 0-byte allocated/reserved/free-memory delta on every GPU

The benchmark host validates the real distributed all-unsupported failure path. The exact partial-P2P topology from #584 is covered by the 48/56 regression test.

Scope

This is an initialization-only check. Dispatch and combine hot paths are unchanged.

Comment thread deep_ep/utils/envs.py Outdated
Comment on lines +165 to +167
local_device = torch.cuda.current_device()
rank_devices = _all_gather_object(group, (socket.gethostname(), local_device))
local_access_results = build_local_peer_access_results(rank, rank_devices, torch.cuda.can_device_access_peer)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 critical: 使用源进程可解析的 CUDA 设备标识: 当启动器为各 rank 分别设置 CUDA_VISIBLE_DEVICES 时,current_device() 返回的是进程本地序号;例如两个 rank 都会报告设备 0,随后实际查询的是 can_device_access_peer(0, 0),从而把有效的跨 GPU P2P 拒绝掉。不同的可见设备排序还可能查询错误的物理 GPU;应交换稳定的物理标识并转换为源进程的本地序号,或明确验证所有 rank 使用相同映射。

🤖 v6

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2161508. Ranks now exchange physical GPU UUIDs rather than process-local CUDA ordinals. Each source process maps UUIDs into its own visible ordinal space; if a peer is hidden by CUDA_VISIBLE_DEVICES, the query falls back to NVML READ/WRITE capability checks by UUID. Host-only regressions cover reordered visibility and the one-visible-GPU case, and an RTX 5080 real-CUDA/NCCL check passed with one visible device.

Comment thread deep_ep/utils/envs.py Outdated
# Close NVML
pynvml.nvmlShutdown()
local_device = torch.cuda.current_device()
rank_devices = _all_gather_object(group, (socket.gethostname(), local_device))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 warning: Intranode grouping keys on socket.gethostname(). In containerized deployments, ranks on the same physical host may report distinct hostnames (per-container), silently skipping real intranode edges, while ranks on different hosts may all report the same generic hostname (e.g. 'localhost'), producing false 'unsupported pair' failures for inter-node ranks. Consider a more robust host identity (e.g. host UUID, boot id, or MAC-derived id) or at least documenting this assumption.

🤖 v5

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2161508. Linux kernel boot ID is now the primary physical-node key, so containers on one machine can have different hostnames while different machines remain distinct. Hostname is only a portability fallback when the Linux boot ID is unavailable.

Comment thread deep_ep/utils/envs.py Outdated

# Close NVML
pynvml.nvmlShutdown()
local_device = torch.cuda.current_device()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 suggestion: The preflight uses torch.cuda.current_device(), while the legacy runtime later derives the device from self.runtime.get_local_device_id(). If a caller constructs Buffer before calling torch.cuda.set_device(...), the preflight may validate the wrong device (typically device 0 on every rank). Consider asserting the device has been set, or deriving the device the same way the runtime will.

🤖 v5

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The preflight intentionally uses the current CUDA device because the native Buffer constructor immediately reads the same process state via cudaGetDevice. If the caller never selects the intended device, both paths use device 0. The cross-process ambiguity is addressed separately by exchanging physical UUIDs instead of current_device ordinals.

Comment thread deep_ep/utils/p2p.py Outdated
local_host, local_device = rank_devices[rank]
access_results: List[PeerAccessResult] = []
for peer_rank, (peer_host, peer_device) in enumerate(rank_devices):
if peer_rank != rank and peer_host == local_host:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 suggestion: If two ranks on the same host map to the same CUDA device (GPU oversubscription), cudaDeviceCanAccessPeer(d, d) returns 0, so the pair is reported as an unsupported P2P edge. Failing is correct since DeepEP does not support shared devices, but the 'unsupported directed pairs' message is misleading for this case; detecting duplicate (host, device) tuples and reporting them explicitly would be more actionable.

🤖 v5

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2161508. The gathered (boot ID, GPU UUID) assignments are validated before queries, and duplicate rank assignments now produce an explicit same-physical-GPU error.

Comment thread deep_ep/buffers/legacy.py

# Normal kernels directly access every intranode peer buffer. Validate the
# complete directed P2P matrix before allocating the large IPC buffer.
if num_nvl_bytes > 0:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 suggestion: Pure low-latency mode with allow_nvlink_for_low_latency_mode=True (NVSHMEM_DISABLE_P2P='0') still bypasses the preflight when num_nvl_bytes == 0, even though NVSHMEM may then use P2P transport. If NVSHMEM P2P has the same full-matrix requirement, consider extending the gate to that configuration in a follow-up.

🤖 v5

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this as a follow-up. This PR preflights the direct intranode buffer path guarded by num_nvl_bytes > 0. NVSHMEM owns transport selection in pure low-latency mode, and I do not yet have evidence that enabling its P2P transport requires the same full directed matrix.

Comment thread deep_ep/utils/p2p.py

def format_p2p_preflight_error(unsupported_pairs: Sequence[UnsupportedPeerPair], num_required_pairs: int) -> str:
"""Build one deterministic, actionable error for all unsupported pairs."""
pair_list = ', '.join(f'(rank {src_rank} GPU {src_device} -> rank {dst_rank} GPU {dst_device})'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 suggestion: On large partial-P2P hosts the aggregated error enumerates every unsupported pair in one line (56 entries for 8 GPUs, more for larger domains). Consider capping the enumerated list (e.g. first N pairs plus a total count) or formatting one pair per line to keep logs readable, while keeping the full count deterministic.

🤖 v5

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2161508. The error still reports every pair for an 8-GPU domain (up to 56 directed pairs), but caps larger lists at 64 entries and reports the deterministic omitted count plus the full unsupported/required total.

Comment thread deep_ep/utils/envs.py Outdated
# Close NVML
pynvml.nvmlShutdown()
local_device = torch.cuda.current_device()
rank_devices = _all_gather_object(group, (socket.gethostname(), local_device))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 warning: socket.gethostname()torch.cuda.current_device() 都是进程本地视角,不能保证在同一物理节点的所有 rank 上一致:容器化部署中每个 pod/容器的 hostname 可能不同,导致同一节点的 rank 被当成跨节点而跳过必检的 P2P 边;若各 rank 的 CUDA_VISIBLE_DEVICES 不一致(例如每 rank 只暴露自己的 GPU),上报的 local_device 可能都是 0,can_access_peer(0, 0) 恒为 True,预检会误通过。这样仍会在后续 CUDA peer setup 阶段才失败,违背预检目的。建议使用全局唯一的物理设备标识(如 GPU UUID/PCI bus ID)或基于 NCCL 物理域信息来确定 intranode 拓扑,并在 device ID 无法映射时显式报错。

🤖 v4p

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2161508 by the same physical-identity change: boot ID replaces hostname as the primary node key, GPU UUID replaces process-local device ordinal, UUIDs are mapped in the source process, and hidden peers use NVML READ/WRITE status. Rank-local query failures are also gathered before raising so no rank waits in a later collective.

@ds-review-bot

Copy link
Copy Markdown
Collaborator

🤖 ds-review-bot Code Review

v6

新的预检错误地跨进程复用了 CUDA 逻辑设备序号,会在常见的单 GPU 可见性配置下阻止合法拓扑初始化。

v5

This MR implements a full-matrix intranode CUDA P2P preflight before buffer construction, fixing #584. The change is well-scoped and initialization-only: a new pure control-plane module deep_ep/utils/p2p.py provides three testable functions (build local directed access results, aggregate/validate the full matrix, and format one deterministic error), and check_nvlink_connections in deep_ep/utils/envs.py now gathers (hostname, device) tuples plus per-rank access results via a backend-agnostic all-gather supporting both torch dist groups and MPI-style comms. Preflight placement is correct in both entry points: in legacy.py it is gated on num_nvl_bytes > 0 and runs before _C.Buffer(...), so the large IPC buffer is never allocated on incompatible topologies; in elastic.py it runs before get_nccl_comm_handle, calculate_elastic_buffer_size, and symmetric buffer allocation. Aggregation is deterministic (results are all-gathered, then iterated in rank order), so all ranks raise identical errors. Documentation in README.md and docs/legacy.md now states the full directed P2P requirement and alternative-backend guidance. The 6 host-only regression tests load p2p.py via importlib so they run without the CUDA extension — a good pattern — and cover the #584 8-GPU four-pair topology (verified by inspection: 56 required and 48 unsupported directed pairs with deterministic ordering, first (0,0,2,2), last (7,7,5,5)), inter-node skipping, missing-result validation, and error formatting. Dispatch/combine hot paths are untouched. Overall the implementation matches the description and is mergeable; the comments below are robustness/usability improvements, none blocking.

v4p

本 MR 将 DeepEP 的 P2P 校验前移:在分配 legacy _C.Buffer 和创建 elastic communicator/buffer 之前,收集各 rank 的 hostname 与 CUDA device ID,查询所有有向 intranode cudaDeviceCanAccessPeer 边,聚合并确定性地报告所有不支持的 rank/device pair,同时补充文档和 host-only 回归测试。整体实现清晰、热路径无改动,测试覆盖了 #584 的 48/56 拓扑;主要风险在于节点/设备标识依赖进程本地信息,在部分容器化或异构可见设备部署中可能漏检或误判。

Files reviewed: 7
Issues found: 🔴 1 critical | 🟡 2 warning | 🔵 4 suggestion
Inline comments posted: 7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

deep_ep.cpp:200 init fails on partial CUDA peer access; preflight and report unsupported device pairs (cudaErrorPeerAccessUnsupported)

2 participants