feat(inference): add declarative serving catalog compiler - #8152
Conversation
ericksoa
added
area: inference
📝 WalkthroughWalkthroughAdds strict managed-inference schemas and TypeScript contracts. The compiler validates trusted YAML, enforces semantic references, produces deterministic digests, and writes a compiled catalog during CLI builds. Tests verify compilation, parsing, schema validity, and npm packaging. ChangesManaged inference catalog
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant RecipePresetYAML
participant generateServingCatalog
participant compileTrustedServingCatalog
participant CatalogJSON
RecipePresetYAML->>generateServingCatalog: discover and read trusted sources
generateServingCatalog->>compileTrustedServingCatalog: pass schemas and registries
compileTrustedServingCatalog->>compileTrustedServingCatalog: validate and compute digests
compileTrustedServingCatalog->>CatalogJSON: write canonical catalog JSON
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit dc31cca in the TypeScript / code-coverage/cliThe overall coverage in commit dc31cca in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-8152.docs.buildwithfern.com/nemoclaw |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/package-contract/managed-inference-catalog.test.ts (1)
20-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the source revision explicitly.
The title claims a source revision check. The body proves it only indirectly, through schema validation inside
parseCompiledServingCatalogJson. An explicit assertion makes the failure output name the offending field.✅ Proposed assertion
- parseCompiledServingCatalogJson(source, { + const catalog = parseCompiledServingCatalogJson(source, { catalog: catalogSchema, preset: presetSchema, recipe: recipeSchema, }); + + expect(catalog.sourceRevision).toMatch(/^[0-9a-f]{40,64}$/); + expect(catalog.catalogDigest).toMatch(/^sha256:[0-9a-f]{64}$/);🤖 Prompt for 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. In `@test/package-contract/managed-inference-catalog.test.ts` around lines 20 - 30, Update the test around parseCompiledServingCatalogJson to explicitly assert that the compiled catalog contains a source revision, using the parsed catalog result and its source-revision field. Keep the existing schema validation and digest checks unchanged, and ensure the assertion failure identifies the missing field.src/lib/inference/serving/generate-catalog.ts (1)
15-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument or narrow the empty registry default.
EMPTY_REGISTRIESmakes the build path reject every recipe, becausevalidateRecipeSemanticsrequires eachmaterializerRefandlifecycleRefto exist in the registry. The build passes only whilemanaged-inference/recipes/andmanaged-inference/presets/are empty. The first added definition breaksnpm run build:cli.Add a comment that states this default is valid only for the empty-catalog state, and link the issue that tracks the real registry source.
📝 Proposed comment
+// The catalog contains no definitions yet, so no adapter or readiness IDs are +// registered. Callers that compile real recipes must inject populated registries. const EMPTY_REGISTRIES: ServingCatalogRegistries = { materializers: new Set(), lifecycles: new Set(), readiness: new Map(), };🤖 Prompt for 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. In `@src/lib/inference/serving/generate-catalog.ts` around lines 15 - 19, Add a concise comment directly above EMPTY_REGISTRIES stating that it is valid only when the catalog contains no recipes or presets, and include the issue-tracker link for implementing the real registry source. Do not alter the registry values or validation behavior.
🤖 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 `@src/lib/inference/serving/catalog.test.ts`:
- Around line 115-131: Remove the requireValidationFailure helper and replace
both call sites in the serving catalog tests with the test framework’s
expect(...).toThrow(substring) assertion, preserving each existing validation
invocation and expected message.
In `@test/package-contract/managed-inference-catalog.test.ts`:
- Around line 41-42: Update the packlist assertions in the managed-inference
catalog contract test to also require
managed-inference/schemas/recipe.schema.json and
managed-inference/schemas/preset.schema.json, while preserving the existing
catalog and catalog.schema.json checks.
---
Nitpick comments:
In `@src/lib/inference/serving/generate-catalog.ts`:
- Around line 15-19: Add a concise comment directly above EMPTY_REGISTRIES
stating that it is valid only when the catalog contains no recipes or presets,
and include the issue-tracker link for implementing the real registry source. Do
not alter the registry values or validation behavior.
In `@test/package-contract/managed-inference-catalog.test.ts`:
- Around line 20-30: Update the test around parseCompiledServingCatalogJson to
explicitly assert that the compiled catalog contains a source revision, using
the parsed catalog result and its source-revision field. Keep the existing
schema validation and digest checks unchanged, and ensure the assertion failure
identifies the missing field.
🪄 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: CHILL
Plan: Enterprise
Run ID: b88f7954-0907-4f23-8465-2583372ed81c
📒 Files selected for processing (9)
managed-inference/schemas/catalog.schema.jsonmanaged-inference/schemas/preset.schema.jsonmanaged-inference/schemas/recipe.schema.jsonpackage.jsonsrc/lib/inference/serving/catalog.test.tssrc/lib/inference/serving/catalog.tssrc/lib/inference/serving/generate-catalog.tssrc/lib/inference/serving/types.tstest/package-contract/managed-inference-catalog.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
4 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 4 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Exact-head CI is implementation-green, but the required reviewed npm audit is blocked by |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed the exact current revision. The compiler accepts only repository-scoped recipe and preset paths, parses YAML with strict duplicate-key and alias rejection, applies closed JSON schemas, rejects executable fields and mutable image references, binds adapter and readiness references to injected registries, detects duplicate definitions and ambiguous automatic selectors, canonicalizes output, and verifies the compiled payload digest. The build and package contracts include the generated catalog and all three schemas. All CLI shards, build/typecheck, CodeQL, platform checks, image builds, and focused compiler/package tests pass; the reviewed-npm-audit failure is from an unrelated repository dependency graph. No blocking correctness, security, compatibility, or regression defect found.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — re-reviewed exact head dc31cca. The follow-up narrows immutable model revisions to exact 40-hex, 64-hex, or sha256 forms; rejects noncanonical and duplicate model paths, duplicate structured arguments and readiness requirements; and verifies readiness registry kinds. The focused negative coverage matches those contracts, and completed exact-head growth, DCO, docs, CLI-parity, macOS, installer, plugin, dependency-audit, and ARM image checks are green. Remaining broad checks are still running with no current attributable failure. No blocking correctness, security, compatibility, or regression defect found.
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed exact head dc31cca. The post-approval delta tightens model revision identity to exact immutable 40-hex, 64-hex, or sha256 forms; restricts model-file paths to canonical relative POSIX segments; rejects duplicate structured arguments, model files, and readiness requirements; and verifies readiness kinds against the injected registry. The new negative coverage exercises each boundary. Focused catalog tests pass 10/10 and the CLI build passes. Current CI reports no failures, with several broader jobs still running. No blocking correctness, security, regression, compatibility, test, or intended-behavior defect found.
<!-- markdownlint-disable MD041 --> ## Summary Extend the managed inference catalog with a typed, declarative llama.cpp recipe contract and recipe-linked readiness validation. This stacked change targets the catalog compiler in #8152 and remains inert until #8173 supplies approved production artifacts and adapter identities. ## Related Issue Fixes #8181 Part of #8144. Follow-up production configuration remains in #8173. ## Changes - Add a discriminated `install-llama-cpp` recipe schema for the catalog compiler, covering immutable server and GGUF identity, Docker/CUDA/GPU boundaries, bounded resources and serving limits, offline policy, disabled surfaces, readiness probes, and explicit capabilities. A direct production recipe is not appropriate in this slice because its artifact tuple and runtime adapters are not yet approved; synthetic fixtures protect the contract without activating serving. - Add typed readiness comparisons and injected observation roles for the accepted Linux, architecture, Docker, GPU-count, and driver matrix. Compiler tests prove that a selecting preset cannot contradict its linked recipe or bypass agent qualification readiness. - Add receipt and readiness-contract registries alongside existing materializer and lifecycle registries, and reject unknown references for both generic and llama.cpp recipes. - Reserve the llama.cpp backend/provider/server discriminators, require explicit-only selection, and reject mutable artifacts, executable syntax, host paths, unsupported capabilities, incomplete limits, and contradictory declarations. - Keep production registries and catalog definitions empty. Deterministic compiler, package-contract, and negative tests cover the new contract. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This is an inert schema/compiler contract with synthetic fixtures only; it adds no production recipe, provider selection, lifecycle action, default, or support claim. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Codex Desktop completed the repository's nine-category security review at exact head `cc298cdfbcbe973ff7dc36bdd6e024a99a12e3b9`; all categories passed with no blocking findings. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: The exact committed diff adds an inert schema/compiler contract with synthetic fixtures and no production recipe, provider selection, lifecycle action, default, or support claim. - Agent: Codex Desktop <!-- docs-review-head-sha: cc298cd --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run --project cli src/lib/inference/serving/catalog.test.ts` (54 passed); package-contract catalog tests (2 passed); `npm run test:changed` (54 passed); the changed-test conditionals guard passes. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable; the focused compiler tests, package contract, `npm run build:cli`, and repository hooks cover this bounded catalog-contract change. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added richer serving recipe definitions, including provider metadata, runtime requirements, model artifacts, serving limits, readiness probes, policies, surfaces, and capabilities. * Added specialized llama.cpp recipe validation with platform, GPU, runtime, model, and agent qualification checks. * Added observation-based readiness requirements, including equality, membership, numeric minimum, and version minimum comparisons. * Added validation for receipts, readiness contracts, artifacts, revisions, versions, and image digests. * **Improvements** * Updated the catalog compiler version to 1.1.0. * Strengthened validation for readiness requirements, duplicate declarations, and runtime compatibility. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
2 participants
Summary
Adds a strict compiler that turns repository-owned managed-inference YAML recipes and presets into canonical, digest-verified JSON shipped with the CLI. This establishes the accepted RFC #7636 trust boundary before llama.cpp-specific configuration and does not change provider selection or product support.
Related Issue
Fixes #8151
Changes
managed-inference/schemas/.dist/managed-inference/catalog.jsonduring the CLI build and include source schemas in the npm package. The package-contract test verifies the build artifact and real npm packlist.Type of Change
Quality Gates
dc31cca6faa045eac994915c1b7c7ad493141a34.Documentation Writer Review
no-docs-needed5f87afa53to headdc31cca6f. The model-revision, model-file-path, readiness-requirement, and trust-boundary terminology matches confirmed RFC RFC: Managed inference serving specifications #7636 and repository vocabulary. Test titles are behavior-oriented, and validation errors identify the rejected condition. The catalog still has no production definitions or runtime consumer.git diff --checkpassed; compiler tests passed 10/10;npm run build:clipassed; package-contract tests passed 2/2.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passednpm run docsbuilds without warnings (doc changes only)Security Review
No findings at exact head
dc31cca6faa045eac994915c1b7c7ad493141a34.All nine categories pass. The compiler remains a trusted, inert build-time boundary with empty production registries. Exact model revisions, portable canonical model paths with normalized uniqueness, duplicate canonical readiness rejection, immutable artifacts, injected registry identities, deterministic serialization, and catalog digest verification were independently exercised. Adversarial checks included reordered duplicate readiness objects, noncanonical filesystem aliases, YAML aliases, mutable references, dangling identities, and catalog tampering.
Signed-off-by: Aaron Erickson aerickson@nvidia.com