Repository navigation
fix(certification): write render annotations in offline certification render - #532
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. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely identifies the main fix: writing render annotations during offline certification rendering. |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description check | |
The PR description is specific and on-topic. It explains that offline nvcrectl certification render discarded the nvcrectl.nvidia.com/ annotations (detected-platform, detected-gpu-architecture, ap… |
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
Full details: Description check
-
Explanation
The PR description is specific and on-topic. It explains that offline
nvcrectl certification renderdiscarded thenvcrectl.nvidia.com/annotations (detected-platform, detected-gpu-architecture, applied-overrides), names the code change (render.SetRenderAnnotationsinresolveWorkflowsOffline), lists the docs updates, the new testTestCertificationRenderAnnotationswith its five cases, and the testing performed. This matches the changeset summary and the PR objectives, so it passes.
✨ Finishing Touches: 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- 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 fix looks correct and the tests are solid. I merged it onto current main, and build, tests and lint pass. One thing needs to change before merge:
Attribution. The commit has a Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> trailer, so GitHub lists a second author, and the PR description ends with a "Generated with Claude Code" footer. CLAUDE.md ("Git Configuration") and CONTRIBUTING.md (AI-assisted contributions) don't allow agent attribution in commits or PR bodies. The DCO check only looks at Signed-off-by, so CI won't flag it. Please amend the commit to drop the trailer (keep -S -s) and remove the footer from the description.
… render Offline "nvcrectl certification render" (no --dry-run) resolved overrides against its synthetic node but discarded the result, so the rendered Workflows carried none of the nvcrectl.nvidia.com/ annotations (detected-platform, detected-gpu-architecture, applied-overrides). The --dry-run path and "nvcrectl workflow render", offline included, have always written them, and CLAUDE.md/AGENTS.md and ADR-075's testing plan tell readers to check them in offline render output. No ADR, doc or commit gives a reason for leaving them out; the discard dates to the initial commit. resolveWorkflowsOffline now passes the resolve metadata to render.SetRenderAnnotations. The values come from the synthetic node, so they record what override matching actually saw. That makes visible a case the output used to hide: without --platform the synthetic node has no providerID, detects as onprem, and the on-prem overrides apply, so the render is identical to --platform onprem. The annotations are informational. The controller never reads them, and no existing golden captures certification render stdout, so no existing test output changes. TestCertificationRenderAnnotations pins the three annotations through resolveWorkflowsOffline for AWS GB200 (two categories), AWS L20, GB300 without --platform (onprem), Nscale with --gpu-arch and no product label, and on-prem H100, where nothing matches and applied-overrides is omitted. The docs now name certification render beside workflow render, and CLAUDE.md/AGENTS.md note the onprem default. Signed-off-by: Kang Zhang <kangz@nvidia.com>
05fcde1 to
bd27c1b
Compare
ndipebot
left a comment
ndipebot
left a comment
There was a problem hiding this comment.
The fix itself is right and the shape matches the other paths: resolveWorkflowsOffline now calls render.SetRenderAnnotations with the same metadata the --dry-run branch at certification.go:247 uses, the rendered spec is untouched, and the five goldens go through testutil.TestCaseParser. No argument from me there.
One thing I would change before this lands. The new goldens pin positional override indices that come out of pkg/catalog/entries/communication/nccl-all-reduce.yaml, and no golden in the repo did that before. nscale-gpu-arch-flag/expected.json pins "index": 18 (the # --- Nscale NKS block at line 619 of that file), no-platform-gb300-nccl pins 15, aws-gb200-nccl pins 5, 6, 7, 8. That makes pkg/certification fail whenever someone inserts an override block earlier in the catalog entry, even though their PR and this one are each green on their own branch. I checked out this head, dropped in the Akamai block that #537 adds at @@ -612,6 +612,22 @@ of that same file, right above the Nscale anchor, and TestCertificationRenderAnnotations/nscale-gpu-arch-flag fails with want 18, got 19. Nothing else in ./pkg/... newly fails, so it is purely the pinned index.
The existing goldens that carry an index all derive it from somewhere stable: workloadrun-mpi indexes pkg/platform/overrides/workloadrun.yaml, workflow-standalone-unresolved-render indexes the overrides: block written inline in its own input at line 154. I could not find one that indexes a catalog entry file. Dropping Index from the annotationsWorkflow projection in render_annotations_test.go keeps the whole assertion: when and patches already say which blocks matched, and the ordinal adds nothing a reader of the golden can use.
The other item is smaller and it is a docs wording thing. The new sentence (repeated in docs/concepts/platform-detection.md and CLAUDE.md/AGENTS.md) says an offline render without --platform detects onprem. That holds for a plain nodeSelector, but syntheticRenderNode copies the whole selector onto the synthetic node, so a target carrying nke.nvidia.com/site-name or a Forge-ish kubernetes.io/hostname detects as forge instead. I built nvcrectl from this head and a GB300 cert with nke.nvidia.com/site-name: az24 renders detected-platform: forge with no flag, and still forge under --platform onprem, because render.SyntheticProviderID("onprem") returns "" and the offline path forwards no platform name anyway. --platform aws on the same cert does give aws, so the flag works for every cloud value; only onprem and forge are no-ops.
That precedence predates this change, so it is your call whether to touch it here. Hedging the sentence ("unless the nodeSelector carries a Forge marker") is enough. If you would rather make the flag mean what the docs say, swapping certification.go:313 to render.ResolveWorkflowForPlatform(&workflows[i], syntheticNodes, platformFlag) lines the offline branch up with the dry-run one; I tried it and go test ./pkg/certification/ stays green, including all five new goldens, though none of them use a Forge-marked selector so they do not actually cover the difference.
kaynetu
left a comment
kaynetu
left a comment
There was a problem hiding this comment.
Please fix the CI error
…tion goldens The render annotation goldens pinned each applied override's index, which is its position in the catalog entry's overrides list. Inserting any block above it in pkg/catalog/entries/communication/nccl-all-reduce.yaml shifts the ordinal, so the goldens broke without any change to this code: after main brought in NVIDIA#537's Akamai block, nscale-gpu-arch-flag wanted 18 and got 19, failing Test on the PR head. The projection now records only when, patches and noOp, which still identify the blocks that matched. The four affected goldens change only by losing their index lines. The docs also said an offline render without --platform detects onprem. That holds for a plain nodeSelector, but syntheticRenderNode copies the whole selector onto the synthetic node, so a Forge marker (nke.nvidia.com/site-name, or a hostname containing forge) detects as forge instead, and --platform onprem does not change it because its synthetic providerID is empty. That precedence predates this change. The sentence is hedged in docs/cli-reference/certification.md, docs/concepts/platform-detection.md, CLAUDE.md/AGENTS.md and the resolveWorkflowsOffline comment; matching behaviour is unchanged. Signed-off-by: Kang Zhang <kangz@nvidia.com>
ndipebot
left a comment
ndipebot
left a comment
There was a problem hiding this comment.
Dropping Index from the projection is the right call. when and patches already carry the assertion that the right blocks matched, and all five annotation goldens come back with no index at all now, so nothing in pkg/certification is coupled to the ordering of a catalog entry file any more. #537 landing with its Akamai block above the Nscale anchor would have broken the old goldens, and it cannot now.
|
/ok-to-test 227d211 |
Summary
Offline
nvcrectl certification render(without--dry-run) resolved overrides against its synthetic render node, then discarded the result. The rendered Workflows carried none of thenvcrectl.nvidia.com/annotations:detected-platform,detected-gpu-architecture, andapplied-overrides. Other paths already write them: the--dry-runbranch of the same command, andnvcrectl workflow renderin all of its modes, offline included. CLAUDE.md/AGENTS.md ("What to Verify in Output", step 1) and ADR-075's testing plan both tell readers to check these annotations in offline render output, which was impossible. No ADR, doc, or commit gives a reason for leaving them out; the discard dates to the initial commit and was carried over unchanged when #303 extractedresolveWorkflowsOffline.resolveWorkflowsOfflinenow passes the resolve metadata torender.SetRenderAnnotations, the same call the dry-run branch makes:The values come from the synthetic node, so they record what override matching actually saw. That exposes a case the output used to hide. Without
--platform, the synthetic node has noproviderID, so it detects asonpremand the on-prem overrides apply. A render without--platformis byte-identical to--platform onprem(checked on GB300communication/nccl-all-reduce, which picks up the on-prem tolerations). It now saysdetected-platform: onprem. This PR does not change that matching behaviour; it only makes it visible. One exception predates this PR: anodeSelectorcarrying a Forge marker (nke.nvidia.com/site-name, or akubernetes.io/hostnamecontainingforge) detects asforge, with or without--platform onprem, and the docs say so.Impact: the annotations are informational. The controller never reads
nvcrectl.nvidia.com/*, and--dry-runoutput (which can also be applied) has always carried them. The renderedspecdoes not change. No existing golden capturescertification renderstdout, so no existing test output changes.Docs:
docs/concepts/platform-detection.mdnamescertification renderbesideworkflow renderfor both annotation paragraphs, and explains theonpremdefault.docs/cli-reference/certification.mdgets one paragraph describing the annotations.--platformreportsonprem. They stay identical (make check-agents-syncpasses).Related Issue
None. This is a trivial fix, which is exempt from the issue-first rule.
Type of Change
Component(s) Affected
Testing
New test:
TestCertificationRenderAnnotations(pkg/certification/render_annotations_test.go,testutil.TestCaseParser) drivesresolveWorkflowsOffline, the functionrunCertificationRendercalls without--dry-run. It records everynvcrectl.nvidia.com/annotation per Workflow, withapplied-overridesdecoded and recorded aswhen,patchesandnoOp. The override'sindexis left out: it is the block's position in the catalog entry file and shifts whenever a block is inserted above it, as #537 did. Five new goldens, no existing goldens touched:aws-gb200-nccl(all-reduce + loopback)awsgb200gpuArchitecture=gb200, platform=aws, per Workflowaws-l20-ncclawsl20notIn [gb200,gb300])no-platform-gb300-ncclonpremgb300in [gb200,gb300], platform=onpremnscale-gpu-arch-flag(--gpu-arch NVIDIA-GB300, no product label)nscalegb300in [b200,gb300], platform=nscaleonprem-h100-nccl-no-overrideonpremh100Tests run:
SetRenderAnnotationscall removed, all five cases fail.nvcrectl certification render --platform awson anNVIDIA-L20cert (communication/nccl-all-reduce) printed 0 lines matchingnvcrectl.nvidia.com/. It now prints all three annotations, in YAML and in--output json.make lint-fix: 0 issues.make build: passes.make test: unit and envtest integration tests pass.make verify:verify-codegen,verify-license-headersandverify-doc-linkspass.verify-modfirst failed on a proxy.golang.org TLS timeout, then passed on retry.go.modandgo.sumare unchanged.Not run:
--dry-runagainst a live cluster: that branch is unchanged.Checklist
git commit -s)make manifests generaterun (if*_types.gowas modified) — N/A, no type changes