feat(recipes): add kubeflow training overlay for rtx-pro-6000 on EKS#1857
Conversation
Adds rtx-pro-6000-eks-training / -ubuntu-training / -ubuntu-training-kubeflow so training on EKS resolves kubeflow-trainer + kubeflow-trainer-post, matching the GB200-EKS / rtx-pro-6000-LKE pattern. NCCL all-reduce gating intentionally omitted: RTX PRO 6000 is single-GPU, no NVLink (see rtx-pro-6000-lke-training.yaml), and isn't in the compiled supportedNCCLCombinations matrix (validators/performance/nccl_all_reduce_bw_constraint.go). Fixes NVIDIA#1372 Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
📝 WalkthroughWalkthroughAdds a base RTX PRO 6000 EKS training recipe with Kubernetes version constraints, GPU operator and node customization dependencies, NFD topology configuration, and training conformance checks. Adds Ubuntu and Kubeflow overlay recipes that inherit from the base recipes, apply matching criteria and mixins, enforce the Kubernetes version constraint, and define component references. Extends conformance and NFD topology updater test coverage for the new recipes. Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@recipes/overlays/rtx-pro-6000-eks-training.yaml`:
- Around line 91-101: Add metadata-store regression coverage for the
rtx-pro-6000-eks-training recipe chain, covering both leaf overlays. Assert
inherited component references, Kubeflow trainer components, the Kubernetes
constraint, the listed conformance checks, and the intentional absence of NCCL
performance checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ef6f1a3b-09ba-45d1-a781-d9c4703d394a
📒 Files selected for processing (3)
recipes/overlays/rtx-pro-6000-eks-training.yamlrecipes/overlays/rtx-pro-6000-eks-ubuntu-training-kubeflow.yamlrecipes/overlays/rtx-pro-6000-eks-ubuntu-training.yaml
Recipe evidence checkOther affected recipes without evidence yet: 3These recipes are affected by this PR but carry no committed evidence pointer, so there is
This gate is warning-only and never blocks merge. See ADR-007 for the trust model. |
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
Thanks for the PR! Three findings: one CI blocker (trivial trailing newline), one scope/documentation correction on the NCCL omission rationale, and one non-blocking test-coverage gap. Otherwise this is pattern-consistent with the GB200-EKS / LKE training overlays and safe to merge once addressed. Details inline; the test-coverage item is below since it touches files outside the diff.
Also, the branch is currently 80 commits behind main — please rebase onto origin/main (rather than a merge commit or the "Update branch" button) when you push the fixes, so CI runs against current main.
Test-coverage gap (non-blocking): the topology-updater regression table's RTX PRO 6000 EKS section (pkg/recipe/metadata_test.go#L2322-L2326) contains only the inference variants. The three new training leaves all resolve with topologyUpdater: true but aren't covered, so a future regression dropping it would go uncaught. The conformance table (pkg/recipe/conformance_test.go#L304) likewise doesn't cover the new training chain. The sibling inference overlays were added to both tables when introduced — please add the three training criteria (rtx-pro-6000-eks-training, -ubuntu-training, -ubuntu-training-kubeflow) for parity. Coverage debt only; no present runtime failure.
Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/recipe/conformance_test.go`:
- Around line 511-519: Update the requiredComponents list in the conformance
test to include both Kubeflow trainer components by adding kubeflow-trainer-post
alongside kubeflow-trainer, ensuring the test detects regressions in either
component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: fc07f809-4362-4f59-b546-a9494c7cf340
📒 Files selected for processing (2)
pkg/recipe/conformance_test.gopkg/recipe/metadata_test.go
Addresses CodeRabbit review thread r3637067326 on #1857. The suggested requiredComponents entry "kubeflow-trainer-post" is not a recipe component — it is a helmfile release the bundler injects from the kubeflow-trainer component's manifestFiles — so the literal suggestion fails resolution. Instead, assert that manifest-carrying components keep their manifestFiles in the resolved recipe, which is the actual regression surface for the -post release. Also regenerate testdata/coverage_golden.yaml (AICR_UPDATE_GOLDEN=1): the rtx-pro-6000 EKS training projections from #1857 were merged without a golden refresh, leaving TestCoverageGoldenMatrix failing on main. The update is purely additive. Signed-off-by: Mark Chmarny <mark@chmarny.com>
3 participants
Adds rtx-pro-6000-eks-training / -ubuntu-training / -ubuntu-training-kubeflow so training on EKS resolves kubeflow-trainer + kubeflow-trainer-post, matching the GB200-EKS / rtx-pro-6000-LKE pattern.
NCCL all-reduce gating is intentionally descoped from this PR pending calibration. Follow-up work is tracked in #.
Related to #1372