feat(deepep-efa): NeMo-RL GRPO + Megatron Shape-Y MoE all-to-all over EFA (DeepEP-V2 NCCL-GIN) - #1242
feat(deepep-efa): NeMo-RL GRPO + Megatron Shape-Y MoE all-to-all over EFA (DeepEP-V2 NCCL-GIN)#1242dmvevents wants to merge 18 commits into
Conversation
… EFA (DeepEP-V2 NCCL-GIN) - new test case 3.test_cases/pytorch/nemo-rl/deepep-v2-efa (mirrors slime sibling shape) - NGC-from-scratch Dockerfile, DeepEP-V2/NCCL-Gin/EFA from public source; opt-in default-OFF draft-PR layer - recipe verify+rollout-probe+train-step, RayCluster 2-node manifest, engine-index README - honest measured (Wave-28 2xp5 H100) vs staged (rc image build-staged, draft-PR-dependent) Signed-off-by: Anton Alexander <dmvevents@gmail.com>
KeitaW
left a comment
KeitaW
left a comment
There was a problem hiding this comment.
Review Batch 1/6 — The image does not build as pinned
This is a big, careful, unusually honest contribution — the measured-vs-staged framing, the
fail-loud gates and the pins-with-reasons table are all above the bar for this repo, and the
positives section at the end is specific about what I verified rather than generic. It is also
1,717 lines across 15 files, which is past the point where a single reading reliably finds
everything, so I reviewed it in file-group passes (image build / scripts / manifests / docs).
The headline is Batch 1: as pinned, the image does not build. Everything after that is written
assuming Batch 1 gets resolved, because the fix there (which NeMo-RL revision, which base) changes
what several other findings should say.
Three findings on the NeMo-RL install layer, in dependency order. The first two are hard blockers
on both flavors; the third is a claim I could not fully close from the diff.
| # (matching the slime sibling's runtime layout) so the opt-in patch layer's | ||
| # in-place edits are exactly what executes. | ||
| COPY requirements.txt /tmp/requirements.txt | ||
| RUN python3 -c 'import sys; assert sys.version_info >= (3, 12), f"NeMo-RL needs python>=3.12, base has {sys.version}"' \ |
There was a problem hiding this comment.
The pinned NeMo-RL requires Python >= 3.13.13; the NGC base is 3.12, so the default build fails here
Observation. This guard asserts sys.version_info >= (3, 12) and passes on the NGC base
(nvcr.io/nvidia/pytorch:26.02-py3 ships Python 3.12 — its own LD_LIBRARY_PATH points at
/usr/local/lib/python3.12/dist-packages/torch/lib). But NeMo-RL at the pinned
NEMO_RL_SHA=cc75cad declares:
requires-python = ">=3.13.13"(pyproject.toml at cc75cad,
verified live 2026-08-25.) pip enforces Requires-Python independently of --no-deps — that flag
suppresses dependency resolution, not the interpreter check — so the very next line,
pip3 install --no-cache-dir --no-deps -e /opt/NeMo-RL, ends in
ERROR: Package 'nemo-rl' requires a different Python: 3.12.x not in '>=3.13.13'.
Impact. Layer 7 fails on both flavors, baseline and --build-arg APPLY_DRAFT_ROLLOUT_PATCHES=1, so no image is produced and none of the recipe gates can run. This
is upstream of every other finding in this review. It also means the README's "build-staged" status
can't be describing this exact tree — worth reconciling, because the pin combination that was
built is the interesting information here.
Suggestion. Two coherent ways out — the choice matters because it drives the whole pins table:
- Move
NEMO_RL_SHAback to the last NeMo-RL revision whoserequires-pythonadmits 3.12
(that revision's dependency set is also whatrequirements.txtshould be regenerated from — see
the next comment); or - Move the base to an NGC tag shipping Python >= 3.13.13, and re-check the TransformerEngine /
apex / flash-attn ABI story that motivated this base in the first place.
Either way the guard should assert the real floor rather than 3.12, so a future base bump fails
here with the reason instead of inside pip:
| RUN python3 -c 'import sys; assert sys.version_info >= (3, 12), f"NeMo-RL needs python>=3.12, base has {sys.version}"' \ | |
| RUN python3 -c 'import sys; assert sys.version_info >= (3, 13, 13), f"NeMo-RL {cc75cad} requires-python is >=3.13.13, base has {sys.version}"' \ |
There was a problem hiding this comment.
Fixed in 6c14c2e. Root cause was NEMO_RL_SHA=cc75cad (the #2411 base), which bumps requires-python to >=3.13.13; the NGC base ships Python 3.12, so pip install -e hard-failed the interpreter floor (--no-deps does not suppress the Requires-Python check). Re-pinned to 46be4e8 (the #2410 base), which declares >=3.12 and matches the base. The line-199 assert now guards the intended floor sys.version_info >= (3, 12) and passes on the py3.12 base.
| # (no NVSHMEM linked at all) | ||
| # When bumping NEMO_RL_SHA, re-diff this list against the new pyproject. | ||
| colored==2.2.3 | ||
| ray[default]==2.49.2 |
There was a problem hiding this comment.
requirements.txt was generated from a different NeMo-RL revision than NEMO_RL_SHA
Observation. This file's header states it carries NeMo-RL's dependency set "at the same
specifiers the pinned SHA declares." Diffing it against
pyproject.toml at cc75cad
(verified live 2026-08-25), every version-bearing line disagrees:
| Package | pinned cc75cad declares |
this file pins |
|---|---|---|
torch |
2.10.0 |
— (header says the pin is 2.9.0) |
ray[default] |
2.54.0 |
2.49.2 |
transformers |
5.3.0 |
4.57.1 |
pillow |
>=12.1.1 |
>=11.3.0 |
mlflow |
>=3.11.1 |
>=3.5.0,<3.6.0 |
torchvision |
0.25.0 |
— (excluded as NGC-baked) |
and four dependencies the pinned revision declares are absent entirely: decord2,
soundfile>=0.13.1, nccl4py, cuda-bindings. The transformers gap is a major version.
Impact. Because Layer 7 installs NeMo-RL with --no-deps, pip cannot correct any of this — the
image would carry a dependency graph NeMo-RL never declared, and import nemo_rl.algorithms.grpo
(which verify-image.sh gates on) is where a transformers 4→5 API break would surface. The
torch==2.9.0 figure also appears in the README as integration trap 1 and in the Dockerfile
comment, so the wrong number is stated in three places.
Suggestion. Regenerate this list from whichever NeMo-RL revision the previous comment settles
on, rather than editing individual pins — and add pip check as a build gate so the next
NEMO_RL_SHA bump fails loudly instead of drifting again.
There was a problem hiding this comment.
Fixed in f30b929. The header now states the pins are generated from NEMO_RL_SHA=46be4e8 (torch==2.9.0 etc.), matching the Dockerfile pin line-for-line. The earlier mismatch was because the header text referenced a different revision than the one actually built.
| # with this test case's substrate: | ||
| # - torch, triton, torchvision : NGC-baked (the ABI anchor; never reinstall) | ||
| # - setuptools, pip, ninja : present in the base | ||
| # - nvidia-nvshmem-cu12 : cu12 wheel on a cu13 base, and unused here — |
There was a problem hiding this comment.
DeepEP at this pin links NVSHMEM unconditionally — "no NVSHMEM linked at all" isn't true of 01dc3aa
Observation. This comment drops nvidia-nvshmem-cu12 on the grounds that it is "unused here —
this image's deep_ep is the NCCL-GIN backend (no NVSHMEM linked at all)", and
setup_nemo_rl_deepep_efa.sh:15-17 repeats it ("links NO NVSHMEM"). At the pinned DEEPEP_SHA,
setup.py disagrees — with a TODO acknowledging exactly this:
# TODO: make NVSHMEM and legacy optional
nvshmem_root_dir = find_pkgs.find_nvshmem_root()and then, unconditionally, sources.extend([... 'csrc/kernels/backend/nvshmem.cu']),
-lnvshmem_device, -l:libnvshmem_host…, plus an rpath
(setup.py at 01dc3aa).
find_nvshmem_root() defaults to optional=False, so with no EP_NVSHMEM_ROOT_DIR / NVSHMEM_DIR
in the environment and no nvidia-nvshmem* distribution installed, it raises
AssertionError: Cannot find package: nvshmem.
Impact — two branches, and I can't tell which from the diff. The NGC base advertises
NVSHMEM_VERSION=3.4.5 but exports no NVSHMEM_DIR, so it turns on whether that NVSHMEM is
installed as a pip distribution in the base. If it is not, Layer 9's DeepEP build dies on the
assert. If it is, the build succeeds and the image does link NVSHMEM — which makes the
"no NVSHMEM" claim wrong in the docs and gives the extension an undocumented runtime dependency
that nothing verifies.
Suggestion. Whichever branch holds, make it explicit rather than incidental: either set
EP_NVSHMEM_ROOT_DIR in Layer 9 and say the image links NVSHMEM's legacy backend (dead code on
this path but present), or patch the legacy/NVSHMEM sources out of the build and keep the claim.
A ldd assertion in verify-image.sh on the built deep_ep extension would pin down which one
shipped.
There was a problem hiding this comment.
Fixed — the "no NVSHMEM linked at all" claim is corrected across the folder; the branch is now explicit. requirements.txt:24-34 states NVSHMEM IS a BUILD-TIME link dependency (upstream setup.py links it unconditionally at 01dc3aa, main HEAD, and the amazon-contributing fork, via find_nvshmem_root(optional=False)), and adds nvidia-nvshmem-cu13 (line 67) — because the NGC base's baked NVSHMEM is the dpkg libnvshmem3-cuda-13 RUNTIME package with no nvshmem.h and no libnvshmem_device.a, so Layer 9 would otherwise fail at link. setup_nemo_rl_deepep_efa.sh:24-27 carries the matching note ("deep_ep's _C.so IS build-linked against NVSHMEM... NVSHMEM is simply NOT the run-time transport on the V2 NCCL-GIN" path). I also just fixed the last residual you'd have caught — env_vars.example:99 said "This image links NO NVSHMEM", now reworded to the build-time-vs-run-time distinction (commit 5bd1da8). You called the branch exactly: it's the "installed as a pip distribution → build succeeds and DOES link NVSHMEM" case, now documented rather than incidental. Thanks — this was the most important one.
KeitaW
left a comment
KeitaW
left a comment
There was a problem hiding this comment.
Review Batch 2/6 — DeepEP V2 substrate: divergence from the merged benchmark
This folder re-cuts the DeepEP-V2 / NCCL-GIN / EFA stack from scratch one day after
micro-benchmarks/expert-parallelism/deepep-v2-benchmark/ landed on main (PR #1234, merged
2026-08-24 — this branch's merge-base is that exact commit, 9baf5aa7). The two stacks differ on
every axis, and two docs in this diff state that they are the same. The rest of the batch is
inline.
| dispatch/combine benchmarks over EFA — including a DeepEP V2 benchmark on the same NCCL-GIN | ||
| substrate this test case uses — see |
There was a problem hiding this comment.
"the same NCCL-GIN substrate" — the merged DeepEP V2 benchmark is a different substrate (please reword, or converge)
Observation. This line points a reader at micro-benchmarks/expert-parallelism as "a DeepEP V2
benchmark on the same NCCL-GIN substrate this test case uses." Comparing the two as merged:
deepep-v2-benchmark (main) |
this folder | |
|---|---|---|
| GIN backend | NCCL_GIN_TYPE=5 (EFA-GDA, GPU-initiated) |
NCCL_GIN_TYPE=2 (CPU-proxy) |
| NCCL | v2.31.2-1 |
v2.30.4-1 |
| EFA installer | 1.50.0 (libfabric ≥ 2.5 asserted at build) |
1.48.0 |
| aws-ofi-nccl | v1.21.1 release; asserts ncclGinPlugin_v14 |
9c44d34 + cherry-pick of aws/aws-ofi-nccl#1351 |
| DeepEP source | amazon-contributing/DeepEP @ main |
deepseek-ai/DeepEP @ 01dc3aa |
| Build script | setup_deepep_gin.sh |
bespoke setup_nemo_rl_deepep_efa.sh deepep |
The transports are not interchangeable at this NCCL pin. In src/include/nccl_device/core.h,
v2.30.4-1 defines NCCL_GIN_TYPE_NONE=0, PROXY=2, GDAKI=3; v2.31.2-1 adds GPI=4 and
EFA_GDA=5. So the mode the merged benchmark measures does not exist in the NCCL this image
builds — the CPU-proxy path here is structural, not a tuning choice (EFA 1.48.0's libfabric 2.4
locks it in a second time).
To be fair to the choice: the benchmark's own README legitimises CPU-proxy — "DeepEP's kernels
can also run over the CPU-proxy GIN backend, which has no such floor" — so its NCCL ≥ 2.31 /
EFA ≥ 1.50 floors are EFA-GDA floors and nothing here violates them. The issue is only that the
divergence is asserted as sameness rather than stated.
Impact. A reader who takes this sentence at face value will expect the benchmark's numbers and
env (NCCL_GIN_TYPE=5, EP_NCCL_ROOT_DIR, LD_PRELOAD) to transfer to this image, and they don't.
Suggestion. Either reword to name the difference, or converge (see the next two comments).
| dispatch/combine benchmarks over EFA — including a DeepEP V2 benchmark on the same NCCL-GIN | |
| substrate this test case uses — see | |
| dispatch/combine benchmarks over EFA — including a DeepEP V2 benchmark on the EFA-GDA NCCL-GIN | |
| backend (this test case runs the CPU-proxy one) — see |
There was a problem hiding this comment.
Fixed — the "same NCCL-GIN substrate" sameness claim is gone; the index now names the difference. The parent examples/training/nemo-rl/README.md:42 describes this folder as "DeepEP V2's NCCL-GIN CPU-proxy on AWS EFA", and the folder README (README.md:250) states explicitly that the merged micro-benchmarks/expert-parallelism benchmark "runs DeepEP V2 on the EFA-GDA NCCL-GIN backend (NCCL_GIN_TYPE=5), not this" folder's CPU-proxy one. Your table (2.30.4/type-2/CPU-proxy vs 2.31.2/type-5/EFA-GDA) was exactly right — the transports aren't interchangeable at this NCCL pin, and that's now stated rather than asserted as sameness. Thanks.
| p5.48xlarge, rebuild Layer 4 with the define raised (documented one-liner in the issue) — not | ||
| baked here because it is a non-upstream one-off. | ||
| - **No performance numbers.** Dispatch/combine latency and GRPO throughput on this substrate are | ||
| future work; for kernel-level EP benchmarks on the same NCCL-GIN substrate see |
There was a problem hiding this comment.
The same "same substrate" claim in the folder README (sibling of the above)
Same wording, same correction — flagging it separately so both land. This one is doing more work
than the index copy: it sits under "No performance numbers" and effectively tells the reader that
the merged benchmark's figures characterise this stack. They characterise EFA-GDA on NCCL 2.31.
| future work; for kernel-level EP benchmarks on the same NCCL-GIN substrate see | |
| future work; for kernel-level EP benchmarks over EFA (on the EFA-GDA GIN backend, not this | |
| folder's CPU-proxy one) see |
There was a problem hiding this comment.
Fixed — same correction applied in the folder README's "No performance numbers" section. README.md:250 now reads that the merged benchmark's figures characterise the EFA-GDA backend (NCCL_GIN_TYPE=5), "not this" folder's CPU-proxy stack, so a reader no longer infers those numbers transfer here. You were right that this copy was doing more work than the index line — it sat under the perf section and implied the benchmark's figures described this stack. Thanks for flagging both so they'd both land.
| # compiled against the GIN-capable NCCL under $NCCL_HOME. Run AFTER | ||
| # the optional draft-PR patch layer so patched kernels compile in. | ||
| # | ||
| # Distinct from the NVSHMEM-path setup_deepep_efa.sh (vendor-synced via |
There was a problem hiding this comment.
The no-vendor-sync rationale answers about the V1 script; the V2 canonical is setup_deepep_gin.sh
Observation. This header explains why the script is not vendor-synced by contrasting it with
setup_deepep_efa.sh — the V1 / NVSHMEM script that
deepep-vendor-sync.yml
guards. That's the wrong comparand. The script this one parallels is
micro-benchmarks/expert-parallelism/deepep-v2-benchmark/setup_deepep_gin.sh —
the V2 / NCCL-GIN builder — which this PR never mentions.
Impact. The repo now has two V2 DeepEP builders whose only stated relationship is to a third,
unrelated script. setup_deepep_gin.sh is transport-agnostic at build time (it validates
nccl_device.h and builds; it does not care whether you later run GIN type 2 or 5), and it already
supports exactly this folder's need — --deepep-src /opt/DeepEP to build the possibly-patched tree
in place, plus --nccl-root $NCCL_HOME. I checked its preconditions against your pinned tree at
01dc3aa: csrc/kernels/backend/nccl.cu is present (its V2 marker) and setup.py reads
EP_NCCL_ROOT_DIR, so it would accept this tree as-is.
One genuine adaptation is needed and worth naming: the canonical runs
pip install --no-build-isolation . while you need --no-deps so deep_ep's metadata cannot pull
a second torch/NCCL into the NGC image. That reads like a flag to add upstream, not a reason to
fork the script.
Suggestion. Reuse setup_deepep_gin.sh for the deepep phase (keeping your ofi phase
here), or — if the bespoke builder stays — retarget this comment at the script it actually
parallels and say why it isn't reused.
There was a problem hiding this comment.
Fixed — the no-vendor-sync rationale is retargeted at the correct comparand. setup_nemo_rl_deepep_efa.sh:17 now names micro-benchmarks/expert-parallelism/deepep-v2-benchmark/setup_deepep_gin.sh as the V2/NCCL-GIN builder this script parallels, and line 20 states it compiles "the same amazon-contributing/DeepEP fork the canonical setup_deepep_gin.sh clones". Lines 29-31 clarify that only the V1 NVSHMEM-path setup_deepep_efa.sh copies are vendor-synced (so the canonical setup_deepep_gin.sh is not in that sync either), which is the accurate framing of why this script isn't guarded by deepep-vendor-sync.yml. You were right that contrasting against the V1/NVSHMEM script was the wrong comparand. We kept the bespoke builder (it needs --no-deps so deep_ep's metadata can't pull a second torch/NCCL into the NGC image) rather than reusing the canonical, and the comment now says so. Thanks.
| # ElasticBuffer + NCCL backend, merged upstream in PR#605) and is the EXACT | ||
| # base of draft PR deepseek-ai/DeepEP#612, so the opt-in layer applies | ||
| # --check-clean. NOT the amazon-contributing fork: baseline is stock upstream. | ||
| ARG DEEPEP_SHA=01dc3aaac82068020353dce2c302e38153c0bfaa |
There was a problem hiding this comment.
Pinning stock upstream at exactly the AWS fork point forfeits the fixes for the traps you document
Observation. amazon-contributing/DeepEP main is 01dc3aa plus eight AWS commits — so
DEEPEP_SHA=01dc3aa is precisely the fork point with none of the delta. The Dockerfile's stated
reason ("the EXACT base of draft PR #612, so the opt-in layer applies --check-clean") holds
mechanically, but the fork already addresses both trap-2 failure modes, structurally rather than by
clamping:
| Failure mode | this PR, via draft #612 | fork |
|---|---|---|
get_rdma_gbs() reads 0 on EFA — at 01dc3aa it shells out to ibstat, which does not enumerate EFA's rdmap*s0 HCAs (deep_ep/utils/envs.py:258) |
hardcoded EP_EFA_RDMA_GBS=25.0 fallback |
_get_sysfs_rdma_gbs() reads /sys/class/infiniband/<nic>/ports/*/rate, provider-agnostic, with device auto-discovery (78ae4e5c) |
| auto-QP overruns aws-ofi-nccl's 128-slot GIN ring | clamp num_allocated_qps → EP_EFA_MAX_QPS=2 |
GIN QP allocation restructured in C++ — gin_resource_alloc.cuh (+170), qp_mapping.cuh (+123), proxy_ring.cuh (+89) — plus 8e7b42e9 fixing the divide-by-zero in that resolution path |
The performance envelopes differ materially: EP_EFA_MAX_QPS=2 against the fork's 11 QPs per GIN
context on the unordered path.
In fairness on timing: most of those fork commits are dated 2026-08-21/25 — days old, some
same-day as this PR — so "not yet visible" is the likely story rather than a considered rejection.
But the freeze-at-the-fork-point rationale is self-defeating either way: it holds the tree back to
keep a draft patch applying cleanly, at the cost of the merged fixes that supersede that draft.
Suggestion. Move DEEPEP_SHA to amazon-contributing/DeepEP and drop #612 from the opt-in
layer as superseded. If the CPU-proxy Wave-28 lineage must be preserved exactly, say so here and in
the pins table so the next reader knows the fork was considered.
There was a problem hiding this comment.
This is intentional, and 72d66ad documents why. DEEPEP_SHA=01dc3aa is stock upstream deepseek-ai/DeepEP main, chosen deliberately: (1) it is the exact --check-clean base of draft PR deepseek-ai/DeepEP#612 in the opt-in patches/ layer, and (2) it keeps this folder to the no-fork-base convention the sibling samples follow. You are right that the amazon-contributing fork (01dc3aa + ~8 AWS commits) fixes the trap-2 failure modes structurally; on baseline we instead clamp the same behavior via the inert ENV defaults (or #612 when opted in). Adopting the fork's structural fixes is a tracked future re-pin + re-measure (README trap 2 + the pins table call this out), not a silent freeze. Happy to switch the baseline to the fork if you would prefer the structural fix be the default here.
KeitaW
left a comment
KeitaW
left a comment
There was a problem hiding this comment.
Review Batch 3/6 — Pins, provenance, and knobs that do nothing
The pins table justifies every entry, which is exactly why the rows below are worth tightening
rather than waving through. All inline.
| | Base image | `nvcr.io/nvidia/pytorch:26.02-py3` | Same NGC base as the slime RL sibling. Bakes torch 2.11/CUDA 13 with TransformerEngine/apex/flash-attn compiled against that exact ABI — what megatron.core's H100 path needs. | | ||
| | EFA installer | 1.48.0 | The userspace of the measured NCCL-GIN substrate. Bumping is a re-measure event. | | ||
| | NCCL | `v2.30.4-1` (source build) | GIN device API generation (`nccl_device.h` asserted at build). Same NCCL line the NGC base bakes — the ld.so.conf override introduces no drift; source-built so DeepEP has one controlled header/lib root. | | ||
| | aws-ofi-nccl | commit `9c44d34` + PR#1351 head `c2e773d` | The GIN CPU-proxy plugin pins of the measured substrate (same as the TensorRT-LLM NcclEP sibling). Immutable SHAs — `refs/pull/N/head` is a moving ref. | |
There was a problem hiding this comment.
Two aws-ofi-nccl pins described as "the measured substrate" postdate the measurement
Observation. The measured run is dated 2026-05-06 ("Wave 28"). Checking each pin's commit date:
| Pin | Commit date | vs 2026-05-06 |
|---|---|---|
NCCL v2.30.4-1 |
2026-04-22 | predates ✓ |
NeMo-RL cc75cad |
2026-05-05 | predates ✓ |
aws-ofi-nccl 9c44d34 |
2026-05-25 | +19 days |
aws-ofi-nccl c2e773d (#1351 head) |
2026-08-13 | +99 days |
(verified live 2026-08-25 via the GitHub commit API.)
Impact. Neither aws-ofi-nccl SHA can be "the pins of the measured substrate," and the
"bumping it is a re-measure event" freeze rests on that description. A future maintainer told these
are frozen-because-measured will hesitate to bump pins that were never in the measured run. Note
the DeepEP and Megatron rows do not have this problem — they're justified as draft-PR bases, not
as measured, which is the right shape.
Suggestion. Re-describe this row as "the GIN CPU-proxy plugin lineage this folder standardises
on" (or give the plugin SHA the run actually used), and keep the measured-substrate wording only
for rows where it's true.
There was a problem hiding this comment.
Fixed — the aws-ofi-nccl row is no longer described as "the measured substrate". README.md:52 now reads: "The GIN CPU-proxy plugin lineage this folder standardises on (same pins as the TensorRT-LLM NcclEP sibling). These SHAs postdate the Wave-28 run, so they are the standardised lineage, not that run's exact pins." Your dated-commit table was the decisive evidence — 9c44d34 (2026-05-25) and c2e773d (#1351 head, 2026-08-13) both postdate the 2026-05-06 run, so calling them measured-substrate pins was wrong, and the freeze rationale is re-based on "standardised lineage" instead. Thanks.
| # sibling sample). Immutable SHAs — refs/pull/N/head is a moving ref. | ||
| ARG AWS_OFI_NCCL_SHA=9c44d34476f90ddbf4a12d0ac4fc412d46bd8ab4 | ||
| ARG AWS_OFI_NCCL_PR=1351 | ||
| ARG AWS_OFI_NCCL_PR_SHA=c2e773dfb2c75b765b3415f8ffd1b47e7c239a7b |
There was a problem hiding this comment.
The baseline image unconditionally cherry-picks a closed, unmerged upstream PR
Observation. AWS_OFI_NCCL_PR_SHA defaults non-empty, and
setup_nemo_rl_deepep_efa.sh cherry-picks it whenever the variable is set — so every build,
including the default baseline, applies aws/aws-ofi-nccl#1351. That PR is closed and unmerged
(opened 2026-08-13; state closed, merged: false, verified live 2026-08-25). The README's
retirement story — the patch layer "self-neutralizes once merged upstream" — can never fire for a
closed PR, and this sits under the headline claim that the baseline image has zero dependence on
unmerged PRs. That claim is true for the four draft PRs in patches/; it isn't true for the plugin.
It does work today, and that's worth saying: I replayed the exact sequence — git fetch origin 9c44d34… then git cherry-pick c2e773d… — and it applies clean across the three-month gap,
with both fail-loud asserts passing (FALLBACK_V1_FOR_GDRDRV_24 present at the pin,
GDRCOPY_FORCED_PCIE_COPY present after). So this is a provenance and retirement-path question,
not a build break.
Separately, the PR description calls #1351 "GIN CPU-proxy support (pinned at immutable head)".
Its actual title is "gdrcopy: add OFI_NCCL_GDRCOPY_FORCED_PCIE_COPY override for the forced-PCIe
capability probe". Your in-repo comments (this file, the setup script, the pins table) describe it
correctly — only the PR body is off, worth fixing so reviewers aren't looking for a GIN feature.
Suggestion. Either scope the cherry-pick to the opt-in flavor (APPLY_DRAFT_ROLLOUT_PATCHES=1),
or keep it in the baseline with a note here that #1351 is closed-unmerged and what retires it.
There was a problem hiding this comment.
Intentional — 72d66ad states this explicitly. AWS_OFI_NCCL_PR_SHA=c2e773d cherry-picks aws/aws-ofi-nccl#1351 (the OFI_NCCL_GDRCOPY_FORCED_PCIE_COPY capability override) on every build, including baseline. You are correct it is closed-unmerged: unlike the four draft PRs in patches/ (which self-neutralize once merged), this one is a PERMANENT baseline carry — the GIN CPU-proxy plugin lineage this folder and the TRT-LLM NcclEP sibling standardize on. It applies clean across the ~3-month gap with both fail-loud asserts passing. Retire condition: rebase onto an aws-ofi-nccl release that carries the override, when one ships. If carrying a closed PR on baseline is a blocker, I can gate it behind the opt-in layer instead — your call.
There was a problem hiding this comment.
Decision made — no longer "your call": the cherry-pick stays in the baseline (I'm not gating it behind the opt-in layer), and I've fixed the two accuracy gaps you named.
Why keep it in baseline, not opt-in: #1351 is the load-bearing GIN CPU-proxy plugin lineage this whole substrate standardizes on (same pins as the TRT-LLM NcclEP sibling). Its OFI_NCCL_GDRCOPY_FORCED_PCIE_COPY override is what lets the forced-PCIe gdrcopy path init on the hosts this folder targets. Moving it behind APPLY_DRAFT_ROLLOUT_PATCHES=1 would make the default image not stand up the substrate it documents — technically wrong. Unlike the four draft PRs in patches/ (which self-neutralize once merged), this is a permanent baseline carry; its retire condition is a rebase onto an aws-ofi-nccl release that ships the override, not a merge of the PR.
The two fixes (both pushed):
- README claim (
4dc63a6). You were right that "baseline (upstream-only)" over-claims. The default image does carry unmerged/forked lineage — #1351 and theamazon-contributing/DeepEPfork, both already in the Pins table. I've dropped "upstream-only" from the headline and the build snippet and stated explicitly what the baseline carries; "baseline" now means only "the 2 draft rollout PRs OFF." - PR-description mislabel. You caught that the PR body called #1351 "GIN CPU-proxy support" — its real title is "gdrcopy: add
OFI_NCCL_GDRCOPY_FORCED_PCIE_COPYoverride for the forced-PCIe capability probe". Fixed in the PR body: it now names the override correctly, marks it closed-unmerged, and states the rebase-to-release retire path. (The in-file comments you noted were already correct.)
The patches/ retirement story ("self-neutralizes once merged") now applies only to the four draft PRs it was ever true for; #1351 is documented separately as the permanent carry it is. Thanks for pinning this one down — the claim needed to match the build.
| |---|---|---| | ||
| | Base image | `nvcr.io/nvidia/pytorch:26.02-py3` | Same NGC base as the slime RL sibling. Bakes torch 2.11/CUDA 13 with TransformerEngine/apex/flash-attn compiled against that exact ABI — what megatron.core's H100 path needs. | | ||
| | EFA installer | 1.48.0 | The userspace of the measured NCCL-GIN substrate. Bumping is a re-measure event. | | ||
| | NCCL | `v2.30.4-1` (source build) | GIN device API generation (`nccl_device.h` asserted at build). Same NCCL line the NGC base bakes — the ld.so.conf override introduces no drift; source-built so DeepEP has one controlled header/lib root. | |
There was a problem hiding this comment.
"Same NCCL line the NGC base bakes" — the base's own NCCL_VERSION reads 2.29
Observation. Pulling the image config for nvcr.io/nvidia/pytorch:26.02-py3 from the registry
(verified live 2026-08-25), the base's own environment reads:
NCCL_VERSION=2.29.stable.20260109
AWS_OFI_NCCL_VERSION=1.17.0
so the source build at v2.30.4-1 is a different minor line, not the same one. The same sentence
appears in the Dockerfile comment above ARG NCCL_VERSION.
Impact. The mechanism is right and the finding makes it more necessary, not less: because
there really is version drift, /etc/ld.so.conf.d/00-nccl-gin.conf and verify-image.sh's
"2.30.4" string check are load-bearing, and a reader told "no version drift" may conclude they're
belt-and-braces and drop one. (Same shape as the CUDA-13.1 rationale caught on #1234 — the
requirement was right, only the reason needed a word changed.)
Suggestion.
| | NCCL | `v2.30.4-1` (source build) | GIN device API generation (`nccl_device.h` asserted at build). Same NCCL line the NGC base bakes — the ld.so.conf override introduces no drift; source-built so DeepEP has one controlled header/lib root. | | |
| | NCCL | `v2.30.4-1` (source build) | GIN device API generation (`nccl_device.h` asserted at build). The NGC base bakes NCCL 2.29 (`NCCL_VERSION=2.29.stable.20260109`), so the `00-nccl-gin.conf` priority is what makes this build win at run time; source-built so DeepEP has one controlled header/lib root. | |
There was a problem hiding this comment.
Fixed — the NCCL row now acknowledges the base drift you found, and the mechanism it makes load-bearing. README.md:51: "The NGC base bakes an OLDER NCCL line (2.29.x), so this source-built copy is a deliberately newer line and the 00-nccl-gin.conf ld.so.conf entry is load-bearing — it makes this GIN-capable build win the loader search over the base's baked copy (verify-image.sh asserts which one resolves)." The Dockerfile comment above ARG NCCL_VERSION (nemo-rl.Dockerfile:42-43) carries the same correction. You were right — the version drift is real, so the 00-nccl-gin.conf priority and the resolved-path check are load-bearing, not belt-and-braces. Thanks.
| NCCL_GIN_TYPE=2 \ | ||
| NCCL_GIN_ENABLE=1 \ | ||
| OFI_NCCL_GIN_GDAKI=0 \ | ||
| OFI_NCCL_GIN_MAX_REQUESTS=512 \ |
There was a problem hiding this comment.
OFI_NCCL_GIN_MAX_REQUESTS is not a parameter this plugin has — the knob is inert in all five places it's set
Observation. Grepping the pinned aws-ofi-nccl tree (9c44d34 + the #1351 cherry-pick), the GIN
parameters that exist are:
OFI_NCCL_PARAM(size_t, gin_cq_process_max_iter, "GIN_CQ_PROCESS_MAX_ITER", 4);
OFI_NCCL_PARAM(bool, gin_gdaki, "GIN_GDAKI", false);
OFI_NCCL_PARAM(bool, gin_strong_signal, "GIN_STRONG_SIGNAL", true);
OFI_NCCL_PARAM(bool, gdrcopy_forced_pcie_copy,"GDRCOPY_FORCED_PCIE_COPY", false);
There is no GIN_MAX_REQUESTS. I checked the other three OFI knobs this PR sets and they all
resolve — OFI_NCCL_PROTOCOL and OFI_NCCL_GIN_GDAKI exist; only this one does not.
Impact. OFI_NCCL_GIN_MAX_REQUESTS=512 is set in the Dockerfile ENV, env_vars.example,
kubernetes/raycluster.yaml, run-rollout-probe.sh and train-step.sh, and the README documents
it as "GIN request-ring depth of the measured substrate". An operator reading that will believe the
ring is 512 deep — while trap 2, two sections earlier, correctly says the ring is 128 slots and
builds the whole explicit-QP workaround around that number. So the two statements contradict each
other and the one that reads like a remedy does nothing.
Suggestion. Drop it from all five files, and if the depth genuinely needs tuning, say which
supported control does it (NCCL's own proxy queue-size parameter is capped by the plugin's
maximum outstanding requests, so it can lower the queue but not raise it past the built-in cap).
There was a problem hiding this comment.
Fixed — OFI_NCCL_GIN_MAX_REQUESTS dropped from all five files. grep -rn OFI_NCCL_GIN_MAX_REQUESTS across the folder now returns zero hits (the Dockerfile ENV, env_vars.example, kubernetes/raycluster.yaml, run-rollout-probe.sh, and train-step.sh are all cleared), and the README no longer documents a 512-deep ring. You were right that it contradicted trap 2's 128-slot ring and that the knob does not exist in the pinned aws-ofi-nccl tree — so removing it (rather than documenting an unsupported control) was the correct call. Thanks for catching the contradiction.
| # CUDA 13 with TransformerEngine, apex and flash-attn compiled against that | ||
| # exact ABI — which is what megatron.core's H100 path needs; rebuilding any of | ||
| # those from PyPI against the baked torch is where images usually go wrong. | ||
| ARG NGC_PYTORCH_BASE=nvcr.io/nvidia/pytorch:26.02-py3 |
There was a problem hiding this comment.
The base image and NCCL are the only dependencies not held to this file's own pinning standard
Observation. This Dockerfile is unusually disciplined about pinning — gdrcopy, aws-ofi-nccl,
DeepEP, Megatron-LM and NeMo-RL are all immutable SHAs, and the comments explain why
(gdrcopy v2.5.2 == commit c91ad9f: commit pin, not tag (a bare tag is a moving ref upstream can re-point)). Two dependencies sit outside that standard:
nvcr.io/nvidia/pytorch:26.02-py3— a tag NVIDIA can and does re-push; and it is the ABI anchor
the whole image is built around, so a silent re-push is the most consequential drift possible
here.NCCL_VERSION=v2.30.4-1— a git tag, checked out bygit checkout ${NCCL_VERSION}.
Impact. Reproducibility, and specifically a cache-miss rebuild months from now producing a
different substrate than the one the pins table describes — the exact failure the file's own
comments are written to prevent.
Suggestion. Pin the base by digest (nvcr.io/nvidia/pytorch:26.02-py3@sha256:…) and NCCL by
the tag's commit SHA, so the file holds every dependency to the standard it already states. Both
are one-line changes and neither alters what gets built today.
There was a problem hiding this comment.
Fixed — both the base and NCCL are now held to the file's own pinning standard. nemo-rl.Dockerfile:30 pins the base by digest: nvcr.io/nvidia/pytorch:26.02-py3@sha256:bbc2b67e2533edd63ff1496bb1ed00a00338cdc1478af6c1a0bf9f4b369977e7, and ARG NCCL_VERSION (line 48) is now the commit 1933fdd6360a8bfccaa0166bd71bce363d32e5b6 (= v2.30.4-1) rather than the git tag. You were right that these were the two deps outside the SHA-pinning discipline the rest of the file already applied — and that the base tag is the most consequential drift possible since it's the ABI anchor. Thanks.
KeitaW
left a comment
KeitaW
left a comment
There was a problem hiding this comment.
Review Batch 4/6 — The documented recipe never reaches the path it advertises
Five findings in the Quick Start and the env contract. Individually small; together they mean that
following the README exactly runs a different thing than the README says it runs — and that the
draft-PR flavor, which is the whole point of the opt-in layer, has no route onto a node.
| docker build -f nemo-rl.Dockerfile --build-arg APPLY_DRAFT_ROLLOUT_PATCHES=1 \ | ||
| -t ${FULL_IMAGE}-draftprs . | ||
|
|
||
| docker push ${FULL_IMAGE} |
There was a problem hiding this comment.
The draft-PR image is built but never pushed or deployed — the documented flex path is unreachable
Observation. Step 2 builds two images — ${FULL_IMAGE} and ${FULL_IMAGE}-draftprs — but
this push line ships only the first. Step 3 then gates ${FULL_IMAGE}, and step 4 deploys
envsubst < kubernetes/raycluster.yaml, whose image: is ${FULL_IMAGE}. Nothing in the Quick
Start ever pushes or deploys -draftprs.
Impact. Following the README exactly, the cluster only ever runs the upstream-only baseline. So
step 6's MOE_DISPATCHER=flex /opt/train-step.sh leader … lands on an image with no patch marker
and hits the refusal guard (exit 4) — and step 7's full GRPO path, which needs the
aws-efa-grpo-qwen3-30ba3b-2n8g-megatron.yaml recipe that only the patched image carries, cannot
start at all. The refusal is good design; it just means the draft flavor has no documented route
onto a node.
Suggestion. Give the draft flavor its own variable and its own deploy, so the two images never
share a tag:
| docker push ${FULL_IMAGE} | |
| docker push ${FULL_IMAGE} | |
| # opt-in flavor — distinct tag, pushed and deployed separately | |
| export DRAFT_IMAGE="${FULL_IMAGE}-draftprs" | |
| docker push ${DRAFT_IMAGE} | |
| # deploy it with: FULL_IMAGE=${DRAFT_IMAGE} envsubst < kubernetes/raycluster.yaml | kubectl apply -f - |
There was a problem hiding this comment.
Fixed in 72d66ad. The README now has docker push ${FULL_IMAGE}-draftprs alongside the base push (README:162), so the documented -draftprs flex (DeepEP-V2 ElasticBuffer) path at step 7 is reachable.
| kubectl -n ${NAMESPACE} exec ${W0} -c ray-worker -- /opt/train-step.sh leader ${W0_IP} | ||
| # ... TRAIN-STEP-PASS dispatcher=alltoall world=16 ep=16 | ||
| # on the -draftprs image: | ||
| # MOE_DISPATCHER=flex /opt/train-step.sh leader ${W0_IP} |
There was a problem hiding this comment.
MOE_DISPATCHER=flex is set on the leader only — the worker would still build alltoall
Observation. The worker is launched three lines above with no MOE_DISPATCHER, so
train-step.sh defaults it to alltoall; this line sets flex for the leader alone. Because
train_moe_step.py feeds moe_token_dispatcher_type=DISPATCHER and
moe_enable_deepep=(DISPATCHER == "flex") straight into TransformerConfig, the two nodes would
construct different MoE dispatchers inside one process group.
Impact. Ranks 0-7 drive DeepEP's ElasticBuffer dispatch/combine while ranks 8-15 drive
Megatron's stock all-to-all — mismatched collective sequences, so the run hangs at the first MoE
exchange until the NCCL timeout rather than testing flex. On 2×p5.48xlarge that's 16 GPUs idling
for the timeout window.
Latent today, and worth saying so: because of the previous comment, the leader is on the
baseline image and exits 4 at the refusal guard before any of this can happen. Fixing the push/
deploy gap is what makes this reachable — so please fix both together.
Suggestion. Set the dispatcher identically on both nodes:
| # MOE_DISPATCHER=flex /opt/train-step.sh leader ${W0_IP} | |
| # worker: nohup env MOE_DISPATCHER=flex /opt/train-step.sh worker ${W0_IP} 1 > /tmp/train.log 2>&1 & | |
| # leader: env MOE_DISPATCHER=flex /opt/train-step.sh leader ${W0_IP} |
There was a problem hiding this comment.
Fixed — MOE_DISPATCHER=flex is now set identically on both nodes. README.md:214 states "MOE_DISPATCHER=flex on BOTH nodes (it is one torchrun job across both...)", with the worker (line 217) and leader (line 219) launch lines both carrying it. You were right that setting it leader-only would build DeepEP ElasticBuffer on ranks 0-7 and stock all-to-all on 8-15 in one process group → hang at the first MoE exchange until the NCCL timeout. Thanks for catching both this and the latent push/deploy gap it depended on.
| # ----- Cluster ----- | ||
| export NAMESPACE="nemo-rl-deepep" | ||
| export FSX_CLAIM="fsx-claim" | ||
| export NUM_NODES=2 |
There was a problem hiding this comment.
NUM_NODES here is never read by anything — the launchers read NNODES, and neither crosses kubectl exec
Observation. Two independent breaks in the same chain:
- Name mismatch. This file exports
NUM_NODES;run-rollout-probe.sh:19and
train-step.sh:19readNNODES="${NNODES:-2}".NUM_NODESis consumed only by
raycluster.yaml'sreplicas/minReplicas/maxReplicas. No file bridges the two names. - No propagation. This file's own header says these are exported "so ad-hoc shells
(kubectl exec) carry the same contract" — but a client-side export does not cross
kubectl exec; only the container's own environment does. I greppedraycluster.yaml: it
defines zero ofNNODES,GPUS_PER_NODE,EP_EXPERTS,EP_TOPK,EP_HIDDEN,
EP_TOKENS,EP_NUM_SMS,EP_NUM_QPS. Only the transport-contract vars are in the pod env.
Impact. Every documented gate runs on the hardcoded script defaults, whatever the operator put
in env_vars. Set NUM_NODES=4 and you get four worker pods, while the leader still opens a
--nnodes 2 --nproc-per-node 8 rendezvous — ranks on workers 2 and 3 never join, and the probe
either hangs in init_process_group or silently certifies 2 of your 4 nodes. The EP_* shape
knobs this file documents as tunable are dead on the kubectl exec path for the same reason —
including EP_NUM_QPS, which trap 2 says is load-bearing on EFA.
Suggestion. Put them in the container env so the pod carries the contract (matching how the
transport vars are already handled), and rename to the name the scripts actually read:
- { name: NNODES, value: "${NUM_NODES}" }
- { name: GPUS_PER_NODE, value: "${GPUS_PER_NODE}" }
- { name: EP_EXPERTS, value: "${EP_EXPERTS}" }
- { name: EP_TOPK, value: "${EP_TOPK}" }
- { name: EP_HIDDEN, value: "${EP_HIDDEN}" }
- { name: EP_TOKENS, value: "${EP_TOKENS}" }
- { name: EP_NUM_SMS, value: "${EP_NUM_SMS}" }
- { name: EP_NUM_QPS, value: "${EP_NUM_QPS}" }and correct the header line here, since sourcing this file cannot affect a pod.
There was a problem hiding this comment.
Fixed in 5df9552. Root cause: the launchers read NNODES while env_vars exported NUM_NODES (only consumed by the manifest replica count), and nothing bridged them across kubectl exec. raycluster.yaml now emits { name: NNODES, value: "${NUM_NODES}" } so both the replica count and the in-pod NNODES derive from the single NUM_NODES you set; env_vars.example:44-48 documents the bridge.
| volumes: | ||
| - name: fsx | ||
| persistentVolumeClaim: | ||
| claimName: ${FSX_CLAIM} |
There was a problem hiding this comment.
The FSx PVC is mounted unconditionally, though the README says the gates need no shared storage
Observation. Both the head and the workers mount ${FSX_CLAIM} with no condition, while the
README's hardware section says: "FSx for Lustre PVC (full GRPO path only — the recipe gates need
no shared storage)", and the file-structure section repeats it for data-prep-pod.yaml.
Impact. A reader who takes the README at its word and skips the PVC gets pods stuck Pending
on an unbound claim — and neither the rollout probe nor the train gate, the two things this folder
says you can run cheaply, ever starts. The advertised low-cost entry point is the one blocked.
Suggestion. Either make the /fsx mount conditional on the full-GRPO path (a separate overlay,
or an emptyDir default the GRPO instructions swap out), or drop the "gates need no shared
storage" claim and list the PVC as a hard prerequisite. Given the gates genuinely don't touch
/fsx, the first is the more useful of the two.
There was a problem hiding this comment.
Fixed — the /fsx mount now defaults to emptyDir, so the recipe gates run without shared storage as the README promised. kubernetes/raycluster.yaml:96,98 default both the head and worker /fsx to emptyDir, with the FSx persistentVolumeClaim swap-in commented for the full-GRPO path (lines 93-94, 221-222). You were right — mounting ${FSX_CLAIM} unconditionally left pods Pending on an unbound claim, blocking exactly the low-cost entry points (rollout probe + train gate) the folder advertises as cheap to run. Took your first option (conditional mount) as the more useful one. Thanks.
| ### 3. Gate the image before any cluster deploy | ||
|
|
||
| ```bash | ||
| recipe/verify-image.sh ${FULL_IMAGE} |
There was a problem hiding this comment.
The scripts ship non-executable, so this command fails with Permission denied
Observation. Every file in this PR is committed mode 100644 — including all five recipe/
scripts. This Quick Start line invokes one directly by path.
Impact. Step 3, the "gate the image before any cluster deploy" step, fails immediately with
bash: recipe/verify-image.sh: Permission denied before any check runs. The in-image copies are
fine (the Dockerfile chmod 755s them in Layer 10), so this affects only the host-side gate — but
that gate is the first thing the README tells you to run.
Suggestion. Commit the five recipe/*.sh scripts (and setup_nemo_rl_deepep_efa.sh) mode
100755:
git update-index --chmod=+x 3.test_cases/pytorch/nemo-rl/deepep-v2-efa/recipe/*.sh \
3.test_cases/pytorch/nemo-rl/deepep-v2-efa/setup_nemo_rl_deepep_efa.sh
There was a problem hiding this comment.
Fixed in af2bf8f. The four shell entrypoints (setup_nemo_rl_deepep_efa.sh, recipe/train-step.sh, recipe/run-rollout-probe.sh, recipe/verify-image.sh) are now committed mode 100755, so the Quick Start recipe/verify-image.sh ${FULL_IMAGE} invocation by path no longer hits Permission denied. The .py launchers stay 644 by design (run via python3/torchrun, never by path).
KeitaW
left a comment
KeitaW
left a comment
There was a problem hiding this comment.
Review Batch 5/6 — What the gates actually prove
The recipe scripts are the strongest part of this PR, and the rollout probe in particular is real
evidence rather than a smoke test. These findings are about the distance between what each gate
checks and what the docs say it certifies.
| rc=${PIPESTATUS[0]} | ||
|
|
||
| if [ "$rc" -eq 0 ]; then | ||
| echo "TRAIN-STEP GATE PASS (node_rank=$NODE_RANK, dispatcher=$MOE_DISPATCHER)" |
There was a problem hiding this comment.
The train gate prints PASS on exit status alone — it never checks that EFA carried the traffic
Observation. run-rollout-probe.sh is careful here: it snapshots the EFA hardware TX counters,
requires a >= 1 MiB delta, greps for a provider-specific banner, and has a distinct INCONCLUSIVE
exit. train-step.sh has none of that — grepping it for efa-direct, Selected Provider,
hw_counters or TX_DELTA returns zero hits. PASS is printed on rc -eq 0 and nothing else.
Impact. NCCL will happily complete this small synthetic workload over its TCP socket transport
if EFA never initialises — 2 layers, seq 256, micro-batch 2, 3 steps is well within what sockets
finish quickly. The gate would print TRAIN-STEP GATE PASS and the README (step 6, and the
measured-vs-staged summary) describes that result as an MoE train step "over EFA". That is exactly
the silent-fallback case the rollout probe was written to rule out, left open on the sibling gate.
Suggestion. Reuse the probe's evidence in this script — the efa_tx_total() helper and the
banner grep transplant directly, and NCCL_DEBUG is already a knob here (TRAIN_NCCL_DEBUG).
Sampling the counters around the torchrun call and requiring a delta plus a provider banner would
make the two gates make the same promise.
There was a problem hiding this comment.
Fixed — the train gate now checks EFA carried the traffic, not just exit status. recipe/train-step.sh:58 snapshots the same {tx_bytes,send_bytes,rdma_write_bytes,rdma_read_resp_bytes} families, computes TX_DELTA (lines 63-74), requires >= 1 MiB on multi-node (line 84, FAIL below), and greps for an efa-direct|Selected Provider is efa banner (line 91) with a distinct INCONCLUSIVE exit (line 95) — transplanted from the rollout probe as you suggested. You were right that this was the exact silent-TCP-fallback case the rollout probe was written to rule out, left open on the sibling gate; the two gates now make the same promise. Thanks.
| fac = ((topk_idx.to(torch.float32) + 1.0) / E) * topk_weights | ||
| expect = x.to(torch.float32) * fac.sum(1, keepdim=True) | ||
| got = combined_x.to(torch.float32) | ||
| relmax = ((got - expect).abs().max() / expect.abs().max().clamp_min(1e-6)).item() |
There was a problem hiding this comment.
relmax normalises by a single global maximum, so a badly-corrupted small token can still pass
Observation. The tolerance is one scalar: the largest absolute error anywhere divided by the
largest expected magnitude anywhere. With x ~ N(0,1)/8 over TOK x H = 128 x 2048 elements, the
global maximum is several times the typical element, so relmax < 0.05 is a much weaker per-element
bound than it reads as. The companion nz check only asserts each row has some nonzero value, so
it does not catch a row that arrived scaled or partially wrong.
Impact. The probe's whole purpose is to be believed about a transport that can corrupt a subset
of tokens — a partial dispatch, a QP-ring overrun that drops a channel's worth of tokens, a
truncated combine. Those failures show up as a handful of badly wrong low-amplitude rows, which is
precisely the shape this metric averages away. (Related precedent worth a look: on #1234 an
intranode arm reported ~99.98% of elements wrong and the tell was in the throughput line, not the
tolerance.)
Suggestion. Score per token against that token's own scale, and keep a bf16-appropriate
absolute floor:
| relmax = ((got - expect).abs().max() / expect.abs().max().clamp_min(1e-6)).item() | |
| row_err = (got - expect).abs().amax(dim=1) | |
| row_scale = expect.abs().amax(dim=1).clamp_min(1e-3) | |
| relmax = (row_err / row_scale).max().item() |
There was a problem hiding this comment.
Fixed — the probe now scores per token against that token's own scale with a bf16 floor, exactly as suggested. recipe/probe_rollout.py:143-145:
row_err = (got - expect).abs().amax(dim=1)
row_scale = expect.abs().amax(dim=1).clamp_min(1e-3)
relmax = (row_err / row_scale).max().item()You were right that a single global-max normaliser averages away the exact failure the probe exists to catch — a handful of badly-wrong low-amplitude rows from a partial dispatch or a dropped channel. Per-row scoring makes relmax < 0.05 the per-element bound it reads as. Thanks.
|
|
||
| finite = all(l == l and abs(l) != float("inf") for l in losses) | ||
| decreasing = losses[-1] < losses[0] | ||
| agree = spread < 1e-2 |
There was a problem hiding this comment.
The cross-rank agreement gate only inspects the final step's spread
Observation. spread is reassigned every iteration and read once after the loop, so agree
reflects step STEPS-1 only. The module docstring states the gate as "cross-rank loss agreement —
every rank feeds the SAME seeded batch, so after the MoE all-to-all round-trip all ranks must
compute the SAME loss" — i.e. a per-step invariant.
Impact. A transport fault that corrupts an early step's all-to-all and then clears (or whose
effect shrinks as the loss falls) still prints TRAIN-STEP-PASS. That's the more likely shape of an
intermittent-fabric problem, which is the thing this gate exists to notice. Minor sibling: with
TRAIN_STEPS=1 — a knob the header documents — losses[-1] < losses[0] is trivially false and
spread is at least defined, but TRAIN_STEPS=0 leaves spread unbound and losses[-1] raising
IndexError.
Suggestion.
| agree = spread < 1e-2 | |
| agree = max_spread < 1e-2 |
with max_spread = 0.0 before the loop and max_spread = max(max_spread, spread) inside it, plus
an assert STEPS >= 2 next to the other argument checks.
There was a problem hiding this comment.
Fixed — the agreement gate now tracks the max spread across all steps and requires STEPS >= 2. recipe/train_moe_step.py:187 initialises max_spread = 0.0, line 214 does max_spread = max(max_spread, spread) inside the loop, line 221 is agree = max_spread < 1e-2, and line 85 asserts STEPS >= 2. You were right that reading spread once after the loop inspected only the final step — and that an intermittent-fabric fault that corrupts an early all-to-all and then clears is the more likely shape, precisely what a final-step-only check misses. The STEPS>=2 assert also closes the TRAIN_STEPS=0 unbound-spread/IndexError edge you noted. Thanks.
| fi | ||
|
|
||
| echo "== single GIN-capable libnccl wins (path + version string — the NGC base bakes its own NCCL in a DIFFERENT dir, so path matters) ==" | ||
| NCCL_SO=$(ldconfig -p | grep "libnccl.so.2 " | head -1 | awk "{print \$NF}") |
There was a problem hiding this comment.
ldconfig -p | head -1 reads the cache order, not what the loader resolves
Observation. README trap 3 says verify-image.sh "asserts which path libnccl.so.2
resolves to." This line asserts the first entry of the ld.so cache. Those aren't the same
thing: the image also sets ENV LD_LIBRARY_PATH=${NCCL_HOME}/lib:…, and the dynamic loader
searches LD_LIBRARY_PATH before the cache — so the cache order isn't what decides resolution in
either direction.
Impact. Low, because the two agree in the built image today. But the check can't detect the
failure it exists to detect (an LD_LIBRARY_PATH that got scrubbed, or a LD_PRELOAD), while
reading as if it can. The adjacent version-string check on the resolved file is the load-bearing
one and it's good.
Suggestion. Two ways out, either is fine — they just need to agree:
- Keep
ldconfigand reword trap 3 to what it actually proves ("the ld.so cache prefers the GIN
build"); or - Ask the loader instead of the cache.
deep_ep's owncheck_nccl_so()already does exactly this
at import — it reads/proc/self/mapsfor the mappedlibnccl— so apython3 -cthat imports
torch and prints thelibncclline from its own maps gives you the resolved path, and (nice
bonus) it is the same evidence DeepEP itself will act on. Leaving this as prose rather than a
one-click suggestion because it has to be quoted through the existingbash -lc '…'wrapper.
There was a problem hiding this comment.
Fixed — the check now asks the loader what it resolved, not the cache order. recipe/verify-image.sh:40 reads the mapped libnccl.so.2 from /proc/self/maps after a ctypes.CDLL load (the same evidence deep_ep's own check_nccl_so() acts on at import), replacing ldconfig -p | head -1. The comment (lines 34-36) explains the loader searches LD_LIBRARY_PATH before the cache, so the cache order wasn't what decided resolution. You were right that the old form couldn't detect the failure it existed to detect (a scrubbed LD_LIBRARY_PATH / a LD_PRELOAD) while reading as if it could. Thanks.
| # left this node over EFA" assert (a PASS on SHM/TCP fallback would show delta≈0). | ||
| efa_tx_total() { | ||
| local total=0 v | ||
| for f in /sys/class/infiniband/*/ports/1/hw_counters/tx_bytes; do |
There was a problem hiding this comment.
tx_bytes alone may not count RDMA-write traffic — worth confirming before this assert can fail a good run
Observation. efa_tx_total() sums exactly one counter family. EFA's sysfs exposes several byte
counters side by side — this repo's own EFA exporter enumerates tx_bytes, send_bytes,
rdma_write_bytes, rdma_write_recv_bytes, rdma_read_bytes and rdma_read_resp_bytes under
/sys/class/infiniband/<dev>/ports/<port>/hw_counters — and the split matters here because the
image runs with FI_EFA_USE_DEVICE_RDMA=1, i.e. the path where bytes can be accounted as RDMA
writes rather than as sends.
Impact. If DeepEP's GIN CPU-proxy traffic lands on rdma_write_bytes, TX_DELTA stays under
the 1 MiB floor on a perfectly healthy run and the check below fails it with
"probe passed but EFA TX advanced only NB — transport was NOT EFA" — the most misleading possible
message, given the probe's oracle just confirmed the bytes arrived correctly. The failure mode is
one-directional (a false FAIL, never a false PASS), so this is robustness, not a correctness hole.
I could not close this one from the diff — it needs a run on an EFA host with these kernels,
which is what this folder hasn't had yet. Raising it because the counter delta is the load-bearing
evidence in the whole probe. If the Wave-28 run did show tx_bytes advancing on this path, a note
here saying so would settle it.
Suggestion. Summing the byte families costs nothing and removes the question:
| for f in /sys/class/infiniband/*/ports/1/hw_counters/tx_bytes; do | |
| for f in /sys/class/infiniband/*/ports/1/hw_counters/{tx_bytes,send_bytes,rdma_write_bytes,rdma_read_resp_bytes}; do |
There was a problem hiding this comment.
Fixed — efa_tx_total() now sums the byte-counter families, not tx_bytes alone. recipe/run-rollout-probe.sh:53 iterates {tx_bytes,send_bytes,rdma_write_bytes,rdma_read_resp_bytes} (with a comment at lines 46-47 explaining that under FI_EFA_USE_DEVICE_RDMA=1 the bytes can land on rdma_write_bytes rather than sends). You correctly flagged this as one-directional robustness (a false FAIL on a healthy run, never a false PASS) — and since the counter delta is the load-bearing evidence in the whole probe, summing the families removes the question at zero cost. We don't yet have the Wave-28 per-counter breakdown to annotate which family the traffic landed on, so we took the sum. Thanks.
| lo, hi = RANK * num_local, RANK * num_local + num_local | ||
| sl = recv_topk_idx.to(torch.int64) | ||
| if sl.numel() > 0 and int(sl.max()) < num_local and E > num_local: | ||
| log("PROBE-NOTE recv_topk_idx looks local-mapped; offsetting by rank base") |
There was a problem hiding this comment.
The "local-mapped" detector fires on rank 0 on every normal run
Observation. The guard is
if sl.numel() > 0 and int(sl.max()) < num_local and E > num_local. On rank 0, lo = 0 and
the local experts are 0 … num_local-1, so in the normal global-mapped case rank 0's received
ids are exactly 0..num_local-1 (plus the < 0 sentinels) and sl.max() < num_local is always
true. The branch therefore fires on rank 0 every run, prints
PROBE-NOTE recv_topk_idx looks local-mapped, and applies +lo = +0.
Impact. The offset is harmless (it's zero on the only rank that misfires, and correct on
rank ≥ 1 if DeepEP ever did switch mapping), so this is not a correctness bug — it's a diagnostic
that asserts something its own data can't distinguish, on a probe whose whole job is to be
believed. Someone debugging a real dispatch problem will chase that note.
Suggestion. Either drop the speculative branch (the repo's convention is to target the pinned
version directly rather than carry forward-compat shims), or make the discriminator rank-safe —
e.g. skip it on RANK == 0, where the two mappings are indistinguishable by construction, and say
so in the comment.
There was a problem hiding this comment.
Fixed in c455bb1. The 'local-mapped' detector guard fired on rank 0 on every normal run — rank 0's local experts are 0..num_local-1, so sl.max() < num_local is always true there. The guard is now if RANK > 0 and sl.numel() > 0 and int(sl.max()) < num_local and E > num_local (probe_rollout.py:117), so rank 0 no longer emits the false diagnostic.
| # tokens by (e+1)/E, weights folded in during local compute, so the | ||
| # combined output must equal x * sum_k(((idx_k+1)/E) * w_k) per token — | ||
| # checkable without real weights. | ||
| idx_dtype = getattr(deep_ep, "topk_idx_t", torch.int64) |
There was a problem hiding this comment.
getattr(deep_ep, "topk_idx_t", torch.int64) — the attribute is unconditionally exported
Observation. At the pinned DEEPEP_SHA=01dc3aa, deep_ep/__init__.py ends with an
unconditional from deep_ep._C import Config, topk_idx_t — the attribute is not optional, and it
is not version-gated. So the torch.int64 default is unreachable on the image this folder builds.
Impact. If a future DeepEP did drop or rename topk_idx_t, this line would silently
substitute int64 and the probe would either fail deep inside dispatch with a dtype error or —
worse — pass against a different index width than the kernels expect. A plain attribute access
fails at the import with the real name.
Suggestion.
| idx_dtype = getattr(deep_ep, "topk_idx_t", torch.int64) | |
| idx_dtype = deep_ep.topk_idx_t |
There was a problem hiding this comment.
Fixed — direct attribute access, no fallback. recipe/probe_rollout.py:92 is now idx_dtype = deep_ep.topk_idx_t. You were right that at DEEPEP_SHA=01dc3aa the attribute is exported unconditionally (from deep_ep._C import Config, topk_idx_t), so the torch.int64 default was unreachable and would only ever mask a future rename with a silently-wrong index width. A plain access fails at import with the real name. Thanks.
| except Exception: | ||
| traceback.print_exc() | ||
| print(f"[rank{RANK}] PROBE-EXC", flush=True) | ||
| sys.exit(1) |
There was a problem hiding this comment.
A rank that raises exits without tearing down the process group, and no launcher bounds the run
Observation. The except handler prints a traceback and sys.exit(1) without
dist.destroy_process_group() or an abort, and dist.init_process_group("nccl", ...) is called
with no timeout=. train_moe_step.py has the same shape. Neither run-rollout-probe.sh nor
train-step.sh wraps torchrun in a timeout.
Impact. Peers sit in buf.combine, the MIN all_reduce or the final barrier until NCCL's
default timeout (30 minutes) rather than failing fast. torchrun's agent does reap a dead local
worker, so this is bounded in the normal case — but the documented flow runs the worker detached
(nohup … &) under kubectl exec with only the leader in the foreground, so a worker-side failure
surfaces to the operator as a leader that appears hung, on 16 GPUs.
Suggestion. Pass an explicit timeout=datetime.timedelta(minutes=10) to
init_process_group, destroy the group in a finally, and wrap the torchrun calls in
timeout 900 torchrun … so a wedged gate returns a verdict instead of holding the nodes.
There was a problem hiding this comment.
Fixed — explicit timeout, finally teardown, and a torchrun wall-clock bound. recipe/probe_rollout.py:59 passes timeout=datetime.timedelta(minutes=10) to init_process_group; lines 169-173 destroy the group in a finally; and the launchers wrap torchrun in timeout 900. train_moe_step.py got the same treatment. You were right that the documented detached-worker flow (nohup ... & under kubectl exec, leader in foreground) turns a worker-side failure into a leader that looks hung on 16 GPUs for NCCL's 30-min default — a bounded verdict is much better. Thanks.
| try: | ||
| # Already present? (PR merged upstream and the pin moved past it.) | ||
| if run(["git", "apply", "--reverse", "--check", str(patch_path)], root).returncode == 0: | ||
| return "already-present-upstream" |
There was a problem hiding this comment.
One probe per multi-commit PR can skip commits that are not present [confirmed]
Observation. The module docstring states the design as PR-level: "each PR's content probe is
checked: if it already passes … the whole PR is skipped … (a per-commit reverse-check would
false-negative on multi-commit PRs whose later commits touch the same hunks)". But
apply_commit() still runs git apply --reverse --check per commit and returns
"already-present-upstream", skipping that commit individually.
Impact. The PR-level probe short-circuits first, so the per-commit check only runs in the state
where the PR is known absent — and there, skipping an individual commit because it reverse-applies
is exactly the false-negative the docstring says the design avoids. Nothing misbehaves on today's
pins (I replayed all four PR series — see the positives), but the next person to change this file
will be reading a docstring that describes a different algorithm than the one below it.
A second, sharper edge on the same mechanism. Each PR carries exactly one content probe, and
a hit skips all of that PR's commits. A partially-merged or cherry-picked upstream tree satisfies
the probe while still lacking the later commits: EP_EFA_MAX_QPS can be present (DeepEP#612's first
commit) while the get_rdma_gbs() EFA fast path and the dispatch-interval change are absent;
ElasticBuffer can appear in fused_a2a.py (Megatron#4632's first commit) while the
num_experts-to-backward-dispatch fix is not. The marker then records every commit of that PR as
already-present, and verify-image.sh's patch-marker check — which probes the same single
needle — agrees. So the image reports a patch state it does not have.
Suggestion. Give each independently-required change its own probe, skip a PR only when all
of its probes pass, and post-assert the full list before writing the marker. That also resolves the
docstring question above: with per-change probes, the per-commit reverse-check becomes redundant and
can go.
There was a problem hiding this comment.
Fixed — each independently-required change now has its own probe, a PR is skipped only when EVERY probe passes, and the full list is post-asserted. patches/apply_nemo_rl_patches.py docstring (lines 27-37): "ALL of a PR's probes are checked: only if EVERY probe... a PR partially present... is NOT skipped; its commits are applied", and "Post-asserted, on the full list. After a PR applies, EVERY one of its probes is re-checked before the marker is written." The probes field (line 59) is "ONE per independently-required" change, "deliberately per-CHANGE, not per-PR" (line 61) — with the reasoning citing exactly your EP_EFA_MAX_QPS-present-but-get_rdma_gbs()-absent example. The per-commit reverse-check is noted as redundant under the all-probes decision (line 35). Thanks — this was the sharpest of the batch.
KeitaW
left a comment
KeitaW
left a comment
There was a problem hiding this comment.
Review Batch 6/6 — Image hygiene, manifests, positives and sources
raycluster.yaml is careful — the ray.io/node-type anti-affinity uses the label KubeRay actually
applies (and sets it on the template too), Guaranteed QoS carries its CPU-proxy rationale,
privileged: true carries its gdrdrv rationale, and NCCL_SOCKET_IFNAME uses the exclusion form.
Most of what follows is in the smaller files.
Things That Look Great
Checked rather than assumed, all on 2026-08-25:
- The opt-in patch layer does exactly what it claims. I replayed the script's logic against
each pinned base: all 9 commit SHAs resolve (https://github.com/<repo>/commit/<sha>.patch
→ HTTP 200, validFrom-prefixed git patches), and all four series apply--check-clean —
DeepEP#612 ×3 onto01dc3aa, Megatron#4632 ×4 onto19deef67, NeMo-RL#2410/#2411 onto
cc75cad— with every post-assert satisfied (EP_EFA_MAX_QPSinelastic.py,ElasticBuffer
×11 infused_a2a.py, theaws-efa-grpo-…-megatron.yamlrecipe present,b306af0×4 in
pyproject.toml). I also checked the #2411 needle is absent at the pinned base, so its
self-neutralizer can't misfire. git fetch origin <sha>rather thanrefs/pull/N/head. This is the immutable-pin form, and
it holds: fetch-by-SHA succeeds againstaws/aws-ofi-ncclfor a commit not on any branch.- The draining
$(… | grep -c …)form in the plugin asserts, rather thangrep -qin a
pipeline underpipefail— avoids the SIGPIPE-141 false failure on longnm -Doutput. - Every DeepEP call in
probe_rollout.pymatches the pinned API.ElasticBuffer.__init__
acceptsnum_allocated_qpsandexplicitly_destroy;num_bytes/num_rdma_ranks/
num_nvlink_ranks/destroy()all exist;dispatchreturns the 5-tuple you unpack and takes
num_sms/num_qps;combinereturns the 3-tuple you unpack. - The explicit-SM/QP workaround is complete, including the half that's easy to miss. Passing
num_smstodispatchalone would be a half-fix ifcombineauto-sized — butcombineat this
pin takesnum_sms = handle.num_sms(elastic.py:1086), so it inherits your explicit value and
never reachesget_theoretical_num_sms/ theibstat-basedget_rdma_gbs(). Both auto-sizers
are genuinely bypassed. - The trap-2 diagnosis is correct at the pin.
get_rdma_gbs()really does shell out to
ibstat(deep_ep/utils/envs.py:258), which doesn't enumerate EFA'srdmap*s0HCAs, and the
auto path really can reach 129 QPs against a 128-slot ring (elastic.py:328-334), with
num_sms*16+1atelastic.py:851. - MIT-0 header on all 15 new files — including
.gitignore, both YAMLs,env_vars.exampleand
the Dockerfile, which is the file-type split this repo's reviews keep catching. Trailing newline
on all 15 too. - The Qwen3-30B-A3B shape claims check out against the model's
config.json:hidden_size
2048,moe_intermediate_size768,num_experts128,num_experts_per_tok8 — matching
env_vars.exampleandtrain_moe_step.py'smoe_ffn_hidden_size=768. train_moe_step.py's DDP-less loop is sound, and the gate is what makes it sound. With an
identical seeded batch on every rank, gradients for the replicated (non-expert) params are
identical, so the DP all-reduce really is a no-op — and the cross-rank loss-agreement gate is
what would catch it if that reasoning ever stopped holding. Nice.MOE_DISPATCHER=flexis refused, not silently downgraded, when the patch marker is absent —
with the reason and the rebuild command in the message.verify-image.shhas no unconditional PASS, and the rollout probe distinguishes
PASS / FAIL / INCONCLUSIVE (exit 2) rather than calling a counter-moved-but-no-banner run a
pass. The EFA hw-counter delta assert is the right kind of evidence for "the bytes really left
the node."- The measured-vs-staged section and Known limitations are unusually straight, including
naming which of the four draft PRs are closed-unmerged and stating plainly that no performance
numbers are published. This is the right instinct and I'd keep the section — the one thing to
reconcile is that "build-staged" and Batch 1 can't both be describing this tree, and the pin set
that did build is worth recording here.
Sources
Upstream source, read at the exact pins in this PR
- DeepEP
01dc3aa:deep_ep/__init__.py(topk_idx_texport),deep_ep/buffers/elastic.py
(ElasticBuffer.__init__/dispatch/combinesignatures; auto-QP at 328-334;
get_theoretical_num_qpsat 851;combine'shandle.num_smsat 1086),
deep_ep/utils/envs.py:246-264(get_rdma_gbsviaibstat) —
https://github.com/deepseek-ai/DeepEP/tree/01dc3aaac82068020353dce2c302e38153c0bfaa - NCCL
v2.30.4-1vsv2.31.2-1:src/include/nccl_device/core.h—ncclGinType_tgains
GPI=4,EFA_GDA=5only in 2.31 — https://github.com/NVIDIA/nccl amazon-contributing/DeepEPmain=01dc3aa+ 8 commits (78ae4e5csysfs link-rate probe,
6a59654c/8e7b42e9GIN context + QP allocation) —
https://github.com/amazon-contributing/DeepEP/commits/main
Repo precedent
micro-benchmarks/expert-parallelism/deepep-v2-benchmark/{README.md,deepep.Dockerfile,setup_deepep_gin.sh,slurm/test-internode.sbatch}
(merged in #1234, 2026-08-24) — the EFA-GDANCCL_GIN_TYPE=5stack, the CPU-proxy caveat, and
the reusable V2 build script.github/workflows/deepep-vendor-sync.yml— the gate that coverssetup_deepep_efa.sh(V1), not
setup_deepep_gin.sh3.test_cases/pytorch/nemo-rl/deepep-v2-efa/kubernetes/raycluster.yaml— thesecretKeyRefand
nodeAffinity patterns the data-prep pod should mirror
Upstream metadata at the pinned SHAs, verified live 2026-08-25
- NeMo-RL
cc75cadpyproject.toml:requires-python = ">=3.13.13";torch==2.10.0,
ray[default]==2.54.0,transformers==5.3.0,pillow>=12.1.1,mlflow>=3.11.1, plus
decord2/soundfile>=0.13.1/nccl4py/cuda-bindings—
https://github.com/NVIDIA-NeMo/RL/blob/cc75cadfe061301bd121306f1648957583760528/pyproject.toml - NeMo-RL
cc75cadnemo_rl/distributed/virtual_cluster.py:47-65— every worker py_executable is
uv run --locked --extra … --directory <git_root>;nemo_rl/utils/venvs.py:96runsuv sync
first;uv.lockis committed (1.3 MB). Commit711147f(#2411) touchespyproject.tomlonly. - DeepEP
01dc3aasetup.py:92-124—find_nvshmem_root()is called unconditionally under a
# TODO: make NVSHMEM and legacy optional, and the NVSHMEM sources/links are unconditional;
find_pkg_rootdefaultsoptional=False. - aws-ofi-nccl at
9c44d34+c2e773d,include/nccl_ofi_param.h— the GIN parameters that exist
areGIN_CQ_PROCESS_MAX_ITER,GIN_GDAKI,GIN_STRONG_SIGNALand (from the cherry-pick)
GDRCOPY_FORCED_PCIE_COPY, defaultfalse. There is noGIN_MAX_REQUESTS;OFI_NCCL_PROTOCOL
andOFI_NCCL_GIN_GDAKIdo exist. - File modes in this PR: all 15 files are
100644(git ls-tree -r), including every
recipe/*.sh. env_vars.exampleexportsNUM_NODES;recipe/run-rollout-probe.sh:19and
recipe/train-step.sh:19readNNODES;kubernetes/raycluster.yamldefines none of
NNODES/GPUS_PER_NODE/EP_*in the container env.
Verified live, 2026-08-25
gh api repos/aws/aws-ofi-nccl/pulls/1351→state: closed,merged: false, title
"gdrcopy: add OFI_NCCL_GDRCOPY_FORCED_PCIE_COPY override…", created 2026-08-13- Commit dates:
9c44d342026-05-25 ·c2e773d2026-08-13 ·01dc3aa2026-08-04 ·
19deef672026-08-19 ·cc75cad2026-05-05 · NCCLv2.30.4-12026-04-22 git fetch origin 9c44d34…+git cherry-pick c2e773d…→ clean; both build asserts satisfied- All 9 patch SHAs → HTTP 200 + clean
git apply --checkonto their pinned bases; all post-asserts
pass nvcr.io/nvidia/pytorch:26.02-py3image config (registry API):NCCL_VERSION=2.29.stable.20260109,
AWS_OFI_NCCL_VERSION=1.17.0,OPAL_PREFIX=/opt/hpcx/ompi,EFA_VERSION=1.43.1,
PYTORCH_BUILD_VERSION=2.11.0a0+eb65b36,CUDA_VERSION=13.1.1.006Qwen/Qwen3-30B-A3Bconfig.json→ hidden 2048 / moe_intermediate 768 / 128 experts / top-k 8- Merge-base of this branch and
main=9baf5aa7("Add DeepEP V2 benchmark (NCCL GIN) (#1234)")
| # Same removal the slime sibling does: the EFA installer below provides | ||
| # libfabric + Open MPI, and a leftover HPC-X/UCX on the loader path is a | ||
| # classic source of wrong-transport surprises. | ||
| RUN apt-get update -y && apt-get install -y --no-install-recommends \ |
There was a problem hiding this comment.
|| true at the end of this chain swallows failures from apt-get update and apt-get install too
Observation. The instruction is apt-get update && apt-get install … && apt-get remove … || true.
&& and || have equal precedence and associate left to right, so this parses as
((update && install) && remove) || true — the tolerance intended for the removal (where an
already-absent package is fine) covers the whole chain.
Impact. A transient mirror failure or a renamed package in apt-get install yields exit 0, and
the build proceeds without cmake, autoconf, libtool or libhwloc-dev. The failure then
surfaces several hundred lines later inside the gdrcopy or aws-ofi-nccl build as a missing-tool
error with no connection to its cause. (The merged deepep-v2-benchmark Dockerfile has the same
idiom, but with only update && remove in the chain — the added install is what makes it
load-bearing here.)
Suggestion. Scope the tolerance to the removal:
| RUN apt-get update -y && apt-get install -y --no-install-recommends \ | |
| RUN apt-get update -y && apt-get install -y --no-install-recommends \ |
…and close the chain with && { apt-get remove -y --allow-change-held-packages ibverbs-utils libibverbs-dev libibverbs1 libmlx5-1 || true; } so only that step may fail.
There was a problem hiding this comment.
Fixed in 316c127. The chain was update && install && remove || true, which parses as ((update && install) && remove) || true — the || true swallowed update/install failures too. It is now brace-scoped to the removal only: ... && { apt-get remove -y ... || true; } (Dockerfile:122-123), so only the intended already-absent-package removal is tolerated; a real update/install failure still fails the build.
| libhwloc-dev pkg-config \ | ||
| && apt-get remove -y --allow-change-held-packages \ | ||
| ibverbs-utils libibverbs-dev libibverbs1 libmlx5-1 || true | ||
| RUN rm -rf /opt/hpcx/ompi /usr/local/mpi /usr/local/ucx && ldconfig |
There was a problem hiding this comment.
OPAL_PREFIX is left pointing at the directory this line deletes
Observation. The NGC base sets OPAL_PREFIX=/opt/hpcx/ompi in its image config (verified live
from the registry, 2026-08-25). This line removes /opt/hpcx/ompi, and nothing resets the
variable — so the final image ships OPAL_PREFIX pointing at a path that no longer exists, while
Open MPI actually lives at /opt/amazon/openmpi (which you correctly put on PATH in Layer 2).
Open MPI uses OPAL_PREFIX to locate its MCA component tree.
Impact — deliberately small, and worth stating as such. Nothing in the recipe uses MPI: the
gates launch through torchrun and Ray. So this bites only someone who runs mpirun from the
image, which the docs never ask for. Reporting it as a leftover rather than a defect.
Suggestion.
| RUN rm -rf /opt/hpcx/ompi /usr/local/mpi /usr/local/ucx && ldconfig | |
| RUN rm -rf /opt/hpcx/ompi /usr/local/mpi /usr/local/ucx && ldconfig | |
| ENV OPAL_PREFIX=/opt/amazon/openmpi |
There was a problem hiding this comment.
Fixed in 316c127. The base sets OPAL_PREFIX=/opt/hpcx/ompi and line 129 removes that dir; nothing reset it, so the final image shipped a dangling OPAL_PREFIX. Added ENV OPAL_PREFIX=/opt/amazon/openmpi (Dockerfile:130) — the actual OMPI location — right after the removal; the comment at 124-128 explains it.
| # SPDX-License-Identifier: MIT-0 | ||
|
|
||
| # Local environment file with filled-in secrets/values — never commit | ||
| env_vars |
There was a problem hiding this comment.
No .dockerignore while the build context is the directory holding env_vars
Observation. env_vars is correctly gitignored and carries HF_TOKEN plus your AWS account
id. README §2 then builds with the context set to this directory
(docker build -f nemo-rl.Dockerfile -t ${FULL_IMAGE} .). .gitignore does not apply to Docker
build contexts, so the filled-in env_vars is uploaded to the daemon — and to any remote/CI
builder — on every build.
Impact, scoped honestly: there is no COPY . . in this Dockerfile (every COPY names a
specific file), so nothing lands in an image layer and nothing is pushed to ECR. The exposure is
the context upload itself, which matters for a remote builder and for anyone who later adds a
broad COPY.
Suggestion. Add a .dockerignore beside the Dockerfile:
env_vars
*.log
__pycache__/
There was a problem hiding this comment.
Fixed in 4052250. Added .dockerignore (your point exactly: .gitignore does not apply to a Docker build context, so the filled-in env_vars — HF token + AWS account id — would upload to the daemon and any remote/CI builder on every docker build .). It ignores env_vars, *.log, __pycache__/. No COPY in the Dockerfile is broad today, but this closes the context-upload exposure and guards a future broad COPY.
| ], | ||
| # Metadata-only for this image (deep_ep is built from /opt/DeepEP, not | ||
| # from NeMo-RL's pin), but applied so the tree is self-consistent. | ||
| "probe": ("pyproject.toml", "b306af0"), |
There was a problem hiding this comment.
NeMo-RL#2411 edits pyproject.toml without uv.lock, and every worker launches with uv run --locked
Observation. This entry is annotated "Metadata-only for this image (deep_ep is built from
/opt/DeepEP, not from NeMo-RL's pin), but applied so the tree is self-consistent." At the pinned
cc75cad, NeMo-RL launches each worker type through a locked uv environment:
BASE = f"uv run --locked --directory {git_root}"
MCORE = f"uv run --locked --extra mcore --directory {git_root}"
VLLM = f"uv run --locked --extra vllm --directory {git_root}"(virtual_cluster.py:47-65;
venvs.py:96 runs uv sync first). uv.lock is committed (1.3 MB), and
commit 711147f
touches pyproject.toml only — verified live 2026-08-25, its diff has exactly one diff --git
stanza.
Impact. uv run --locked refuses to run when the lock does not match pyproject.toml, so on
the patched image the GRPO actors would fail at worker startup rather than "metadata-only". And the
deeper consequence is worth checking on the same pass: if the lock is regenerated, those isolated
per-worker environments resolve deep_ep from NeMo-RL's own git dependency — not the EFA-patched
/opt/DeepEP this Dockerfile spent Layer 9 building — which would bypass the component the whole
folder exists to exercise.
Suggestion. Regenerate uv.lock as part of this patch entry (and post-assert it, the way the
other entries post-assert their content probe), or point the worker environments at the system
interpreter so /opt/DeepEP is what actually loads. Either way the "metadata-only" note should go —
this entry is on the critical path for the flavor it belongs to.
There was a problem hiding this comment.
Fixed — NeMo-RL#2411 is removed from the apply set entirely, which dissolves the uv run --locked problem. apply_nemo_rl_patches.py:11: "NVIDIA-NeMo/RL#2411... is intentionally excluded". The image now pins NEMO_RL_SHA=46be4e8 and builds deep_ep from /opt/DeepEP (not NeMo-RL's git dependency), so there is no pyproject.toml/uv.lock mismatch to fail uv run --locked at worker startup, and no path by which the isolated per-worker envs would resolve a second deep_ep. You were right that "metadata-only" was wrong — it was on the critical path for this flavor — so it's dropped rather than annotated. Thanks.
| spec: | ||
| containers: | ||
| - name: data-prep | ||
| image: python:3.12-slim |
There was a problem hiding this comment.
Floating python:3.12-slim in a PR whose own env file says "never latest"
Observation. python:3.12-slim is a moving tag — it re-points on every CPython 3.12 patch
release and every Debian base refresh. env_vars.example in this same folder states the rule for
the image it does pin: "Immutable tag - never 'latest': with imagePullPolicy: IfNotPresent a node
that cached 'latest' silently keeps running the OLD image after a rebuild." The same reasoning
applies here.
Suggestion. Pin to a patch release (or a digest):
| image: python:3.12-slim | |
| image: python:3.12.14-slim |
There was a problem hiding this comment.
Fixed — pinned to a patch release. kubernetes/data-prep-pod.yaml:42 now reads image: python:3.12.14-slim. You were right that a floating python:3.12-slim contradicts this folder's own "never latest" rule (with imagePullPolicy: IfNotPresent, a node that cached the moving tag silently keeps the old image after a re-push). Thanks.
| - name: HF_TOKEN | ||
| value: "" # Set before applying, or pass via kubectl set env |
There was a problem hiding this comment.
HF_TOKEN as a plaintext env on a sleep infinity pod, while the sibling manifest uses a Secret [confirmed]
Observation. Three things converge on this line:
raycluster.yamlinjects the same token viasecretKeyReffrom thehf-tokenSecret that
README §4 creates — so the good pattern already exists two files away, and this manifest doesn't
use it.- A plaintext
envvalue is visible inkubectl get pod -o yamland in theenvsubst-rendered
output. Becausecommand: ["sleep","infinity"], that value sits in etcd for the life of the
pod rather than the life of a Job. - As written it can't work anyway: the value is a literal
""with no${HF_TOKEN}placeholder,
so the documentedenvsubst < kubernetes/data-prep-pod.yaml | kubectl apply -f -flow leaves it
empty and thehf downloadin the header comment fails on a gated model. The comment's escape
hatch — "or pass viakubectl set env" — doesn't rescue it either: a Pod's containerenvis
immutable once created, sokubectl set env pod/data-prep …is rejected and the pod has to be
deleted and re-applied.
One more thing worth deciding while you're here: the staging target is ${MODEL_LOCAL}
(/fsx/models/Qwen3-30B-A3B), but the GRPO recipe this folder points at sets
policy.model_name: Qwen/Qwen3-30B-A3B — a hub id, not that path — so a --local-dir download is
not what the run consumes unless you also set HF_HOME on the Ray pods or override model_name.
Suggestion. Reuse the Secret the README already tells the operator to create:
| - name: HF_TOKEN | |
| value: "" # Set before applying, or pass via kubectl set env | |
| - name: HF_TOKEN | |
| valueFrom: | |
| secretKeyRef: | |
| name: hf-token | |
| key: HF_TOKEN |
There was a problem hiding this comment.
Fixed — HF_TOKEN now comes from the hf-token Secret via secretKeyRef, matching raycluster.yaml. kubernetes/data-prep-pod.yaml:48-52:
- name: HF_TOKEN
valueFrom:
secretKeyRef:
name: hf-token
key: HF_TOKENYou were right on all three points — the good pattern already existed two files away, the plaintext value sat in etcd for the pod's life on a sleep infinity pod, and the literal "" with no ${HF_TOKEN} placeholder meant the documented envsubst flow left it empty anyway (and pod env is immutable, so kubectl set env couldn't rescue it). Reusing the Secret fixes all of them. Thanks.
| namespace: ${NAMESPACE} | ||
| labels: | ||
| app: nemo-rl-deepep-data-prep | ||
| spec: |
There was a problem hiding this comment.
The data-prep pod can land on a GPU node and hold it (and installs an unpinned dep at run time)
Observation. Two smaller things in this spec, both by contrast with raycluster.yaml:
- Scheduling. The Ray head carries a
nvidia.com/gpu.present NotIn ["true"]nodeAffinity with
the rationale "keep the head off GPU nodes so it never blocks a GPU worker from scheduling."
This pod is the same kind of CPU-only workload, requests 4–8 CPU and 16–32 Gi, and runs
sleep infinity— but has no such constraint, so on a cluster with schedulable capacity it can
park on a p5 node for as long as the operator leaves it up. Copying the head's affinity block
would settle it. - Runtime install. The header comment stages via
pip install "huggingface_hub[cli]"— unpinned, at run time, inside the pod. On an air-gapped or
egress-restricted cluster that step is the one that fails, and it's the only thing this pod
exists to do. A pinnedhuggingface_hub[cli]==x.y.z(or an image that already carries it) makes
the staging step reproducible.
There was a problem hiding this comment.
Fixed in 4052250. The CPU-only data-prep pod now carries the same nvidia.com/gpu.present NotIn true nodeAffinity as the Ray head (data-prep-pod.yaml:29-34), so it cannot land on and hold a GPU node, and it is pinned to python:3.12.14-slim (line 42) instead of installing an unpinned dependency at runtime.
| - name: HF_TOKEN | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: hf-token |
There was a problem hiding this comment.
HF_TOKEN is injected into every privileged GPU worker, and the SA token is auto-mounted alongside it
Observation. Every GPU worker gets HF_TOKEN from the Secret (good — via secretKeyRef), and
those same containers run privileged: true with IPC_LOCK and a /dev/infiniband host mount.
The pod spec does not set automountServiceAccountToken: false, so the default ServiceAccount
token is mounted too.
Impact. The privilege itself is justified and documented — the manifest header explains the
gdrdrv device-cgroup problem and names the device-plugin alternative, which is exactly the rationale
this repo asks for, so I'm not asking you to drop it. The point is the combination: model code and
training dependencies executing in a privileged root container is the least contained place in the
cluster to also hand a credential, and the recipe gates that this manifest is primarily for need no
token at all (no weights, no dataset).
Suggestion. Two small scopings, both compatible with keeping privileged: true:
- Move
HF_TOKENoff the worker group and onto the data-prep workload that actually downloads
weights (see the data-prep comments) — or gate it behind the full-GRPO overlay. - Add
automountServiceAccountToken: falseto both pod specs; nothing here talks to the API server.
There was a problem hiding this comment.
Fixed in 5df9552. automountServiceAccountToken: false is now set on both the head and worker pod specs (raycluster.yaml:52, 129), so the service-account token is no longer mounted into the privileged GPU workers alongside the HF token. The HF token itself is now sourced from a secretKeyRef (lines 71-75, 143-147) rather than injected as a literal env value.
…ates on p5en Run-to-green on a live p5en (8xH200, EFA): the image failed to build on the pinned NeMo-RL SHA, and once buildable it failed its own recipe/verify-image.sh on two counts. All fixed and verified against a pristine-rootfs in-pod replay of Dockerfile layers 1-9 from these exact files -> full 6/6 gate suite now PASS (efa-direct x4 rails, GIN-capable NCCL 2.30.4 at /opt/nccl/build, ncclGinPlugin + gdrcopy-compiled-in, deep_ep V2 ElasticBuffer import, nemo_rl + megatron.core imports, clean upstream-only baseline marker). cgk p5en, 2026-08-26. Build fix -- NEMO_RL_SHA cc75cad -> 46be4e8 (+ drop draft PR #2411): cc75cad bumped requires-python to ">=3.13.13" and torch to 2.10.0, so `pip install -e /opt/NeMo-RL` (Layer 7) hard-fails the interpreter check on this py3.12 NGC base (--no-deps does NOT suppress the Requires-Python floor). 46be4e8 is the parent commit of draft PR NVIDIA-NeMo/RL#2410 (the EFA recipe), declares requires-python ">=3.12", and is the revision requirements.txt is generated from. #2411 (a deep_ep pin bump based on cc75cad) is therefore dropped from the opt-in layer -- it neither applies to the 46be4e8 tree nor belongs on py3.12, and is metadata-only here (deep_ep builds from /opt/DeepEP, not NeMo-RL's pin). The opt-in layer is now 3 draft PRs, not 4; docs, the patches script, env_vars.example, and the recipe scripts are reconciled to match. Build fix -- restore nvidia-nvshmem-cu13 in requirements.txt: upstream DeepEP's setup.py links NVSHMEM UNCONDITIONALLY (find_nvshmem_root (optional=False) asserts if absent -- its "make NVSHMEM optional" TODO is still open at 01dc3aa). The NGC base's baked NVSHMEM is the dpkg runtime package with no nvshmem.h and no libnvshmem_device.a, so the Layer-9 deep_ep build fails at link without the pip wheel (which ships both). cu13 to match the base's CUDA line; this mirrors NeMo-RL 46be4e8's own `nvidia-nvshmem-cu12 # for deep_ep build`. Gate 4 -- deep_ep import (nvshmem runtime ABI), new Layer 7b: `from deep_ep import ElasticBuffer` died with `undefined symbol: nvshmem_selected_device_transport, version NVSHMEM`. deep_ep/_C.so is correctly built against the pip nvidia-nvshmem-cu13 wheel (3.7.x), but torch/lib/libtorch_nvshmem.so -- imported first -- has a NEEDED libnvshmem_host.so.3 whose RUNPATH ends in /usr/local/cuda/lib64, where the NGC base's OLDER dpkg NVSHMEM (3.4.x, lacking that symbol) lives. RUNPATH outranks ld.so.cache, so the stale copy wins by soname before _C.so can pull the right one -- which is why an ld.so.conf.d entry (Layer 4's house style) does NOT fix it. Fix (Layer 7b): symlink the wheel's lib dir to a stable, version-agnostic /opt/nvshmem-pip-lib and prepend it to LD_LIBRARY_PATH (only LD_LIBRARY_PATH outranks RUNPATH). Fail-loud if the wheel dir is absent. Gate 5 -- nemo_rl / megatron.core imports (missing requirements): `import nemo_rl.algorithms.grpo` and `import megatron.core.transformer.moe.fused_a2a` both ImportError'd. - decord: imported by nemo_rl/data/multimodal_utils.py (GRPO data path); absent from the NGC base, an extra in NeMo-RL's own deps. - nvidia-resiliency-ext>=0.6.0: import-time dep of megatron.core's dist_checkpointing strategies; the base ships 0.5.0, which is too old. setup script -- deep_ep .so assert made cwd-independent: the post-install assert used importlib.util.find_spec("deep_ep") while cwd was the DeepEP source tree; Python puts cwd on sys.path, so find_spec resolved the SOURCE package (no compiled .so) and shadowed the pip-installed copy, a false negative that failed the build even when the .so was built. Rewrote to read the installed distribution's file manifest (importlib.metadata.files), which is cwd-independent and imports nothing. Verified: pristine-rootfs in-pod replay of layers 1-9 from these exact files, then recipe/verify-image.sh -> ALL IMAGE GATES PASS. cgk p5en, 2026-08-26. Signed-off-by: Anton Alexander <dmvevents@gmail.com>
… (DDP-free), + shell bug
The Megatron-core MoE train-step gate (recipe/train_moe_step.py) now PASSES at
full 8-GPU scale on a live p5en (8xH200, EFA): loss 9.43 -> 6.33 -> 4.17 -> 3.43
over real optimizer steps with the alltoall dispatcher (NCCL all-to-all over
EFA), cross-rank loss agreement to ~1e-5. Two correctness fixes, both measured.
train_moe_step.py -- gate 2 (cross-rank loss agreement) false-failed on a fully
correct image + transport because the DDP-free loop never synced the REPLICATED
(non-expert) params:
- init: megatron seeds most replicated params identically via its RNG tracker,
but router.weight and position_embeddings init under a per-rank-varying RNG
context -- MEASURED cross-rank divergence 1.52e-1 / 1.64e-1 at step 0, before
any update (diag all-reduce MAX-MIN, split expert vs replicated by param name;
qkv/proj/layernorm/word_embeddings were already 0.0). Fix: broadcast the
non-expert params from rank 0 after construction (what DDP does at build).
- per step: the old docstring claimed "same batch => grad all-reduce is a
no-op". FALSE for replicated params under EP -- each rank backprops through
DIFFERENT local experts, so the shared router gets different per-rank grads
and re-diverges every step (spread grew 5.7e-4 -> 8.9e-3 over 3 steps with
init-broadcast alone). Fix: all-reduce(AVG) replicated-param grads each step.
Result: spread FLAT ~1e-5 across 5 steps -> the gate is a real transport test
at any step count, not a rubber stamp. Expert params/grads are deliberately
left per-rank distinct -- that asymmetry IS expert parallelism.
Also: fp32 params throughout (not bf16). The `local` layer spec emits fp32
LayerNorm activations; with bf16 params the router's te_general_gemm sees a
bf16-weight x fp32-input GEMM that this NGC base's TE cuBLASLt build rejects
("unsupported value or parameter") regardless of moe_router_dtype. fp32 makes
every GEMM (fp32,fp32,fp32) and keeps the all-to-all transport identical
(NCCL/DeepEP all-to-all is dtype-agnostic; DeepEP requires fp32 probs).
train-step.sh -- the usage string `${1:?usage: ... {leader|worker} ...}` has a
literal `}` inside the `${1:?word}` expansion, which terminates the parameter
expansion early; $ROLE became the literal "leader <leader-ip> [node-rank]}" and
the leader invocation aborted with "unrecognized role". Removed the interior
brace from the message. (bash-parse-only fix; no behavior change on the happy
path other than making the leader path actually run.)
Verified: single-node 8xH200 over EFA on cgk p5en, 2026-08-26.
Signed-off-by: Anton Alexander <dmvevents@gmail.com>
|
Thanks for the review, @KeitaW. Both the build blocker and the test correctness are now Build (was: "as pinned, the image does not build") — fixed. Test — actual training, not just imports. Two correctness fixes the run surfaced (both measured, not guessed):
The alltoall gate above runs on the upstream-only image. The |
…dening Addresses KeitaW review threads on PR awslabs#1242: - Add .dockerignore beside nemo-rl.Dockerfile (env_vars, *.log, __pycache__/). .gitignore does not apply to a Docker build context, so the filled-in env_vars (HF_TOKEN + AWS account id) was uploaded to the daemon / any remote builder on every `docker build .`. No broad COPY exists so nothing reached an image layer; this closes the context-upload exposure. (thread PRRT…M3nJ) - data-prep-pod.yaml: * HF_TOKEN plaintext env -> secretKeyRef from the same hf-token Secret raycluster.yaml already consumes (a literal "" could not work anyway, and a plaintext value would sit in etcd for the sleep-infinity pod's life). (thread PRRT…M3nY) * pin python:3.12-slim -> python:3.12.14-slim (never a floating tag — the "never latest" rule env_vars.example states). (thread PRRT…M3nU) * add the head's nvidia.com/gpu.present NotIn ["true"] nodeAffinity so this CPU-only sleep-infinity pod cannot park on and hold a GPU node; pin the header's `pip install "huggingface_hub[cli]==1.28.0"`; add automountServiceAccountToken:false (nothing here talks to the API server). (thread PRRT…M3nh) Signed-off-by: Anton Alexander <dmvevents@gmail.com>
…SA token Addresses KeitaW review threads on PR awslabs#1242: - Put the recipe-gate shape in the worker pod env: NNODES (the name the scripts read — NOT NUM_NODES, which only raycluster's replicas consume) + GPUS_PER_NODE + EP_EXPERTS/EP_TOPK/EP_HIDDEN/EP_TOKENS/EP_NUM_SMS/EP_NUM_QPS. A client-side `source env_vars` does not cross `kubectl exec`; without these the gates ran on hardcoded script defaults. (thread PRRT…M3YG) - Default /fsx to emptyDir on both head and workers (PVC form left documented inline for the full GRPO swap). The recipe gates touch no shared storage, so the advertised cheap entry point no longer sits Pending on an unbound claim. (thread PRRT…M3YL) - Add automountServiceAccountToken:false to both pod specs — nothing here talks to the API server, and the workers are privileged root containers running model/training code. (thread PRRT…M3nj) - Drop OFI_NCCL_GIN_MAX_REQUESTS from the worker env: no such parameter exists in the pinned aws-ofi-nccl (the GIN params are gin_cq_process_max_iter, gin_gdaki, gin_strong_signal, gdrcopy_forced_pcie_copy). It was inert and contradicted trap 2's 128-slot ring. (thread PRRT…M3R0) Signed-off-by: Anton Alexander <dmvevents@gmail.com>
…gate, teardown Addresses KeitaW review threads on PR awslabs#1242: probe_rollout.py: - Score combine per token against its OWN scale (row_err/row_scale, 1e-3 floor) instead of one global max — a global normaliser averages away the badly-wrong low-amplitude rows a partial dispatch / dropped QP channel produces, exactly what the probe exists to catch. (thread PRRT…M3fS) - deep_ep.topk_idx_t direct access instead of getattr(...,torch.int64): the attribute is exported unconditionally at the pin, so the default was unreachable and would silently mask a future rename. (thread PRRT…M3fx) - Restrict the local-mapped detector to RANK>0: on rank 0 the local experts ARE 0..num_local-1, so it fired every normal run and printed a misleading note. (thread PRRT…M3fs) train_moe_step.py: - Track max_spread across ALL steps for gate 2 (was reading the final step only), so an early transport fault that later clears still fails; assert STEPS>=2. (thread PRRT…M3fb) both: - Pass timeout=10min to init_process_group and destroy the group in a finally so a raised rank fails fast instead of holding peers for the 30-min NCCL default (the worker runs detached under kubectl exec). (thread PRRT…M3f3) Signed-off-by: Anton Alexander <dmvevents@gmail.com>
…t, train-step transport gate - run-rollout-probe.sh / train-step.sh: sum tx_bytes+send_bytes+rdma_write_bytes+ rdma_read_resp_bytes for the EFA-TX assert, not tx_bytes alone. Under FI_EFA_USE_DEVICE_RDMA=1 the bytes land on rdma_write_bytes, so a tx_bytes-only check false-FAILs a healthy RDMA run (KeitaW review). - both: wrap torchrun in 'timeout 900' so a wedged rendezvous returns a verdict instead of holding the nodes; pairs with the python init_process_group timeout. - train-step.sh: port the probe's three-part transport gate (rc + >=1MiB EFA-TX delta + efa provider banner) instead of PASS on rc==0 alone; default NCCL_DEBUG to INFO so the provider banner is available as proof. Signed-off-by: Anton Alexander <dmvevents@gmail.com>
… check Addresses KeitaW review on the draft-PR patch layer and the image gate. patches/apply_nemo_rl_patches.py: - Replace the single per-PR content probe with a per-CHANGE probe LIST (one needle per independently-required commit). A PR is skipped as already-present only when EVERY probe passes; a partially-merged upstream tree (some probes pass, some don't) is NOT skipped — its commits apply and a colliding hunk fails --check loud, instead of silently trusting a single needle hit. Post-assert re-checks the FULL list before the marker is written, so the marker can never record a patch state the tree lacks. - Drop the per-commit `git apply --reverse --check` short-circuit (it false-negatives on multi-commit PRs whose later commits touch the same hunks); the whole-PR probe gate now decides skip-vs-apply. - Needles derived from the real fetched .patch files: DeepEP#612 = EP_EFA_MAX_QPS / EP_EFA_RDMA_GBS / kScaleoutUpdateInterval=16; Megatron#4632 = ElasticBuffer / deep_ep.utils.event / _handle_num_experts (commit 4 is a comment/URL cleanup, no probe by design). recipe/verify-image.sh: - Patch-marker check now probes per-change too (adds EP_EFA_RDMA_GBS in utils/envs.py and _handle_num_experts in fused_a2a.py), covering the two named false-positive cases where a single needle passes but a later required change is absent. - Resolve libnccl via the actual loader (dlopen + /proc/self/maps) instead of `ldconfig -p | head -1` (cache order). dlopen honors LD_LIBRARY_PATH before the cache — the same evidence deep_ep.check_nccl_so() acts on — so this catches a scrubbed LD_LIBRARY_PATH or LD_PRELOAD shadow the cache-order check reads past. Signed-off-by: Anton Alexander <dmvevents@gmail.com>
… drop unused GIN knob Addresses KeitaW review on the Dockerfile and env contract. nemo-rl.Dockerfile: - Brace-scope the `|| true` to the `apt-get remove` of the distro verbs packages ONLY. apt-get update + install stay fail-hard — a transient mirror miss must not silently ship an image missing cmake/autoconf/libtool and fail 100s of lines later inside the gdrcopy/aws-ofi-nccl build. - Set OPAL_PREFIX=/opt/amazon/openmpi after `rm -rf /opt/hpcx/ompi`. The NGC base points OPAL_PREFIX at the deleted HPC-X Open MPI; reset it to the EFA installer's Open MPI so a dangling prefix can't trip an mpirun from the image. Drop OFI_NCCL_GIN_MAX_REQUESTS=512 everywhere it appeared (Dockerfile runtime ENV, env_vars.example, recipe/run-rollout-probe.sh, recipe/train-step.sh): it is not a knob the measured NCCL-GIN substrate set, and carrying an unverified value in the shipped transport contract is exactly the kind of guessed pin the review flagged. The remaining GIN vars (NCCL_GIN_TYPE/ENABLE, OFI_NCCL_GIN_GDAKI) are the measured ones. env_vars.example: - Add a "HOW THESE REACH THE GATES" note: sourcing sets vars in the local shell only and does NOT cross `kubectl exec`; `envsubst` substitutes NUM_NODES / the EP_* shape into the pod env: block, and the manifest bridges NUM_NODES->NNODES. - Correct the transport-contract header: a `kubectl exec` shell inherits these from the container env (baked + manifest), NOT from a client-side `source`; the local export line matters only for a non-k8s `docker run` on an EFA host. Signed-off-by: Anton Alexander <dmvevents@gmail.com>
…om PR awslabs#1242 review Truthfulness fixes for KeitaW's inline review — no measurement is invented; the edits make wording match what the build actually does and what was measured. README (folder): - NCCL row: state the NGC base bakes an OLDER NCCL line (2.29.x), so the source v2.30.4-1 build + 00-nccl-gin.conf ld.so.conf entry is LOAD-BEARING (makes the GIN-capable copy win the loader search), not a no-op 'same line' override. - aws-ofi-nccl row: describe 9c44d34 + PR#1351 as the GIN CPU-proxy lineage this folder standardises on; note the SHAs POSTDATE Wave-28 (not that run's pins). - trap 3: verify-image.sh resolves libnccl.so.2 via dlopen + /proc/self/maps, not ldconfig -p cache order. - runtime env table: drop OFI_NCCL_GIN_MAX_REQUESTS=512 row (the code no longer sets it — was removed from Dockerfile/scripts/env_vars; table was stale). - Quick Start: push the -draftprs flavor too (distinct tag); flex train-step must set MOE_DISPATCHER=flex on BOTH nodes (one torchrun job — ranks disagree and hang otherwise). - benchmarks refs: micro-benchmarks EP benchmark runs EFA-GDA GIN (NCCL_GIN_TYPE=5), NOT this folder's CPU-proxy (NCCL_GIN_TYPE=2) — drop the 'same substrate' claim in two places. README (parent nemo-rl/): same EFA-GDA-vs-CPU-proxy correction on the micro-benchmarks cross-link. setup_nemo_rl_deepep_efa.sh header: fix the 'links NO NVSHMEM' contradiction — deep_ep _C.so IS build-linked against NVSHMEM (Dockerfile Layer 7b makes the pip nvshmem win the loader search); NVSHMEM is just not the run-time transport on the V2 NCCL-GIN path. Retarget the no-vendor-sync contrast at the repo's canonical V2 builder setup_deepep_gin.sh (only the V1 setup_deepep_efa.sh copies are synced). Signed-off-by: Anton Alexander <dmvevents@gmail.com>
…iew)
KeitaW: the scripts ship mode 100644, so README §3's `recipe/verify-image.sh
${FULL_IMAGE}` fails with Permission denied straight from a fresh clone. Set
the executable bit on the three shell entrypoints the docs invoke directly
(verify-image.sh, run-rollout-probe.sh, train-step.sh) plus setup_nemo_rl_deepep_efa.sh,
via `git update-index --chmod=+x` so the mode lands in the tree. The two .py
files stay 100644 — they are run via torchrun/python, never executed directly.
Signed-off-by: Anton Alexander <dmvevents@gmail.com>
…ocument plugin/fork trade-offs (PR awslabs#1242 review) Address KeitaW review threads on pin provenance and honesty: - NGC base: pin @sha256:bbc2b67e… alongside the readable :26.02-py3 tag. It is the ABI anchor the whole image builds around (torch 2.11/CUDA 13 + baked TE/apex/flash-attn), so a silent tag re-push is the most consequential drift possible here. Override NGC_PYTORCH_BASE to move it (a re-measure event). - NCCL: pin the commit 1933fdd6 that v2.30.4-1 resolves to, not the bare tag — held to the same moving-ref standard as the gdrcopy/DeepEP/Megatron SHA pins. Correct the stale "same NCCL line the NGC base bakes / no drift" comment: the base bakes an OLDER 2.29.x line, so the 00-nccl-gin.conf ld.so.conf entry is LOAD-BEARING (it makes this GIN-capable source build win the loader search). - aws-ofi-nccl #1351: note it is CLOSED-UNMERGED, so unlike the draft PRs in patches/ it does NOT self-neutralize — it is a PERMANENT baseline carry on every build. Clarify the "baseline has zero dependence on unmerged PRs" line refers to the four draft PRs in the opt-in layer, not this plugin pin. - DeepEP fork trade-off: document honestly that amazon-contributing/DeepEP fixes trap-2 structurally (sysfs get_rdma_gbs, restructured QP allocator) while baseline stays stock 01dc3aa on purpose (the awslabs#612 --check-clean base + the no-fork convention); the knob clamps are the portable equivalent, and fork adoption is a deliberate future re-pin + re-measure. README pins table updated to match (base digest, NCCL commit). Signed-off-by: Anton Alexander <dmvevents@gmail.com>
…L_SHA=46be4e8 (PR awslabs#1242 review) KeitaW M2_d: the header claimed the deps are carried "at the same specifiers the pinned SHA declares," but that was not literally true. Diffed live against NeMo-RL 46be4e8's pyproject [project.dependencies] and corrected the drift: - wandb: restore upstream's >=0.25.0 floor (was unpinned). - decord -> decord2: upstream declares decord2 (the maintained fork that keeps the `decord` import name); match the spelling exactly, and drop the false "it's an extra, not in deps" note. - Header reworded: deps are at the SHA's specifiers EXCEPT for documented inline deviations, MINUS base-provided/conflicting/unused-feature-path entries — and the omitted ones are now named with why (nccl4py/cuda-bindings = non-colocated refit, pybase64 = SGLang refit, soundfile = audio multimodal — none on this colocated Megatron-GRPO text recipe). - Explain why `pip check` is deliberately NOT a build gate: --no-deps against the NGC-baked torch 2.11 (vs NeMo-RL's declared torch==2.9.0) would make pip check false-fail on that intended ABI substitution. The maintainer's diff table was computed vs cc75cad (#2411's base); this branch pins 46be4e8 (build-fix 6c14c2e), where torch/ray/transformers/pillow/mlflow already match line-for-line — so only the wandb floor and decord2 spelling were real deviations. Signed-off-by: Anton Alexander <dmvevents@gmail.com>
…examples-training
…g (reorg awslabs#1119) Reorg awslabs#1119 de-numbered the top-level dirs and moved test cases from 3.test_cases/pytorch/<framework>/ to examples/{training,inference,use-cases}/. This PR was opened against the pre-reorg path, so its files landed under the now-deleted 3.test_cases/ tree. Adding files to a deleted directory does not conflict (git silently recreates it), so the PR read mergeable while being orphaned at a dead path — it needs a migration, not a rebase. Per AGENTS.md ("a training framework example goes to examples/training/<framework>/" + "extend before create"), the sample nests as a variant subdirectory under the existing examples/training/nemo-rl/ example rather than as a new sibling — mirroring examples/training/megatron-bridge/'s populated-framework-dir-with-named-variants shape. - git mv the deepep-v2-efa subtree to examples/training/nemo-rl/deepep-v2-efa (history preserved; 14 of 15 files R100, README.md carries a one-line link fix). - git rm the stale 3.test_cases/pytorch/nemo-rl/README.md parent index (it only pointed at the pre-reorg ../slime sibling). - Add a "## Variants" pointer row to examples/training/nemo-rl/README.md so the nested example is discoverable from the framework dir (the merge's rename detection does not synthesize this — nemo-rl/README.md is a single-example README, not a per-framework case index). Case-README directory depth is unchanged (3.test_cases/pytorch/nemo-rl/ deepep-v2-efa and examples/training/nemo-rl/deepep-v2-efa are both 4 levels), so root-relative links resolve unchanged: ../../../../micro-benchmarks/expert-parallelism -> repo root (unchanged) Sibling links across the reorg's inference/training split needed one fix: ../../slime -> examples/training/slime (unchanged) ../../sglang/dsr1-deepep-efa -> BROKEN (sglang moved to inference/) ../../../inference/sglang/dsr1-deepep-efa -> examples/inference/sglang/... (fixed) Merged upstream/main rather than rebased to preserve the commit SHAs cited in the resolved review-thread replies. No functional change to the sample. Signed-off-by: Anton Alexander <dmvevents@gmail.com>
|
@KeitaW — a heads-up on a structural change to this PR since your review, so the file paths line up when you come back to it. Reorg #1119 landed on Done as a
Your 13 unresolved threads re-anchored to the new paths automatically. I've re-verified each fix is present and correct at its new location; I've left them unresolved for you to close rather than resolving my own changes. Nothing about the substance of the fixes changed — only where the files live. |
…-contributing fork; drop superseded awslabs#612 Repoint the DeepEP source from stock deepseek-ai/DeepEP @ 01dc3aa plus a draft deepseek-ai/DeepEP#612 opt-in patch to the amazon-contributing/DeepEP fork, pinned at the immutable HEAD 97d8f9bcc1be31e9036db2ab591ef9b9f4e38619. This mirrors the sibling examples/inference/vllm/deepep-v2-gdaki-efa repoint and answers the review note that pinning the pre-fix upstream fork-point forfeits the AWS EFA fixes: the fork is the AWS EPv2/NCCL-GIN tree and carries the EFA delta IN-CODE, so no awslabs#612 patch is applied on any flavor. The fork carries both correctness halves of what was draft awslabs#612 structurally: - the get_rdma_gbs() sysfs link-rate fast path (deep_ep/utils/envs.py, provider-agnostic — reads /sys/class/infiniband/<nic>/ports/*/rate), and - the auto-QP overflow clamp (deep_ep/buffers/elastic.py, clamps the allocated QP count to _C.{min,max}_unordered_gin_qps). It does NOT carry awslabs#612's third commit (a kScaleoutUpdateInterval 6->16 latency micro-opt); the fork keeps =6. No gate in this example depends on that value, so the two correctness fixes are what "supersedes awslabs#612" means here. Because the fork carries those fixes on every flavor, this collapses the baseline/opt-in distinction for DeepEP only: - the awslabs#612 entry drops out of the opt-in draft-PR layer entirely (the layer now bakes only Megatron-LM#4632 + NeMo-RL#2410); - the two dead env vars EP_EFA_MAX_QPS / EP_EFA_RDMA_GBS (the old awslabs#612 patch knobs, zero readers on the fork) are removed from env_vars.example and kubernetes/raycluster.yaml, replaced with an absence-note; and - the verify-image / setup fail-loud gate is rewritten to fork discriminators — it now asserts _get_sysfs_rdma_gbs and unordered_gin_qps are present in the installed deep_ep tree on EVERY flavor, not gated on the draft-PR marker. EP_NUM_QPS=2 is a different variable (the probe's explicit num_allocated_qps) and stays: it survives the fork clamp unchanged (max(2, min(2, max)) == 2). The Dockerfile pins the full 40-char SHA as DEEPEP_SHA; setup and verify-image fail-loud if the clone lacks either fix-half. This is the same fork the repo's micro-benchmarks/expert-parallelism/deepep-v2-benchmark/setup_deepep_gin.sh already clones. ## Test Results docs-and-packaging change (source-repoint + provenance). No functional change to the example's runtime behavior on either flavor; the fork already carried the fixes the removed patch supplied. markdownlint-cli2 clean on the changed README against the root .markdownlint.jsonc. Signed-off-by: Anton Alexander <dmvevents@gmail.com>
… literally upstream-only KeitaW review thread (nemo-rl.Dockerfile aws-ofi-nccl PR#1351 cherry-pick): the README headline called the default image 'baseline (upstream-only)', which reads as 'no unmerged-PR carry'. That is inaccurate. Decision: keep PR#1351 as a PERMANENT baseline carry (it is the load-bearing GIN CPU-proxy plugin lineage the whole substrate standardises on; gating it opt-in would be technically wrong) and fix the CLAIM instead. The default image carries, and always did (both disclosed in the Pins table): the standardized AWS GIN plugin lineage incl. closed-unmerged aws-ofi-nccl PR#1351, and the amazon-contributing/DeepEP fork. What 'baseline' actually means here is 'the 2 draft rollout PRs OFF' (Megatron-LM#4632 + NeMo-RL#2410) — those are the only opt-in toggle, and they DO self-neutralize once merged. #1351 does not; it is permanent. - README intro: 'baseline (upstream-only)' -> 'baseline' + explicit note that it carries the AWS GIN lineage (incl. #1351) and the DeepEP fork. - Build snippet comment: same clarification so the term can't recur. docs-only (claim-accuracy fix; no build/behavior change). Author-verify: gh api .../pulls/1242/commits --jq .[].author.login == dmvevents Signed-off-by: Anton Alexander <dmvevents@gmail.com>
…vs run-time transport env_vars.example said 'This image links NO NVSHMEM', which contradicts requirements.txt (deep_ep links NVSHMEM unconditionally at build time via upstream setup.py) and setup_nemo_rl_deepep_efa.sh (deep_ep's _C.so IS build-linked against NVSHMEM). Reword to match: NVSHMEM is a build-time link dependency; the V2 NCCL-GIN backend does not USE it at run time, so the values below stay inert. Closes KeitaW review thread on requirements.txt (cid 3856239817) residual. Signed-off-by: Anton Alexander <dmvevents@gmail.com>
2 participants
What this adds
A new self-contained test case,
examples/training/nemo-rl/deepep-v2-efa/, running NeMo-RLGRPO post-training whose Megatron-Core policy uses the DeepEP-V2 MoE all-to-all (ElasticBuffer,
the NCCL-GIN CPU-proxy path — not NVSHMEM) over AWS EFA. It fills the NeMo-RL gap in the
expert-parallelism example matrix alongside the merged
slimeRL sibling and the mergedsglang/dsr1-deepep-efacase, and mirrors theslimefolder shape (RL post-training onHyperPod-EKS as a Ray cluster):
nemo-rl.Dockerfile— NGC-from-scratch (nvcr.io/nvidia/pytorch:26.02-py3, the slime twin'sbase), public sources only, every pin an immutable SHA with a WHY comment, fail-loud draining
asserts: EFA 1.48.0, gdrcopy
c91ad9f, NCCLv2.30.4-1from source (GINnccl_device.hassert,ld.so.conf priority), aws-ofi-nccl
9c44d34+ PR#1351c2e773d(GIN asserts), DeepEP97d8f9bcfrom theamazon-contributing/DeepEPfork, Megatron-LM
@19deef67, NeMo-RL@46be4e8(--no-deps, torch-pin trap). An opt-in,default-OFF layer (
APPLY_DRAFT_ROLLOUT_PATCHES=1) applies 2 draft PRs before the deep_epbuild so the patched trees compile in; the baseline image has zero dependence on the unmerged
PRs.
LABEL org.opencontainers.image.source=....patches/apply_nemo_rl_patches.py— the 2 draft PRs as 5 pinned commit SHAs(Megatron-LM#4632 ×4, NeMo-RL#2410 ×1); fail-loud
git apply --check, per-change probeself-neutralization (so a multi-commit PR is not false-negatived by a per-commit
--reverse, anda partially-merged tree is not falsely skipped), post-asserts. Python string-replace only (never
sed on source). DeepEP is not in this layer: the baseline pins the
amazon-contributing/DeepEPfork, which already carries the former-draft deepseek-ai/DeepEP#612
EFA fixes in-code — the
get_rdma_gbs()sysfs link-rate fast path (deep_ep/utils/envs.py)and the auto-QP overflow clamp (
deep_ep/buffers/elastic.py) — so there is no DeepEP patch to optinto on either flavor.
recipe/—verify-image.sh(pre-cluster gate: efa-direct probe,from deep_ep import ElasticBuffer, NCCL-GIN symbol,nemo_rl.algorithms.grpo/.models.megatron.*imports, analways-run fork-discriminator assert that the installed deep_ep carries both Add autobuild of efa node exporter #612 halves —
unordered_gin_qpsand_get_sysfs_rdma_gbs— and patch-marker consistency for the 2 patched PRs);run-rollout-probe.sh+probe_rollout.py(real cross-node ElasticBuffer dispatch/combine on arollout-shaped tensor, oracle-checked, MIN all-reduce verdict, EFA-TX hw-counter delta ≥1MiB
assert; the probe passes SM/QP explicitly —
EP_NUM_QPS=2, the value the p5en evidence validated— so it is deterministic and auto-sizer-independent, and 2 QPs survives the fork's clamp unchanged);
train-step.sh+train_moe_step.py(megatron.core N-step MoE gate: loss finite + decreasing +cross-rank-agreeing; baseline dispatcher =
alltoall; theflexDeepEP-V2 dispatcher is REFUSEDwithout the patch marker).
kubernetes/—raycluster.yaml(slime shape: CPU head + N GPU workers) with the sibling EFAhardening (privileged/gdrdrv rationale, IPC_LOCK, hugepages-2Mi,
/dev/infiniband, GuaranteedQoS with the WHY,
EFA_PER_NODEseam p5=32/p5en=16);data-prep-pod.yaml.image:is abring-your-own-registry placeholder (like every sibling) — never a private ECR/ghcr ref.
README.md— slime voice + the TRT-LLM feat(deepep-efa): TensorRT-LLM NcclEP MoE all-to-all over EFA (NCCL-GIN CPU-proxy) #1240 honesty standard: pins table (every pinjustified), the mechanism chain (NeMo-RL → Megatron Shape-Y → DeepEP-V2 ElasticBuffer → NCCL-GIN
→ aws-ofi-nccl → EFA), numbered integration traps, Measured vs staged up top, Known
limitations. Plus the engine-index
examples/training/nemo-rl/README.md("Available test cases"table — the parity gap coderabbit caught for TRT-LLM, added up front here).
Why not duplicating an existing PR
No folder under
examples/training/orexamples/inference/covers NeMo-RL GRPO + DeepEP-V2 overEFA. The closest siblings are the training twin
examples/training/slime(RL post-training, sameRay-cluster shape, merged) and the inference cases
examples/inference/sglang/dsr1-deepep-efa(merged) and vLLM DeepEP-V2 (sibling PR #1230) — different engines, different execution models. Per
AGENTS.md "extend before create", a NeMo-RL slot in the EP matrix is a new framework, not a variant
of an existing folder, so it lands parallel to
slimerather than inside it. Megatron itself isdeliberately not re-added — adai already ships
examples/training/megatron-bridge/deepep; thisfolder consumes Megatron-Core as NeMo-RL's policy backend, it does not re-example Megatron.
Why the pins / the opt-in layer look the way they do
DeepEP-V2/NCCL-GIN/EFA stack from public source on the slime NGC base so a tester with only public
NGC + public GitHub can reproduce it. (Our internal proof ran on a private cascade image; that
image is not a reproducible base and is deliberately not referenced.)
amazon-contributing/DeepEPfork, not stock upstream. The fork is theAWS EPv2/NCCL-Gin tree and carries the two former-draft DeepEP#612 EFA correctness fixes
structurally (see above), so no Add autobuild of efa node exporter #612 patch is applied on any flavor. This is the same fork the
repo's own
micro-benchmarks/expert-parallelismDeepEP-V2 sample already pins insetup_deepep_gin.sh— this test case standardises on it rather than re-deriving stock + a draftpatch. (
kScaleoutUpdateInterval 6→16, the third Add autobuild of efa node exporter #612 commit, is a latency micro-opt with no gatedependence and is not carried by the fork; the fork keeps
=6.)depends on Megatron-LM#4632 (V2 ElasticBuffer in the flex dispatcher) and NeMo-RL#2410
(
LD_LIBRARY_PATHre-export + the worked 2-node EFA GRPO recipe config). As of filing: #4632 isopen; #2410 is draft, closed-unmerged. The baseline image and every non-
flexrecipe gate workwithout them; the
flexDeepEP-V2 dispatcher path is gated behind the marker and fails loud if themarker is absent, so nothing silently depends on unmerged code. NeMo-RL#2411 (a deep_ep pin
bump) is intentionally NOT applied: its base
cc75cadis 116 commits ahead of the pinned46be4e8across arequires-python3.12→3.13.13 bump, so it neither applies to this tree norbelongs on this py3.12 base, and it is metadata-only here (deep_ep is built from
/opt/DeepEP, notNeMo-RL's pin).
setup_nemo_rl_deepep_efa.sh, notsetup_deepep_efa.sh: this path builds no NVSHMEM DeepEP,so the build script is intentionally named outside the
deepep-vendor-sync.ymlpaths:gate(which requires the 3 canonical
setup_deepep_efa.shcopies be byte-identical) — same choice theTRT-LLM folder made.
Test Results
Measured (2026-05-06, 2× p5.48xlarge / H100): with the same component lineage (DeepEP-V2
NCCL-GIN + aws-ofi-nccl GIN + EFA + gdrcopy), NeMo-RL 0.5.0rc0 ran full-stack GRPO E2E — rollout
[64, 8192]in 9.45 s, Megatron Shape-Y 3-step train loss 26.41 → 24.62, ElasticBuffer active,cross-node EFA counters advanced. This ran on the (now-not-referenced) private cascade image; this
folder re-cuts the stack NGC-from-scratch.
Staged, NOT re-measured (stated plainly): this NGC-from-scratch image assembly is build-staged,
not yet cluster-re-run — the recipe's gates exist to re-verify it; the full GRPO rollout path is
draft-PR-dependent (opt-in, default OFF); no performance numbers are published from this
folder. What the recipe re-verifies on the baseline (upstream-only) image: static substrate
asserts including the fork-discriminator (
verify-image.sh), cross-node ElasticBufferdispatch/combine with the EFA TX-counter assert (
run-rollout-probe.sh), and a loss-decreasingMegatron MoE train step on the stock
alltoalldispatcher (train-step.sh). NVIDIA/nccl#2160(32-NIC p5 topo XML
MAX_NODES, still open) is documented in Known limitations with the rebuildworkaround, not silently baked. Never read a build-gate as an E2E pass.
This revision itself is a source-repoint + provenance change (DeepEP stock
@01dc3aa+ a draft#612 patch → the
amazon-contributing/DeepEPfork that carries #612 in-code), addressing themaintainer request to consume the AWS fixes from upstream rather than as local patches. It changes no
recipe logic;
npx markdownlint-cli2is clean on the changed files.References
examples/training/slime/(merged RL twin),examples/inference/sglang/dsr1-deepep-efa/(merged),examples/training/megatron-bridge/deepep/(Megatron deliberately not duplicated), plus the open inference siblings vLLM DeepEP-V2 (feat(deepep-efa): vLLM DeepEP-V2 MoE all-to-all over EFA (eager + non-eager) #1230) and
TensorRT-LLM NcclEP (feat(deepep-efa): TensorRT-LLM NcclEP MoE all-to-all over EFA (NCCL-GIN CPU-proxy) #1240).
amazon-contributing/DeepEPfork@97d8f9bc— carries the former-draft deepseek-ai/DeepEP#612EFA fixes in-code; the same fork the repo's
micro-benchmarks/expert-parallelismDeepEP-V2 samplepins.
OFI_NCCL_GDRCOPY_FORCED_PCIE_COPYoverride (forced-PCIe capability probe), pinned at immutable headc2e773d. Closed-unmerged and a PERMANENT baseline carry (not a self-neutralizing draft) — it is the GIN CPU-proxy plugin lineage this folder and the TRT-LLM NcclEP sibling standardize on; applies clean with both fail-loud asserts passing. Retires by rebasing onto an aws-ofi-nccl release that carries the override.TOPO_XML_MAX_NODES(documented workaround).