feat(recipe): merge external validator catalog with embedded when provided through DataProvider#588
Merged
mchmarny merged 4 commits intoNVIDIA:mainfrom Apr 16, 2026
Conversation
…rge-by-name Route catalog loading through the global DataProvider so the --data flag enables external validator catalog overlays with the same merge-by-name semantics used for the component registry. Extract a generic mergeEmbeddedAndExternal[T] helper to deduplicate the shared load-unmarshal-merge-marshal pipeline between registry and catalog merging.
ArangoGutierrez
approved these changes
Apr 16, 2026
Contributor
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Nice work on this one. The design is clean:
mergeByName[T]generic avoids duplicating the merge-by-name logic between registry and catalog.mergeEmbeddedAndExternal[T]extracts the load-unmarshal-merge-serialize pattern into a reusable function.catalogForMergeusingmap[string]interface{}to avoid the circular dependency withpkg/validator/catalogis the right call.sync.Oncecaching mirrors the existing registry pattern.- Test coverage is thorough: merge, override-by-name, fallback when no external catalog, source tracking, caching, invalid YAML, and order preservation.
The GPU workflow failures (training/inference) are unrelated to this change -- this PR only touches recipe provider and catalog code, and all core CI (unit tests, lint, E2E, security scan) pass.
One minor note: the Load() error message changed from "failed to read embedded catalog" to "failed to read catalog" which makes sense since it now reads from the layered provider, not just embedded.
LGTM. Welcome to the project.
mchmarny
approved these changes
Apr 16, 2026
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis change extends the layered data provider to merge external Changes
Sequence DiagramssequenceDiagram
participant CLI as CLI (--data flag)
participant Provider as DataProvider
participant EmbedFS as Embedded FS
participant ExtFS as External FS
participant Catalog as catalog.Load()
rect rgba(100, 150, 200, 0.5)
Note over CLI,Catalog: Before: Direct Embedded Read
Catalog->>EmbedFS: ReadFile(validators/catalog.yaml)
EmbedFS-->>Catalog: embedded catalog YAML
Catalog-->>CLI: loaded catalog
end
rect rgba(150, 200, 100, 0.5)
Note over CLI,Catalog: After: Layered Merge via DataProvider
CLI->>Provider: ReadFile(validators/catalog.yaml)
Provider->>ExtFS: check external catalog exists
alt External catalog found
Provider->>ExtFS: read external catalog
Provider->>EmbedFS: read embedded catalog
Provider->>Provider: merge by name (external overrides)
Provider-->>Catalog: merged YAML (cached)
else No external catalog
Provider->>EmbedFS: read embedded catalog
Provider-->>Catalog: embedded YAML
end
Catalog-->>CLI: loaded catalog
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Route the validator catalog through the
DataProviderso the--dataflag can overlay externalvalidators/catalog.yamlwith merge-by-name semantics, matching howregistry.yamlalready works.Motivation / Context
catalog.Load()read exclusively from the embedded filesystem (recipes.FS), bypassing theDataProviderinterface that--dataalready uses for registry, overlays, and component values. Teams needing custom or cluster-specific validation checks (e.g., ComputeDomain-aware DRA tests for GB200 clusters) had to fork and rebuild the CLI.Fixes: #528
Related: N/A
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/api,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
Merge semantics (same as
registry.yaml):validators/catalog.yamlis optional (unlikeregistry.yamlwhich is required)Key design decisions:
Merge in
LayeredDataProvider, not incatalogpackage. The provider handles merge transparently —catalog.Load()just reads from the provider and gets pre-merged bytes. This matches how registry merging works and keeps theDataProvider.ReadFile()contract clean.catalogForMergeproxy type withmap[string]interface{}. The provider (pkg/recipe) cannot importpkg/validator/catalogbecause catalog already importspkg/recipeforGetDataProvider()— that would be circular. The proxy type preserves all YAML fields through the round-trip. Validation of the merged result still happens incatalog.Parse()→validate().Generic
mergeByName[T any]andmergeEmbeddedAndExternal[T any]. Both registry and catalog merge shared identical algorithms. These were extracted into generic helpers to eliminate the duplication. ThefileReaderinterface keepsmergeEmbeddedAndExternaldecoupled from the fullDataProvider.Backward compatibility: When no
--dataflag is set,GetDataProvider()returns the defaultEmbeddedDataProvider, which reads fromrecipes.FS— identical behavior to before.Testing
8 new tests in
pkg/recipe/provider_test.go:MergesCatalog— new external validators appended (16 + 1 = 17)CatalogOverrideByName— same-name replacement (16 + 1 same-name = 16, image/desc changed)CatalogMergePreservesOrder— embedded order preserved, new entries at endCatalogNoCatalogInExternal— fallback to embedded when no external catalogCatalogSourceMerged—Source()reports "merged" when external existsCachedCatalog—sync.Oncereturns identical bytes on repeated callsInvalidExternalCatalog— malformed YAML returns error, no panicCatalogMergePreservesOrder— override + append in one operation1 new integration test in
pkg/validator/catalog/catalog_test.go:TestLoadWithExternalCatalog— end-to-end: setsLayeredDataProviderglobally, callscatalog.Load(), verifies typedValidatorEntrystructs from both embedded and external sourcesCoverage delta:
pkg/recipe: 90.3% → 90.7% (+0.4%)pkg/validator/catalog: 80.4% → 80.4% (no change)Risk Assessment
Rollout notes: No migration needed. The
--dataflag behavior is additive — existing users withoutvalidators/catalog.yamlin their external directory see no change. The catalog merge path is only entered when the external file exists.Checklist
make testwith-race)make lint)git commit -S) — GPG signing infoSummary by CodeRabbit
New Features
--dataflag documentation to clarify that both registry components and validator catalog entries are merged when overlaying external dataTests