fix: resolve the GIN context count on the single-node path (kNumQPs == 0 corrupts combine) - #4
Open
KeitaW wants to merge 1 commit into
Conversation
On a single node `scaleout_active` is false, and the else arm copied
`num_allocated_qps` verbatim instead of calling `resolve_gin_context_cnt()`.
When the caller leaves that value at its default of 0 and the unordered hybrid
kernels are in use, the Python auto-fill is deliberately skipped so that C++
resolves the count, so a single-node run kept 0 all the way into the kernels:
nccl.cu else arm -> gin_context_cnt = 0
elastic.py get_theoretical_num_qps -> min(num_sms * 16 + 1, 0) = 0
comm.cuh get_qp_mode -> kNumQPs == 1 fast path skipped,
kNumSMs <= kNumAvailableQPs false
qp_mapping.cuh -> balanced_partition(idx, 512, 0) -> n / 0
Nothing rejects q == 0. Compiled for the host that specialization exits on
SIGFPE; in a GPU run it produced corrupted output and no reported fault: the
combine reported 623 MB in 1.148 us, and its output differed from the reference
in 99.98% of elements, surfacing as 'AssertionError: Diff: nan' in
tests/elastic/test_ep.py.
Resolve the count on both branches and write it back unconditionally, since
Python reads it via get_num_allocated_qps() and caps the per-launch QP count
with it. Add a static_assert so an illegal specialization fails to compile
rather than miscomputing, and assert the delegation invariant in Python at the
point the contract is broken.
Xuan-1998
reviewed
Aug 23, 2026
| gin_config = elastic::gin_alloc::make_gin_resources(resolve_gin_context_cnt()); | ||
| } else { | ||
| gin_config.gin_context_cnt = this->num_allocated_qps; | ||
| // Single node: there are no peer rail teams, so no indexed signals are |
There was a problem hiding this comment.
Think we can get rid of the signal here since the code is self explainable
Xuan-1998
reviewed
Aug 23, 2026
|
|
||
| if (scaleout_active) | ||
| this->num_allocated_qps = gin_config.gin_context_cnt; | ||
| // Unconditional: Python reads this back via `get_num_allocated_qps()` and |
|
Thanks @KeitaW, this is great. We will also add this check to our nightly to exercise the intra node path as well |
Xuan-1998
requested changes
Aug 23, 2026
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
On a single node the GIN context count is never resolved, so
kNumQPs == 0reaches the combine kernel andbalanced_partition()is called withq == 0. Nothing rejects it, and the run does not fail: it produces a wrong result, reported bytests/elastic/test_ep.pyasAssertionError: Diff: nan.Two nodes are unaffected, which is why this does not show up in multi-node testing. "Single node" here still means eight local ranks, so the device-communicator setup is entered normally; what differs is that
ncclTeamRail(comm).nRanksis 1.The chain
All at
main@02efc268.deep_ep/buffers/elastic.py:350-358, automatic QP count. When the caller leavesnum_allocated_qpsat its default of 0 and the unordered hybrid kernels are in use, the Python auto-fill is deliberately skipped, and the comment says why: the count is resolved in C++ from the GIN signal budget. In that configuration 0 is handed to C++ on purpose.csrc/kernels/backend/nccl.cu:115-138.resolve_gin_context_cnt()is the resolver Python delegates to, and it is only called on thescaleout_activebranch. The single-node branch copies the sentinel verbatim:The range check that would have rejected 0 (
ctx >= kMinGinContextCnt, minimum 2) lives inside that lambda, so it is not on this path.elastic.py:862-879,get_theoretical_num_qps().min(num_sms * 16 + 1, self.num_allocated_qps)becomesmin(1025, 0) = 0, and that 0 becomes thekNumQPstemplate parameter of the combine kernel. Because it is a template parameter, the branch is selected at JIT compile time.common/comm.cuh:77-99andcommon/qp_mapping.cuh:47-57,62-88.get_qp_modespecial-caseskNumQPs == 1only. With 0,kNumAvailableQPsis 0,kNumSMs <= kNumAvailableQPsis false, and control reachesbalanced_partition(global_channel_idx, 512, 0), whoseconst int base = n / q;sits directly under the comment "Callers guaranteeq >= 1".Compiling the shipped
qp_mapping.cuhon the host atkNumSMs=64, kNumQPs=0exits 136 (SIGFPE), which confirms the branch is reachable with the real parameters. In the GPU runs below the same specialization produced corrupted output and no reported fault.Why the symptom is misleading
The assertion message says NaN, but in an instrumented single-node run (eight ranks, unpatched) the combine output differs from the reference in 469,453,194 of 469,561,344 elements, 99.977%, while only 8,300 elements, 0.0018%, are actually NaN. The inputs are NaN-free, rows containing no NaNs still differ, and sampled bad rows carry thousands of distinct bit patterns.
The clearest signal is the timing: with
#QPs: 0/0the combine reports 622,881,536 bytes in 1.148 us, which the harness prints as 542,580 GB/s. That timing is not compatible with the kernel having completed the stated payload.calc_diffthen returns NaN because some leftover bit patterns decode as NaN, which is how a resource-indexing problem ends up presenting as a numerics one.The change
nccl.cu: callresolve_gin_context_cnt()on the single-node branch too, and make the write-back unconditional. Python reads the value back throughget_num_allocated_qps()and caps the per-launch QP count with it, so a path that does not write back leaves the Python side at 0.gin_indexed_signals_cnt = 0is unchanged; a single node has no peer rail teams.comm.cuh,qp_mapping.cuh:static_assert(kNumQPs >= 1, ...)inget_qp_mode,channel_to_qpandchannel_to_signal_id, so an illegal specialization fails to compile instead of miscomputing. I deliberately did not widen the fast path tokNumQPs <= 1: 0 means no QP was allocated, and running it as if one existed would hide a resource-allocation problem rather than surface it.elastic.py: assert the delegation invariant immediately afterget_num_allocated_qps(), where it is observable. With the C++ change in place this never fires; it exists so a future branch that skips resolution fails at the handoff rather than downstream in a kernel. The existingassert num_qps <= self.num_allocated_qpsguards pass vacuously when both sides are 0.Verification
p6-b200.48xlarge, EFA kernel driver 3.3.0g, CUDA 13.1.2, NCCL v2.31.2-1, aws-ofi-nccl v1.21.1, EFA installer 1.50.0, torch 2.11.0+cu130. Predictions were written before the runs.
The two builds are the same container image differing only in the DeepEP tree installed into it: base
02efc268versus this branch. They ran on different but matched nodes of the same instance type in the same cluster, with identical launch arguments and environment, so this is a controlled comparison rather than a strict single-variable experiment. One run per configuration, so the figures below carry no uncertainty estimate; ranges are across all ranks.#QPs0/0Diff: nan11/1111/1111/11The two-node arms are the regression check, since the unconditional write-back is on a line the scale-out path also executes. Result: no correctness regression, the QP count is unchanged at 11, and the dispatch ranges overlap. With one run per build I would put that as "no observed regression", not as proof of identical performance.
A third single-node arm confirms the two problems stay separate: with this patch applied but a launcher environment that sets neither
NCCL_GIN_TYPEnorNCCL_IB_DISABLE, the run still fails earlier, atnccl.cu:189, withRequested properties for GIN GDAKI NIC 6, only 2 GIN GDAKI NICs have been created. The passing arms above set both.Compile-side checks on the guard:
kNumQPsqp_mapping.cuh, hostg++comm.cuh,nvcc -arch=sm_100with the JIT flags1 is the existing fast path, 2 and 17 are
kMinGinContextCntandkMaxGinContextCnt, 11 iskDefaultGinContextCnt, and 1025 is the hybridnum_sms * 16 + 1at 64 SMs. The negative sentinelkFlushAllAllocatedQPs = -1is used only for the barrier instantiations inimpls/barrier.cuhand is converted to a runtime value ingin_barrier_wo_local_sync, so it never reaches these templates. The full tree builds and installs with the change (TORCH_CUDA_ARCH_LIST="9.0;10.0"); note that the AOT build does not instantiate the guarded templates, which live in the JIT translation unit, so the GPU runs above are what exercises them.Two things I did not do
No repository test is added. The natural regression path,
tests/elastic/test_ep.py, requires GPUs and distributed initialization, and it now passes on a single node with this change. I did not want to invent a new test shape without knowing your CI. If you would like one, tell me where it should live and I will add it.No allocation-cost measurement for the default. The single-node branch now goes through
resolve_gin_context_cnt(), so an unspecified count becomeskDefaultGinContextCnt = 11. A single node uses no RDMA, so those contexts are allocated and unused. I kept the default because it follows the existing resolver policy rather than introducing a second constant, butkMinGinContextCnt = 2is a reasonable alternative if the footprint matters. On the unmodified library,2/2and8/17performed the same in a single run each, so I have no evidence either way. Happy to change it.