Repository navigation
feat: add AWS B300 EFA support - #558
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (6 skipped: 6 … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title "feat: add AWS B300 EFA support" names the primary change. The PR adds AWS p6-b300.48xlarge NCCL overrides and WorkloadRun EFA settings, which the title covers. The title is concise and spec… Description check The description states the purpose: AWS p6-b300.48xlarge support for NCCL communication and MPI/torch/exec WorkloadRuns, with 16 EFA devices and a CUDA 13 NCCL image. It also covers validation, scope,… Linked Issues check Issue #557requires AWS B300 NCCL entries and MPI/torch/exec WorkloadRuns to use 16 EFA devices, a B300-compatible NCCL image, caller-selected WorkloadRun images, and the eight-GPU default. The change…Out of Scope Changes check The changes support issue #557. Catalog entries implement B300 NCCL behavior. Platform overrides implement B300 WorkloadRun behavior. Mock nodes, tests, and fixtures support rendering and acceptance v…Full details: Title check
-
Explanation
The title "feat: add AWS B300 EFA support" names the primary change. The PR adds AWS p6-b300.48xlarge NCCL overrides and WorkloadRun EFA settings, which the title covers. The title is concise and specific.
Full details: Description check
-
Explanation
The description states the purpose: AWS p6-b300.48xlarge support for NCCL communication and MPI/torch/exec WorkloadRuns, with 16 EFA devices and a CUDA 13 NCCL image. It also covers validation, scope, and documentation, all of which match the changeset.
Full details: Linked Issues check
-
Explanation
Issue
#557requires AWS B300 NCCL entries and MPI/torch/exec WorkloadRuns to use 16 EFA devices, a B300-compatible NCCL image, caller-selected WorkloadRun images, and the eight-GPU default. The change adds AWS+B300 overrides for all five NCCL categories and a B300 runtime patch with 16 EFA devices. The WorkloadRun override adds EFA transport settings without replacing caller images and preserves the default GPU count. The summary also reports resolved-runtime, environment, image-secret, render, integration, and UAT validation. Nemotron is explicitly outside issue#557scope.
Full details: Out of Scope Changes check
-
Explanation
The changes support issue
#557. Catalog entries implement B300 NCCL behavior. Platform overrides implement B300 WorkloadRun behavior. Mock nodes, tests, and fixtures support rendering and acceptance validation. Documentation describes the new B300 requirements and limitations. No unrelated behavior change is demonstrated. Nemotron remains excluded. The listed testdata files were intentionally excluded, but the summary provides independent evidence for the required implementation and validation.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (6 skipped: 6 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
kaynetu
left a comment
kaynetu
left a comment
There was a problem hiding this comment.
The B300 overrides look right to me: each one matches its H100 block except for the EFA count, the dependency and the image, and the WorkloadRun entries follow the GB200/H100 pattern. Two doc points, one inline.
Some places outside the diff still list only "H100 and GB200" for the nccl-tests image. B300 now lands there too:
docs/operations/faq.md:54, the<Warning>about settingimageon AWS: B300 should be listed. On B300 a replacement image also needs a CUDA 13 build withsm_103code, not just the aws-ofi-nccl plugin, so that requirement should go in the warning too.docs/operations/faq.md:59, "AWS H100 and GB200: the catalog already lands the workers on the nccl-tests image": B300 belongs in this bullet as well.pkg/catalog/entries/_lib/nccl/aws-efa-transport-env.yaml:5: the header says the fragment is shared "across H100 and GB200". The B300 WorkloadRun overrides now use it too.
ndipebot
left a comment
ndipebot
left a comment
There was a problem hiding this comment.
I diffed each of the five new AWS + B300 blocks against its AWS + H100 sibling with the image, the dep fragment path and the arch selector normalised out. All five come back empty, so the arg order, the -d uint8 on nccl-alltoall, the timeout 3600 mpirun wrapper and the loopback NCCL_SHM_DISABLE/NCCL_P2P_DISABLE all carry over intact. deps/aws-b300-efa-runtime-patch-comm.yaml is a field-for-field parallel of deps/aws-efa-runtime-patch-comm.yaml with 32 to 16 and the CUDA 13 image.
git grep "notIn: \[gb200" returns exactly the five entries you edit, all now carrying b300, and no other when: in those entries matches aws plus b300, so the new block is the only one that fires. nemotron5-8b.yaml's AWS block is equals: h100, so training really does fall through to the base entry.
The golden churn is all accounted for: the +92/+69 hunks are the two new override entries growing the embedded list, build-workflow-spec/* is overrideCount 23 to 25 plus appliedOverrides.index shifts, sshd-worker-invariants/catalog is containersScanned 358 to 368, and test/uat/testdata/aws/b300/nccl/expected_pods.yaml is line-for-line congruent with the H100 golden at the same offsets. The new integration cases pin the right things: USER_SETTING survives on the runtime, FI_* lands on the trainer, SYS_CHROOT only on the MPI path.
Approving. kaynetu's three doc points are the only things I would still change at 56dfd7a4, and I agree with all three. The faq.md one in particular: aws-b300-efa-runtime-patch-comm.yaml puts a third AWS configuration on the nccl-tests image, so "Two AWS configurations need no override at all" at docs/operations/faq.md:57 is now wrong, and the <Warning> at line 54 should name B300 along with its CUDA 13 / sm_103 requirement.
One merge-order note inline.
|
Thanks for the review, Kayne. Updated the FAQ and shared EFA comment to include B300 and clarified the replacement-image requirements. Also removed the TCP fallback claim—we haven’t validated Nemotron training on B300, so the docs now state that it’s unsupported |
|
@ViriLain Can you please look into the CI failure? Thanks |
|
@kaynetu CI is fixed |
kaynetu
left a comment
kaynetu
left a comment
There was a problem hiding this comment.
LGTM. The docs now describe Nemotron on AWS B300 as unsupported rather than falling back to TCP. The FAQ warning, the "no override needed" list, and the aws-efa-transport-env.yaml header all include B300 and its CUDA 13 / sm_103 image requirement. The aws-l20-nccl render-annotation golden from #532 picks up the widened notIn [gb200,gb300,b300] selector.
Heads-up on merge order: #552 also edits the AWS H100 notIn selector, so whichever of the two lands second needs the union [gb200, gb300, b300, rtxpro6000] in all five NCCL entries and in this golden.
0b3a75a to
247945c
Compare
ndipebot
left a comment
ndipebot
left a comment
There was a problem hiding this comment.
All four doc points landed, and I checked each against the catalog rather than the prose.
faq.md:54 now names B300 alongside H100 and GB200, and the new paragraph at :56 adds the CUDA 13 and sm_103 requirement. nccl-all-reduce.yaml:287 does pin public.ecr.aws/hpc-cloud/nccl-tests:cuda13.0.2-efa1.48.0-ofiv1.19.0-ncclv2.30.4-1-testsv2.18.3 for the AWS + b300 block, so the image and CUDA 13 claims are backed by code. Dropping the count word at :59 instead of bumping it to three is better than what I suggested, since GB300 is its own bullet. The aws-efa-transport-env.yaml header is true and not aspirational: git grep aws-efa-transport-env returns six call sites in pkg/platform/overrides/workloadrun.yaml, and :168 and :182 sit under the aws + b300 blocks.
On platform-detection.md:129-132, I enumerated every when: block in nemotron5-8b.yaml and nemotron5-56b.yaml: the only b300 selectors are onprem + [b200, b300] and nscale + [b200, gb300], so there is no AWS + B300 training override and the base entry is what renders. The "not supported, not validated" wording holds.
247945cd is one line in aws-l20-nccl/expected.json, notIn [gb200,gb300] to notIn [gb200,gb300,b300]. L20 still matches the AWS H100 block, and the workflow name, both detected-* annotations and the patches summary are untouched. I reverted the five notIn lines in a worktree with the golden kept and TestCertificationRenderAnnotations failed on exactly that string, so the snapshot is load-bearing. go test ./pkg/certification/ ./pkg/catalog/... ./pkg/platform/ ./pkg/render/ ./pkg/workloadrun/ ./test/docspolicy/ is green at the head.
One thing the faq.md fix missed: docs/api-reference/certification.md:138 still reads "on AWS EFA platforms (H100, GB200) the platform overrides land the workers on an nccl-tests image". That is the same sentence kaynetu flagged, and after this PR B300 is a third AWS EFA platform with the same footgun on image. The file is hand-maintained, not generated. Could you widen the parenthetical to (H100, GB200, B300) and mirror the CUDA 13 / sm_103 note from faq.md:56? The same phrasing in ADR-077 and ADR-093 is historical and should stay.
The merge-order note with #552 is now wider than when I raised it. git merge-tree --write-tree pr/558 pr/552 at 5ccec790 conflicts on twelve paths. The five NCCL entries are still the core of it: [gb200, gb300, b300] here against [gb200, gb300, rtxpro6000] there, both from [gb200, gb300] on main, so the union is the only safe resolution. New since last time, #552 also rewrites aws-l20-nccl/expected.json, so that golden conflicts directly and the merged value has to be notIn [gb200,gb300,b300,rtxpro6000]. The three sshd-worker-invariants goldens, both render-errors goldens and two build-workflow-spec goldens conflict too. Whichever lands second should regenerate those on the merge result rather than keeping either branch's copy.
I could not check the sm_103 claim from here, since it is about the contents of an external image this repo does not build. It reads plausibly for a CUDA 13 nccl-tests build. If you are touching faq.md:56 anyway, naming the tag would save a reader a trip to nccl-all-reduce.yaml.
247945c to
789afa6
Compare
|
Thank you for checking this; It looks like I pulled main in as commits and not a rebase. I believe I have fixed this and the PR should be back to expected. |
ndipebot
left a comment
ndipebot
left a comment
There was a problem hiding this comment.
My doc item is fixed. b17b163d widens docs/api-reference/certification.md:139 to "(H100, GB200, B300)" and adds the CUDA 13 / sm_103 sentence mirroring faq.md. I checked the claim against the catalog rather than the prose: pkg/catalog/entries/communication/nccl-all-reduce.yaml pins public.ecr.aws/hpc-cloud/nccl-tests:cuda13.0.2-efa1.48.0-ofiv1.19.0-ncclv2.30.4-1-testsv2.18.3 on the aws + b300 block, and _lib/deps/aws-b300-efa-runtime-patch-comm.yaml puts the same image on the node container, so B300 really is a third AWS EFA configuration landing on nccl-tests. The sm_103 assertion is about the contents of a public ECR image and is not checkable from here; it rests on your hardware run.
With the image, the dep path and the arch selector masked out, each of the five new aws + b300 catalog blocks is byte-identical to its aws + h100 sibling, so -d uint8, the timeout 3600 mpirun wrapper, -x LD_LIBRARY_PATH and the loopback NCCL_SHM_DISABLE/NCCL_P2P_DISABLE pair all carry over. The dep fragment differs from aws-efa-runtime-patch-comm.yaml in exactly the image and 32 to 16. git grep notIn: over the catalog and the platform overrides returns only the five widened lines plus two unrelated notIn: [a100], so nothing else quietly picks up b300, and enableMNNVL, hugepages and ComputeDomain are untouched.
I reverted pieces to see what the tests actually hold. Dropping the B300 section from pkg/platform/overrides/workloadrun.yaml fails TestBuildWorkflowSpec, TestApplyPlatformMPIArgs, TestBuildOverridesMPIArgs and TestSSHDWorkerInvariants. Reverting the five notIn lines to [gb200, gb300] fails TestCertificationRenderAnnotations on the aws-l20-nccl string, so that one-line golden churn is real. Deleting the five catalog B300 blocks fails only TestSSHDWorkerInvariants on the containersScanned count of 368, which is a thin package-level pin; the behavioural coverage is the integration case certification-aws-b300-nccl, whose golden carries both the CUDA 13 image and vpc.amazonaws.com/efa: "16", plus the UAT golden. Envtest will not bind a port in my sandbox, so I leaned on CI for the integration run.
The golden churn accounts for itself. The +92/+69 hunks are the two new WorkloadRun entries growing the embedded list plus appliedOverrides.index shifts (19 to 21 on workloadrun-onprem-b300); containersScanned moves 358 to 368 with sshdWorkers correctly still 6, since the B300 fragment patches image and resources only and never a command; list-available and both render-errors goldens gain aws-b300 from the new mock node. Nothing I looked for should have moved and did not: thresholds come from the caller through .Thresholds rather than a per-architecture table, nic_detect.go scopes the b300 NIC block to on-prem, and no new dependency kind means run-role.yaml stays as is. go test ./pkg/catalog/... ./pkg/platform/... ./pkg/certification/ ./pkg/render/... ./pkg/workloadrun/ ./pkg/gpu/ ./test/docspolicy/ is green at the head.
One comment went stale. pkg/platform/overrides/workloadrun.yaml:51 still explains the gb300-scoped plugin cleanup with "H100 and GB200 on AWS are EFA fabrics, and EFA is reachable only through libfabric via this plugin", and the new block at :267 requests vpc.amazonaws.com/efa: 16, so B300 now has exactly that property. You did update the same enumeration in pkg/catalog/entries/_lib/nccl/aws-efa-transport-env.yaml:5, so this reads like the one site that was missed.
Merge order with #552 is still open and it has grown. git merge-tree --write-tree pr/558 pr/552 now conflicts on 24 files rather than the 6 I saw last time: the five NCCL entries, pkg/certification/testdata/certification-render-annotations/aws-l20-nccl/expected.json, all three sshd-worker-invariants goldens, both render-errors goldens, and twelve build-workflow-spec goldens. Both sides derive from notIn: [gb200, gb300], so whichever lands second wants the union notIn: [gb200, gb300, b300, rtxpro6000] in all five entries and then goldens regenerated on the merge result, not either branch's copies. #549 needs its own resolution where aws-b300 joins a sorted list in the pkg/render goldens. Nothing blocking from me.
ac1980f to
74db9d5
Compare
ndipebot
left a comment
ndipebot
left a comment
There was a problem hiding this comment.
Re-approving after the rebase. I diffed the branch content against what I approved at b17b163d, with both sides taken against their own merge base: the file lists are identical and the only hunk that moved is in CHANGELOG.md, where the bullet now slots under the ### Added header that ADR-108's entry created instead of adding the header itself. Everything else is byte for byte the same, and the full check set is green at 74db9d59.
The table row nit stays open and stays optional. Merge order with #552 is unchanged: whichever lands second wants notIn: [gb200, gb300, b300, rtxpro6000] in the five NCCL entries and goldens regenerated on the merge result.
|
@ViriLain pls resolve conflicts |
AWS p6-b300.48xlarge resolves to b300 but currently matches the H100 NCCL fallback, which requests 32 EFA devices and cannot schedule on a node advertising 16. The fallback image also predates B300 support. Add explicit AWS B300 overrides for all five NCCL communication entries, requesting 16 EFA devices and selecting the CUDA 13 image with native sm_103 code. Apply matching EFA resource and transport settings to MPI, torch and exec WorkloadRuns while preserving caller environment variables and the eight-GPU default. Add offline B300 node rendering, resolved-workload integration fixtures, pull-secret coverage and a Kind/KWOK acceptance case. Keep Nemotron training outside this change pending a compatible image and separate hardware validation. The B300 workload manifests match the configuration validated on two AWS B300 nodes with EFA GPU-direct traffic. The latest upstream controller changes are covered by local CI and simulated cluster validation; that newer controller was not rerun on AWS hardware. Signed-off-by: Joe Harp <[email protected]>
Signed-off-by: Joe Harp <[email protected]>
Signed-off-by: Joe Harp <[email protected]>
Signed-off-by: Joe Harp <[email protected]>
9bcd86f to
eadd217
Compare
|
/ok-to-test eadd217 |
* feat: add AWS B300 EFA support AWS p6-b300.48xlarge resolves to b300 but currently matches the H100 NCCL fallback, which requests 32 EFA devices and cannot schedule on a node advertising 16. The fallback image also predates B300 support. Add explicit AWS B300 overrides for all five NCCL communication entries, requesting 16 EFA devices and selecting the CUDA 13 image with native sm_103 code. Apply matching EFA resource and transport settings to MPI, torch and exec WorkloadRuns while preserving caller environment variables and the eight-GPU default. Add offline B300 node rendering, resolved-workload integration fixtures, pull-secret coverage and a Kind/KWOK acceptance case. Keep Nemotron training outside this change pending a compatible image and separate hardware validation. The B300 workload manifests match the configuration validated on two AWS B300 nodes with EFA GPU-direct traffic. The latest upstream controller changes are covered by local CI and simulated cluster validation; that newer controller was not rerun on AWS hardware. Signed-off-by: Joe Harp <[email protected]> * docs: clarify AWS B300 image and training support Signed-off-by: Joe Harp <[email protected]> * test: update AWS render annotation snapshot for B300 Signed-off-by: Joe Harp <[email protected]> * docs: clarify B300 replacement image requirements Signed-off-by: Joe Harp <[email protected]> --------- Signed-off-by: Joe Harp <[email protected]> Signed-off-by: Ebot Ndip-Agbor <[email protected]>
Summary
Add AWS
p6-b300.48xlargesupport for NCCL communication and MPI/torch/exec WorkloadRuns. B300 currently matches the H100 fallback, requesting 32 EFA devices on nodes that expose 16 and selecting an image below AWS's B300 software requirements.sm_103code.Closes #557
Validation
KUBECONFIG=/dev/null make cipassed against upstream7951cad5, including 242 integration cases. All 14make test-uatcases passed.2184b914, the rebuilt controller repeated all five NCCL categories at 128 MiB and two-node MPI successfully. The latest7951cad5refresh passed local CI/UAT and renders the same B300 workloads; that newer controller binary was not rerun on AWS hardware.Measured NCCL bus bandwidth at 8 GiB across three passes:
These are observed measurements, not performance acceptance thresholds.
Scope
Nemotron training is outside this change. No controller, API, dependency, or Helm changes are included.
WorkloadRun images must provide a compatible EFA plugin. NGC PyTorch
26.04-py3required caller settingNCCL_NET_PLUGIN=ofi; the initial socket run was rejected and the EFA rerun passed. This requirement is documented.