refactor(recipe,validator): per-DataProvider sources + criteria-registry de-globalization by hkii · Pull Request #1107 · NVIDIA/aicr

hkii · 2026-05-29T21:47:07Z

Summary

Thread an explicit DataProvider through the recipe loader, validator catalog, and criteria parsing so consumers can resolve against their own data source instead of the process-global provider/registry. Foundation (PR 1 of 2) for migrating the CLI/REST onto aicr.Client (#1077).

What changes

recipe: GetCriteriaRegistryFor(dp) per-provider criteria registry (+ EvictCachedCriteriaRegistry); registry-aware parsing + BuildCriteriaWithRegistry; LoadFromFileWithProvider; RecipeResult.DeepCopy/BindDataProvider.
validator: catalog.LoadWithDataProvider(dp, …) + WithDataProvider option.
config: ResolveCriteriaWithRegistry.

Behavior

No consumer-facing behavior change. Global shims are retained for back-compat (DefaultRegistry, ParseCriteria*, catalog.Load, LoadFromFile, config.ResolveCriteria) so existing callers compile and behave identically. The criteria-registry global is distinct from SetDataProvider/GetDataProvider (#987, closed); shim removal will be tracked separately.

Cache lifecycle

This PR adds criteriaRegistryCache (per-DataProvider, sync.Map-backed, lazy via sync.Once) alongside the existing registryCache (pkg/recipe/components.go) and storeCache (pkg/recipe/metadata_store.go). All three caches grow on first access per DataProvider and have Evict* primitives but no automatic eviction from this PR alone.

Client.Close() wiring that calls EvictCachedCriteriaRegistry, EvictCachedStore, and EvictCachedRegistry for the Client's DataProvider lands in #1108. Long-running consumers that churn through Clients should wait for #1108 (or evict manually) to avoid monotonic cache growth.

Testing

go build ./..., go test -race ./pkg/recipe/... ./pkg/validator/... ./pkg/config/..., and golangci-lint all pass standalone.

Relates to #1077. PR 2 (CLI/API migration) stacks on this branch.

…stry de-globalization Thread an explicit DataProvider through the recipe loader, validator catalog, and criteria parsing so consumers can resolve against their own data source instead of the process-global provider/registry. Global shims are retained for back-compat (DefaultRegistry, ParseCriteria*, catalog.Load, LoadFromFile, config.ResolveCriteria) so existing callers are unaffected. - recipe: GetCriteriaRegistryFor(dp) per-provider registry + EvictCachedCriteriaRegistry; registry-aware parse + BuildCriteriaWithRegistry; LoadFromFileWithProvider; RecipeResult.DeepCopy/BindDataProvider - validator: catalog.LoadWithDataProvider(dp,...) + WithDataProvider option - config: ResolveCriteriaWithRegistry Foundation for migrating the CLI/REST consumers onto aicr.Client (#1077).

coderabbitai · 2026-05-29T21:53:17Z

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: b9bbb5f4-46b2-4e47-9dde-fbe5a1e7a503

📥 Commits

Reviewing files that changed from the base of the PR and between 3b7f7d8 and f33eaef.

📒 Files selected for processing (3)

pkg/recipe/criteria.go
pkg/recipe/provider.go
pkg/validator/catalog/catalog.go

📝 Walkthrough

<review_stack_artifact>

</review_stack_artifact>

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately captures the main refactoring work: threading explicit DataProvider through the codebase and de-globalizing the criteria registry.
Description check	✅ Passed	The description is directly related to the changeset, providing clear context about the architectural changes, backward compatibility, cache implications, and testing performed.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch refactor/aicr-recipe-deglobalization

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

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/validator/catalog/catalog.go`:
- Around line 72-77: The LoadWithDataProvider function performs unbounded I/O
via dp.ReadFile(...) and must accept a context so callers can enforce timeouts;
change the signature of LoadWithDataProvider(dp recipe.DataProvider, version,
commit string) to include ctx context.Context, update callers to pass a
timeout-bounded context from validator entrypoints, and replace the dp.ReadFile
call with the context-aware DataProvider method (e.g., dp.ReadFile(ctx,
"validators/catalog.yaml") or the provider's equivalent). Ensure you do not
introduce context.Background() anywhere in the implementation—propagate the
provided ctx through the read and any downstream calls and handle context errors
appropriately (cancel/return on ctx.Err()).

🪄 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: 7b94424c-f8da-4621-b32b-25a5451ceb76

📥 Commits

Reviewing files that changed from the base of the PR and between fc8ea41 and 3b7f7d8.

📒 Files selected for processing (14)

pkg/config/resolve.go
pkg/recipe/components.go
pkg/recipe/criteria.go
pkg/recipe/criteria_registry.go
pkg/recipe/criteria_registry_test.go
pkg/recipe/loader.go
pkg/recipe/loader_provider_test.go
pkg/recipe/metadata.go
pkg/recipe/metadata_store.go
pkg/validator/catalog/catalog.go
pkg/validator/catalog/catalog_test.go
pkg/validator/options.go
pkg/validator/types.go
pkg/validator/validator.go

github-actions · 2026-05-29T21:58:02Z

Coverage Report ✅

Metric	Value
Coverage	76.6%
Threshold	75%
Status	Pass

Coverage Badge

![Coverage](https://img.shields.io/badge/coverage-76.6%25-green)

Merging this branch changes the coverage (3 decrease, 1 increase)

Impacted Packages	Coverage Δ	🤖
github.com/NVIDIA/aicr/pkg/config	92.46% (-0.22%)	👎
github.com/NVIDIA/aicr/pkg/recipe	87.25% (-4.62%)	👎
github.com/NVIDIA/aicr/pkg/validator	24.31% (-0.23%)	👎
github.com/NVIDIA/aicr/pkg/validator/catalog	97.56% (+0.09%)	👍

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/NVIDIA/aicr/pkg/config/resolve.go	94.36% (-0.43%)	195 (+3)	184 (+2)	11 (+1)	👎
github.com/NVIDIA/aicr/pkg/recipe/components.go	85.59% (-4.04%)	111 (+5)	95	16 (+5)	👎
github.com/NVIDIA/aicr/pkg/recipe/criteria.go	81.96% (-9.71%)	388 (+52)	318 (+10)	70 (+42)	👎
github.com/NVIDIA/aicr/pkg/recipe/criteria_registry.go	100.00% (ø)	93 (+13)	93 (+13)	0
github.com/NVIDIA/aicr/pkg/recipe/loader.go	91.67% (+2.78%)	24 (+6)	22 (+6)	2	👍
github.com/NVIDIA/aicr/pkg/recipe/metadata.go	81.90% (-15.45%)	315 (+50)	258	57 (+50)	💀
github.com/NVIDIA/aicr/pkg/recipe/metadata_store.go	86.67% (-1.22%)	360 (+5)	312	48 (+5)	👎
github.com/NVIDIA/aicr/pkg/recipe/provider.go	91.67% (ø)	228	209	19
github.com/NVIDIA/aicr/pkg/validator/catalog/catalog.go	97.56% (+0.09%)	82 (+3)	80 (+3)	2	👍
github.com/NVIDIA/aicr/pkg/validator/options.go	66.67% (-6.06%)	24 (+2)	16	8 (+2)	👎
github.com/NVIDIA/aicr/pkg/validator/types.go	0.00% (ø)	0	0	0
github.com/NVIDIA/aicr/pkg/validator/validator.go	19.47% (ø)	190	37	153

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

mchmarny

Clean foundation refactor — thanks for splitting this off, the per-provider isolation model is well-shaped and the doc comments are exceptional. Per-provider isolation test, eviction test, strict-per-registry test, and the marker-validator proof in TestLoadHonorsExplicitProvider together pin the behavior I'd want pinned. RecipeResult.DeepCopy handling nested Overrides via deepCopyAnyMap cleanly addresses the prior aliasing bug. All 32 checks green.

Four discussion points, none blocking:

M1 (criteria_registry.go): sync.Map keyed by DataProvider interface needs the dynamic type to be comparable — worth one sentence in the DataProvider interface godoc so external implementers don't trip a runtime panic.
M2 (criteria.go): the new With<X>Registry / BuildCriteriaWithRegistry ship as a parallel API alongside the existing With<X> / BuildCriteria — what's the long-term plan? Either Deprecated: markers on the originals or a doc note on why both coexist.
M3 (criteria_registry.go): cache-eviction wiring for the new criteriaRegistryCache lives in #1108 — worth a forward reference in this PR's body so the cache lifecycle isn't a hidden assumption.
L1 (nit): //nolint:staticcheck reference style varies (#983 Stage 2 vs #983/#987); standardize.

Happy to take this as-is once M1/M2/M3 get a quick pass.

Address review feedback on #1107: - DataProvider interface godoc now states the comparability requirement (the dynamic type must be comparable per the Go language spec) so external implementers don't trip a runtime panic from the sync.Map keys used by metadata-store, component-registry, and criteria-registry caches (M1). - CriteriaOption and BuildCriteria godoc now correctly describe their relationship to RegistryCriteriaOption / BuildCriteriaWithRegistry, and explicitly point per-provider callers at the new path. Drops the misleading "shim over BuildCriteriaWithRegistry" claim that didn't match the actual call structure (M2). - catalog.go nolint:staticcheck reference style now matches criteria_registry.go: "back-compat fallback for pre-WithDataProvider callers (#983 Stage 2)" (L1). No functional change.

mchmarny · 2026-05-29T22:21:29Z

Pushed eac3ecf addressing M1/M2/L1 inline; updated PR body with a Cache lifecycle section addressing M3. Summary:

M1 pkg/recipe/provider.go: DataProvider interface godoc now states the comparability requirement and explains the sync.Map foot-gun for external implementers.
M2 pkg/recipe/criteria.go: CriteriaOption and BuildCriteria docs now correctly describe the relationship to RegistryCriteriaOption / BuildCriteriaWithRegistry (dropping the misleading 'shim over' wording) and explicitly point per-provider callers at the new path. No Deprecated: markers yet — deferring those to a follow-up PR after feat(cli,api): consume aicr.Client facade (#1077) #1108 migrates in-tree callers to avoid SA1019 churn.
L1 pkg/validator/catalog/catalog.go: //nolint:staticcheck reference style now matches criteria_registry.go.
M3 PR body: added a "Cache lifecycle" section noting all three caches (criteriaRegistryCache + the existing registryCache / storeCache) have their Close() wiring landing in feat(cli,api): consume aicr.Client facade (#1077) #1108.

make test + golangci-lint clean on the affected packages. Stepping back from this review — handing off to another reviewer.

github-actions Bot added area/validator size/XL labels May 29, 2026

hkii mentioned this pull request May 29, 2026

feat(cli,api): consume aicr.Client facade (#1077) #1108

Merged

coderabbitai Bot reviewed May 29, 2026

View reviewed changes

Comment thread pkg/validator/catalog/catalog.go

mchmarny mentioned this pull request May 29, 2026

feat(cli,api): migrate CLI and REST server to consume aicr.Client #1104

Closed

23 tasks

mchmarny reviewed May 29, 2026

View reviewed changes

Comment thread pkg/recipe/criteria_registry.go

Comment thread pkg/recipe/criteria.go

Comment thread pkg/recipe/criteria_registry.go

Comment thread pkg/validator/catalog/catalog.go Outdated

mchmarny mentioned this pull request May 29, 2026

[Epic]: Post-facade adoption and surface hardening (follow-ups to #1072) #1076

Closed

mchmarny self-assigned this May 29, 2026

Merge branch 'main' into refactor/aicr-recipe-deglobalization

f33eaef

mchmarny marked this pull request as ready for review May 29, 2026 22:22

mchmarny requested a review from a team as a code owner May 29, 2026 22:22

mchmarny approved these changes May 29, 2026

View reviewed changes

mchmarny mentioned this pull request May 29, 2026

feat(recipe): add context.Context to DataProvider interface for cancellable I/O #1109

Closed

6 tasks

mchmarny enabled auto-merge (squash) May 29, 2026 22:24

mchmarny merged commit 01948eb into main May 29, 2026
31 checks passed

mchmarny deleted the refactor/aicr-recipe-deglobalization branch May 29, 2026 22:33

mchmarny added a commit that referenced this pull request May 30, 2026

chore(recipe): remove back-compat shims introduced by #1107 (#1118)

338a519

This was referenced May 30, 2026

docs(recipe): refresh data/validator docs and godoc after #1118 #1120

Merged

feat(recipe): context-aware DataProvider interface #1121

Merged

refactor: code quality and consistency cleanup #1126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(recipe,validator): per-DataProvider sources + criteria-registry de-globalization#1107

refactor(recipe,validator): per-DataProvider sources + criteria-registry de-globalization#1107
mchmarny merged 3 commits into
mainfrom
refactor/aicr-recipe-deglobalization

hkii commented May 29, 2026 •

edited by mchmarny

Loading

Uh oh!

coderabbitai Bot commented May 29, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented May 29, 2026 •

edited

Loading

Changed files (no unit tests)

Uh oh!

mchmarny left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mchmarny commented May 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

hkii commented May 29, 2026 • edited by mchmarny Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changes

Behavior

Cache lifecycle

Testing

Uh oh!

coderabbitai Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report ✅

Merging this branch changes the coverage (3 decrease, 1 increase)

Changed files (no unit tests)

Uh oh!

mchmarny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mchmarny commented May 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hkii commented May 29, 2026 •

edited by mchmarny

Loading

coderabbitai Bot commented May 29, 2026 •

edited

Loading

github-actions Bot commented May 29, 2026 •

edited

Loading