Skip to content

feat(recipes): add l40-any accelerator overlay - #2366

Open
ntheanh201 wants to merge 1 commit into
NVIDIA:mainfrom
ntheanh201:feat/recipes-l40-any
Open

feat(recipes): add l40-any accelerator overlay#2366
ntheanh201 wants to merge 1 commit into
NVIDIA:mainfrom
ntheanh201:feat/recipes-l40-any

Conversation

@ntheanh201

Copy link
Copy Markdown

Summary

Adds the missing l40-any criteria-wildcard overlay. l40 is already a registered accelerator type, but no overlay provides it, so any query carrying accelerator=l40 fails to resolve.

Motivation / Context

The gap is reachable from a live cluster, not just from explicit flags. aicr snapshot on an L40 node detects accelerator: l40 from nodeTopology.label.nvidia.com/gpu.product, but the snapshot-driven aicr recipe --snapshot then fails:

[INVALID_REQUEST] no recipe provides accelerator 'l40' for criteria(accelerator=l40)

The accelerator type itself is valid — --accelerator foobar is rejected at parse time with invalid accelerator type, while --accelerator l40 parses and fails only at resolution. README.md also lists L40 under supported accelerators, so the catalog and the docs currently disagree.

Fixes: #TBD
Related: N/A

Type of Change

  • New feature (non-breaking change that adds functionality)

Component(s) Affected

  • Recipe engine / data (pkg/recipe)
  • Bundlers (pkg/bundler, pkg/component/*) — golden only, no code change
  • Other: recipes/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 plus Deployment.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 *-any peers, this leaf declares no conformance or performance checks (R:0 D:4 P:0 C:0).

Testing

go test ./pkg/recipe/... ./pkg/bundler/     # pass
make lint-yaml                              # pass
make license                                # pass

Also verified against real L40 hardware — a 2-node bare-metal cluster, K8s v1.35.3, NVIDIA L40, driver 580.173.02, gpu-operator v26.3.0:

readiness constraint passed: K8s.server.version  >= 1.25 vs v1.35.3
validator completed: operator-health        status=passed
validator completed: gpu-operator-version   status=passed   # >= v24.6.0 vs v26.3.0

Two deployment checks did not evaluate on that cluster, both for environmental reasons unrelated to this overlay:

  • check-nvidia-smiSkip: IsNodeGpuBusy reports 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-any recipe was run against the same cluster with the same flags and produced identical outcomes — operator-health passed, gpu-operator-version passed, check-nvidia-smi skipped, expected-resources timed out, with the same messages. Both non-passing checks are therefore properties of this environment, not of the new overlay.

Note: make qualify was 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

  • Low — Isolated change, well-tested, easy to revert

Rollout notes: Data-only. No code paths change. Queries that previously errored on accelerator=l40 now resolve; no existing recipe is affected.

Checklist

  • Tests pass locally (make test with -race) — ran go test ./pkg/recipe/... ./pkg/bundler/; full make qualify not run (see Testing)
  • Linter passes (make lint) — ran make lint-yaml and make license
  • I did not skip/disable tests to make CI green
  • I added/updated tests for new functionality — the catalog-parity, coverage-matrix, and stock-render goldens cover the new leaf automatically
  • I updated docs if user-facing behavior changed — docs/user/recipe-health.md is bot-regenerated and was intentionally left out; happy to include it if preferred
  • Changes follow existing patterns in the codebase
  • Commits are cryptographically signed (git commit -S)

@ntheanh201
ntheanh201 requested review from a team as code owners August 25, 2026 01:13
@copy-pr-bot

copy-pr-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

Copy link
Copy Markdown
Contributor

Welcome to AICR, @ntheanh201! Thanks for your first pull request.

Before review, please ensure:

  • All commits are signed off per the DCO
  • CI checks pass (tests, lint, security scan)
  • The PR description explains the why behind your changes

A maintainer will review this soon.

@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 16a7c221-1ba6-4f65-a61c-6a7a39a7ff7a

📥 Commits

Reviewing files that changed from the base of the PR and between de6514b and f004691.

📒 Files selected for processing (4)
  • pkg/bundler/testdata/stock_render_golden.yaml
  • pkg/recipe/testdata/catalog_parity_golden.yaml
  • pkg/recipe/testdata/coverage_golden.yaml
  • recipes/overlays/l40-any.yaml

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The change adds the l40-any wildcard recipe metadata overlay. It applies deployment checks, NVIDIA SMI validation, and a minimum GPU Operator version of v24.6.0. Golden files now include the overlay digest, catalog digest, and successful unconstrained L40 coverage entry.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to f0046

This adds the missing L40 recipe overlay so valid L40 queries resolve without changing existing recipes or code paths; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: yuanchen8911

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: adding the l40-any accelerator overlay.
Description check ✅ Passed The description directly explains the missing overlay, its behavior, supporting goldens, testing, and rollout impact.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Recipe evidence check

No leaf overlays affected by this PR.

This gate is warning-only and never blocks merge.

`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>
@ntheanh201
ntheanh201 force-pushed the feat/recipes-l40-any branch from 2555634 to f004691 Compare August 27, 2026 01:06
@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

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.

@mchmarny mchmarny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve: no findings against f004691.

@mchmarny
mchmarny self-requested a review August 28, 2026 12:14

@njhensley njhensley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

▎ 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 l40 resolution (persona-flagged, refuted). With no concrete l40 leaves, would --accelerator l40 --intent training still error? Refuted empirically against the embedded catalog: service=eks, accelerator=l40, intent=training resolves 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 training with no service, returns the identical generic intent 'training' requires additional criteria error that l40s (which has concrete leaves) returns — a universal "intent needs a service" guard, not an l40 gap. The PR's motivating error was the bare criteria(accelerator=l40) query, which now succeeds.
  • Goldens additive-only. catalog_parity and stock_render each +1 hash line, coverage +2 lines, inserted in correct sorted position; no existing leaf hash moved (verified against the upstream merge-base de6514b13, not the stale local fork origin/main).
  • BOM unaffected. Overlay uses base: base with no componentRefs, adds no images; registry.yaml untouched, so TestCommittedBOMVersionsMatchRegistry scope is unchanged.
  • No enum/contract drift. l40 (pre-existing value) is already co-listed with l40s in README, OpenAPI (all enum blocks), cli-reference, api-reference, recipe.md, doc.go, recipe.go, and the bug-report issue template.
  • >= v24.6.0 floor 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 referenced criteria-wildcard-overlays anchor 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.

@github-actions

Copy link
Copy Markdown
Contributor

@ntheanh201 this PR now has merge conflicts with main. Please rebase to resolve them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants