Repository navigation
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⛔ Files ignored due to path filters: 61
⚙️ Run configuration
⛔ Files ignored due to path filters: 61
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| 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. (3 skipped: 3 … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely identifies the main change: adding AWS B300 EFA support. Description check The description directly explains the AWS B300 support changes, affected workloads, validation results, and scope. Linked Issues check Directly linked issue #557requires AWS p6-b300.48xlarge NCCL entries to request 16 EFA devices, not 32, and to use a B300-compatible image. It also requires AWS+B300 WorkloadRun overrides with 16 EFA…Out of Scope Changes check Changes map to #557or support it. Catalog overrides, the B300 EFA runtime patch, and WorkloadRun overrides implement the B300 behavior. The aws-b300 mock node and TestAWSB300NCCL support rendering an…Full details: Linked Issues check
-
Explanation
Directly linked issue
#557requires AWS p6-b300.48xlarge NCCL entries to request 16 EFA devices, not 32, and to use a B300-compatible image. It also requires AWS+B300 WorkloadRun overrides with 16 EFA devices, retained caller images, and the eight-GPU default. Whole-PR summary evidence: nccl-all-reduce, nccl-all-gather, nccl-alltoall, nccl-loopback, and nccl-loopback-nvswitch each add an AWS+b300 override that uses the B300 EFA runtime patch (vpc.amazonaws.com/efa limits and requests of 16) and a CUDA 13.0.2 EFA NCCL-tests image. The H100 EFA overrides now exclude b300. Head file pkg/platform/overrides/workloadrun.yaml confirms a B300 TrainingRuntime dependency requesting 16 EFA, with IPC_LOCK, and SYS_CHROOT for MPI. It also confirms B300 EFA transport env through mpiArgs (MPI) and trainer.env (non-MPI). These overrides add resources and env only, so caller images are retained. Automated coverage: test/uat/aws_b300_nccl_test.go (TestAWSB300NCCL). Limits: the sm_103 native-code claim comes from the PR description and was not checked against a Dockerfile or image manifest. Integration and render fixtures in testdata were not inspected; the PR reports them passing. The incremental diff from 56dfd7a to 7839e46 could not be read because changed blobs are missing from the checkout, so the head state was assessed directly. Nemotron training is excluded by the issue and is not required. Hardware reruns and maintainer acknowledgment are non-coding and not assessed.
Full details: Out of Scope Changes check
-
Explanation
Changes map to
#557or support it. Catalog overrides, the B300 EFA runtime patch, and WorkloadRun overrides implement the B300 behavior. The aws-b300 mock node and TestAWSB300NCCL support rendering and acceptance coverage. CHANGELOG, platform-detection, certify-a-cluster, run-workloadrun, and FAQ docs describe the B300 behavior. The earlier question about narrowing the AWS GB300 plugin-removal and MPI forwarding entries is resolved. Comparing base and head pkg/platform/overrides/workloadrun.yaml shows those blocks were already scoped to gb300 at the merge base. This PR does not change H100 or GB200 WorkloadRun behavior in that file. The only additions in that file are the AWS+B300 blocks. No unrelated change is demonstrated. No controller, API, CRD, dependency, or Helm change is reported. The incremental diff since the previous reviewed head could not be read, so that range is not independently checked.
-
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. (3 skipped: 3 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.
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 <jharp@nvidia.com>
Signed-off-by: Joe Harp <jharp@nvidia.com>
Signed-off-by: Joe Harp <jharp@nvidia.com>
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.
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.