feat(recipes): add l40-any accelerator overlay - #2366
Conversation
|
Welcome to AICR, @ntheanh201! Thanks for your first pull request. Before review, please ensure:
A maintainer will review this soon. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe change adds the Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR adds L40 recipe resolution without changing existing recipes or runtime code; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
2555634 to
f004691
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
▎ Method: 3 persona passes (recipe-domain, data-integrity/goldens, docs/contract-drift), each finding independently confirmed or refuted by a senior meta-reviewer against the resolved code. Line links pinned to head f004691b.
▎ Tier legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick · ✅ Confirmed non-issue
Overall assessment — Approve
Clean, well-scoped, data-only change: a faithful mirror of l40s-any that closes a real gap (l40 is a registered accelerator but no overlay provided it, so criteria(accelerator=l40) errored). Confirmed the motivating error is fixed — the bare accelerator=l40 query now resolves — and that the change is purely additive with no existing leaf hash moved. Nothing here can break CI or ship a bad artifact. One optional doc nitpick (below); otherwise ready to merge.
The one substantive persona concern — that l40 having no concrete leaves would leave intent-qualified queries broken — did not survive adversarial verification (see non-issues).
🔵 Nitpick — docs/user/recipe-health.md missing the new l40-any row
(Not inline — recipe-health.md isn't in this PR's diff.) The matrix enumerates every overlay (a100-any, h200-any, l40s-any all have rows); the new l40-any leaf is resolvable but has no row, so the committed doc is momentarily stale relative to the catalog. Non-blocking: recipe-health.md is regenerated hermetically by recipe-health-refresh.yaml (make recipe-health-docs) weekly and is not a PR-time merge gate, so a scheduled PR will add the row (predictably | l40-any | — | l40 | — | — | — | pass | R:0 D:4 P:0 C:0 | pending |). You disclosed this in the checklist. Optional: run make recipe-health-docs to include it now for immediate parity, or let the bot handle it.
✅ Confirmed non-issues (checked and cleared)
- Intent-qualified
l40resolution (persona-flagged, refuted). With no concretel40leaves, would--accelerator l40 --intent trainingstill error? Refuted empirically against the embedded catalog:service=eks, accelerator=l40, intent=trainingresolves successfully (overlays[base monitoring-hpa l40-any eks eks-training]) — service/intent overlays are accelerator-agnostic and compose with the floor, so no per-accelerator concrete leaf is needed. The only failing case,--accelerator l40 --intent trainingwith no service, returns the identical genericintent 'training' requires additional criteriaerror thatl40s(which has concrete leaves) returns — a universal "intent needs a service" guard, not anl40gap. The PR's motivating error was the barecriteria(accelerator=l40)query, which now succeeds. - Goldens additive-only.
catalog_parityandstock_rendereach +1 hash line,coverage+2 lines, inserted in correct sorted position; no existing leaf hash moved (verified against the upstream merge-basede6514b13, not the stale local forkorigin/main). - BOM unaffected. Overlay uses
base: basewith nocomponentRefs, adds no images;registry.yamluntouched, soTestCommittedBOMVersionsMatchRegistryscope is unchanged. - No enum/contract drift.
l40(pre-existing value) is already co-listed withl40sin README, OpenAPI (all enum blocks), cli-reference, api-reference, recipe.md,doc.go,recipe.go, and the bug-report issue template. >= v24.6.0floor is defensible. L40 is the same Ada Lovelace generation as L40S; family-baseline floor matching A100/H100/H200/L40S. The comment ("4 standard checks plus the version pin") matches the 4-entry checks list; the referencedcriteria-wildcard-overlaysanchor exists.- Tests pass.
go test ./pkg/recipe/... ./pkg/bundler/green; goldens self-consistent.
Summary
| Tier | Count | Items |
|---|---|---|
| 🔴 Blocker | 0 | — |
| 🟠 Major | 0 | — |
| 🟡 Minor | 0 | — |
| 🔵 Nitpick | 1 | recipe-health.md row (bot-covered, optional) |
Recommendation: Approve. Nothing to fix before merge; the one nitpick is optional and the weekly refresh bot will add the row.
|
@ntheanh201 this PR now has merge conflicts with |
f004691 to
a9673c7
Compare
| kind: RecipeMetadata | ||
| apiVersion: aicr.run/v1alpha2 | ||
| metadata: | ||
| name: l40-any |
There was a problem hiding this comment.
MINOR: Regenerate the user-facing recipe health matrix
This adds l40-any, but the generated recipe-health table still jumps from h200-any to l40s-any. Users checking recipe support and evidence cannot discover the new L40 recipe even though the support overview now claims L40 coverage.
Minimum correction: run make recipe-health-docs and commit the updated matrix so it includes l40-any.
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
▎ Method: Re-review against head 10655780. My prior full-panel review (3 personas + adversarial meta-review) was at f004691b; history was rebased since (feature re-committed as a9673c79, then main merged in). PR's own content re-verified against the real merge-base d3801134c.
▎ Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick · ✅ Non-issue
Overall — Approve (standing verdict holds)
The "170 files changed since last review" is main-merge drift, not this PR. The PR's own diff is still the same 4 files, purely additive:
recipes/overlays/l40-any.yamlis byte-identical to what I cleared atf004691b.- Net golden diff vs the real base is exactly +2
l40-anyhash lines + 1 coverageoutcome: successentry — the rebase did not perturb the additive-only invariant; no existing leaf hash moved.
Everything the prior panel confirmed still stands: the motivating criteria(accelerator=l40) resolution error is fixed, intent-qualified l40 queries resolve, BOM/enum/contract are unaffected, and the >= v24.6.0 floor (matching the A100/H100/H200/L40S Ada baseline) is defensible.
Prior-feedback status
| Prior finding | Tier | Disposition |
|---|---|---|
docs/user/recipe-health.md missing an l40-any row (matrix jumps h200-any → l40s-any) |
🔵 Nitpick | ✖️ Not addressed — deferred to the weekly recipe-health-refresh bot (not a PR-time gate); also flagged by @mchmarny as MINOR. Non-blocking; optional to run make recipe-health-docs for immediate parity. |
New findings
None. The only surface introduced since the prior review is the main merge, which is already-reviewed upstream content.
Summary
| Tier | Count |
|---|---|
| 🔴 Blocker | 0 |
| 🟠 Major | 0 |
| 🟡 Minor | 0 |
| 🔵 Nitpick | 1 (recipe-health.md row — optional, bot-covered) |
Recommendation: Approve. No blocking change since the prior review — the PR was only rebased, its content is identical, and the additive-only invariant holds.
|
@ntheanh201 this PR now has merge conflicts with |
`l40` is a registered accelerator type, but no overlay provides it, so any query carrying `accelerator=l40` fails to resolve. This is reachable from a live cluster: `aicr snapshot` on an L40 node detects `accelerator: l40` from `nodeTopology.label.nvidia.com/gpu.product`, and the snapshot-driven `aicr recipe --snapshot` then fails with `no recipe provides accelerator 'l40'`. Add the criteria-wildcard overlay, mirroring `l40s-any`. L40 is the same Ada Lovelace generation as L40S and tracks the same gpu-operator support timeline, so it carries the same deployment-phase floor: the four standard checks plus a `gpu-operator >= v24.6.0` pin matching the A100/H100/H200/L40S baseline. Goldens regenerated deliberately with AICR_UPDATE_GOLDEN=1 (catalog parity, coverage matrix, stock render). The change is additive: one new leaf, and no existing leaf hash moved. Signed-off-by: The Anh Nguyen <ntheanh201@gmail.com>
Addresses review feedback: the generated table still jumped from `h200-any` to `l40s-any`, so users checking recipe support and evidence could not discover the new L40 recipe even though the support overview now claims L40 coverage. Regenerated with `make recipe-health-docs`; the auto-generated section picks up the new leaf (48 -> 49 recipes) and `make recipe-health-check` is clean. Signed-off-by: The Anh Nguyen <ntheanh201@gmail.com>
1065578 to
074366c
Compare
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
▎ Method: Delta re-review against head 074366cf (2 personas — recipe-domain, data-integrity — each independently confirmed against the resolved code). Third pass; both prior full reviews were Approve. Line links pinned to 074366cf.
▎ Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick · ✅ Non-issue · ✔️ Addressed · ✖️ Not addressed
Overall — Approve (standing verdict holds; the one open item is now closed)
The PR's own diff is still the same small, purely-additive change (5 files; the "changed since last review" file count is main-merge drift, not this PR). The one difference that matters since my last review: commit 074366cf regenerated docs/user/recipe-health.md, which resolves the only outstanding finding — the missing l40-any row that both I and @mchmarny flagged.
Independently re-confirmed this pass:
recipes/overlays/l40-any.yamlis a faithfull40s-anymirror — identical modulo the accelerator name and a comment that's improved (it now explicitly ties L40 to the same Ada Lovelace generation as L40S). apiVersion,criteria, the four deployment checks, and the>= v24.6.0floor all correct.- Goldens are additive-only vs the real merge-base
2ed59ed50—+1hash incatalog_parityandstock_render,+2lines incoverage, zero removed lines, no existing leaf hash moved. Committed goldens passgo test ./pkg/recipe/... ./pkg/bundler/withoutAICR_UPDATE_GOLDEN. l40is a pre-existing registered accelerator (pkg/recipe/criteria.go:127) — no enum/contract/doc expansion triggered.
Duplicate-work note
@mchmarny also reviewed this PR twice (both dismissed on push); their standing concern was the same recipe-health row, now resolved. No divergence between our findings.
Prior-feedback status
| Prior finding | Tier | Disposition |
|---|---|---|
docs/user/recipe-health.md missing an l40-any row |
🔵 Nitpick | ✔️ Addressed — commit 074366cf adds the row at recipe-health.md L56, correct alpha position, format-identical to siblings; summary counts moved 48→49. D:4 matches the leaf's four deployment checks. |
New findings
None. The only surface added since my last review is the recipe-health regeneration (verified internally consistent with the leaf) and the main merge (already-reviewed upstream content).
Summary
| Tier | Count |
|---|---|
| 🔴 Blocker | 0 |
| 🟠 Major | 0 |
| 🟡 Minor | 0 |
| 🔵 Nitpick | 0 |
Recommendation: Approve. Data-only, purely additive, additive-golden invariant holds, and the single deferred nitpick from prior rounds is now fixed. Ready to merge.
|
🌿 Preview your docs: https://nvidia-preview-feat-recipes-l40-any.docs.buildwithfern.com/aicr |
3 participants
Summary
Adds the missing
l40-anycriteria-wildcard overlay.l40is already a registered accelerator type, but no overlay provides it, so any query carryingaccelerator=l40fails to resolve.Motivation / Context
The gap is reachable from a live cluster, not just from explicit flags.
aicr snapshoton an L40 node detectsaccelerator: l40fromnodeTopology.label.nvidia.com/gpu.product, but the snapshot-drivenaicr recipe --snapshotthen fails:The accelerator type itself is valid —
--accelerator foobaris rejected at parse time withinvalid accelerator type, while--accelerator l40parses and fails only at resolution.README.mdalso lists L40 under supported accelerators, so the catalog and the docs currently disagree.Fixes: #TBD
Related: N/A
Type of Change
Component(s) Affected
pkg/recipe)pkg/bundler,pkg/component/*) — golden only, no code changerecipes/overlays/Implementation Notes
Mirrors
l40s-any. L40 is the same Ada Lovelace generation as L40S and tracks the same gpu-operator support timeline, so it carries the same deployment-phase floor: the four standard checks plusDeployment.gpu-operator.version >= v24.6.0, matching the A100/H100/H200/L40S baseline. Concrete leaves can tighten it later.Three goldens were regenerated deliberately with
AICR_UPDATE_GOLDEN=1— catalog parity, coverage matrix, and stock render. The change is purely additive: one new leaf, no existing leaf hash moved.Consistent with its
*-anypeers, this leaf declares no conformance or performance checks (R:0 D:4 P:0 C:0).Testing
Also verified against real L40 hardware — a 2-node bare-metal cluster, K8s v1.35.3,
NVIDIA L40, driver580.173.02, gpu-operatorv26.3.0:Two deployment checks did not evaluate on that cluster, both for environmental reasons unrelated to this overlay:
check-nvidia-smi→Skip:IsNodeGpuBusyreports the node busy because a resident production pod holds a GPU. The check requires a node with zero GPU-requesting pods.expected-resources→ timeout: the cluster has no AICR bundle deployed, so 7 of the recipe's 11 declared components are absent and the poll never converges.As a control, the already-merged
l40s-anyrecipe was run against the same cluster with the same flags and produced identical outcomes —operator-healthpassed,gpu-operator-versionpassed,check-nvidia-smiskipped,expected-resourcestimed out, with the same messages. Both non-passing checks are therefore properties of this environment, not of the new overlay.Note:
make qualifywas not run in full — it needs the complete pinned toolchain (Docker, goreleaser, e2e). The targeted suites above cover the changed surface; the three golden tests are what gate a new leaf.Risk Assessment
Rollout notes: Data-only. No code paths change. Queries that previously errored on
accelerator=l40now resolve; no existing recipe is affected.Checklist
make testwith-race) — rango test ./pkg/recipe/... ./pkg/bundler/; fullmake qualifynot run (see Testing)make lint) — ranmake lint-yamlandmake licensedocs/user/recipe-health.mdis bot-regenerated and was intentionally left out; happy to include it if preferredgit commit -S)