Repository navigation
Conversation
jhcdlg
requested review from
ch-wan,
merrymercy and
yizhang2077
as code owners
September 20, 2026 18:01
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
MultimemAllGathererdecides whether to use the multimem path from node topology only:A host can be single-node and still have no multicast fabric: two consumer GPUs on PCIe
with no NVLink and no P2P are co-located, so
nnodes == 1holds and the multimem path staysenabled. On our box that ends in SIGFPE (exit code -8) inside
symm_mem.rendezvous()- called fromcreate_state(), i.e. before the existingmulticast_ptr == 0fallback in_build()ever runs. The server starts normally and thefirst real inference request kills the scheduler.
This PR keeps the existing condition untouched and adds the capability condition it is
missing.
What changed
_local_gpus_have_peer_access()asks the driver whether the local GPUs can reach each other(
torch.cuda.can_device_access_peer, all ordered pairs). Notes on the design:negative answer falls back to NCCL, which is always correct; a positive answer leaves the
existing decision unchanged.
gpu_p2p_access_check()/can_p2p(): those spawn helperprocesses, which is too expensive to run whenever a gatherer is constructed, and they are
built around a rank->device mapping that a single rank process cannot read directly.
True- i.e. the previousbehaviour is preserved rather than silently changing which path is taken.
How it was verified
CPU only - no GPU, no NCCL, no rendezvous involved.
__init__reads the parallel state(
get_tp_group(),get_parallel().nnodes,in_the_same_node_as()) and the driver's topology(
torch.cuda.device_count,torch.cuda.can_device_access_peer), so all five are faked and_stateis asserted directly (None= NCCL,_UNINIT= multimem stays enabled).Five cases, each run against (a) unpatched code and (b) this patch:
enabled=False-> NCCLworld_size == 1-> stays lazyThe single failing case on unpatched code is the bug this PR fixes; it doubles as proof that
the test is not vacuous. The test file is included in this PR as
test/registered/unit/distributed/test_triton_symm_mem_ag.py(it mirrorspython/sglang/srt/distributed/device_communicators/triton_symm_mem_ag.py), registers itselffor CPU CI via
register_cpu_ci(est_time=5, suite="base-a-test-cpu")and subclassesCustomTestCase, followingtest/registered/unit/README.md.The A/B output below was produced by running this exact test file - first against the
unpatched source file, then against the patched one. Baselines, so the reader can check them:
unpatched
552lines / md5c89b4103acabdc9c88119cecbf575af0; patched588lines /md5
4fe0be55fd15291646e1e8040969391f, which is the unpatched file plus the patch in this PR(
+36 / -0lines). The revision these two files were taken from is upstreammain@80da4432.No GPU was involved: the driver's capability query is mocked, which is
precisely the point of the test.
Scope / limits we want to be upfront about
NVLink, no P2P, WSL2 - same machine, same driver, SGLang revision
4cb5aebruns fine, whichis how we know it is not a hardware fault.
multimem are unaffected. By construction they should be: the new condition only ever adds a
reason to fall back. Worst case on an exotic host is a wrong "no P2P" verdict and a fallback
to NCCL - slower, still correct. If you would prefer a different capability signal, we are
happy to switch.
torch.cuda.can_device_access_peeris the driver's own report and can be wrong in theoptimistic direction (that is why
gpu_p2p_access_checkexists). This patch only trusts anegative answer, so an optimistic false report just leaves today's behaviour.
multicast_ptr == 0fallback in_build(). That check is on thewrong side of
rendezvousto help here; fixing it is a larger, separate change.Related
in_the_same_node_as()segfault under EP/mooncake setups). Our case is compatible with thatintent - the point is narrower: "a single node" does not imply "multicast-capable", so
the guard lacks a capability probe. This PR adds one without undoing [Fix] Skip cross-node probe in MultimemAllGatherer on single-node runs (fixes mooncake EP segfault) #30139.
SGLANG_DISABLE_MULTIMEM_AG, merged) added a manual opt-out that forces theNCCL all-gather path, i.e. upstream has already accepted that some hosts need a way out of
multimem. This PR makes that decision automatic for the case the capability probe can
detect, instead of requiring every affected user to find and set an environment variable.
(While reading today's
mainI could not findSGLANG_DISABLE_MULTIMEM_AGinenviron.pyor
triton_symm_mem_ag.pyany more - was it moved, or was that change based on a differentbranch? Not trying to make a point, just checking whether the env var is still the intended
escape hatch before we rely on it.)
this thread, or open a separate issue if you would rather keep reports out of the PR.
A/B verification output (CPU only: unpatched 4/5 -> patched 5/5)
CI States
Latest PR Test (Base): ❌ Run #35527701759
Latest PR Test (Extra): ❌ Run #35527701553
Latest PR Test (AMD ROCm 10): ❌ Run #35527701721