feat(inference): add owned llama.cpp image build - #8235
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
added
area: inference
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a pinned, multi-platform CUDA llama.cpp server image with non-root runtime settings. Adds manifest validation, pull-request image builds, isolated runtime checks, and Vitest coverage for the image and workflow contracts. Changesllama.cpp CUDA image
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant ConfigJob as Config Job
participant Buildx
participant LlamaImage as Llama Image
PullRequest->>ConfigJob: trigger validation workflow
ConfigJob->>ConfigJob: load and validate image manifest
ConfigJob->>Buildx: provide matrix and build arguments
Buildx->>LlamaImage: build unpublished native image
ConfigJob->>LlamaImage: run metadata and restricted-runtime checks
LlamaImage-->>ConfigJob: return validation results
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 8ca6b8c in the TypeScript / code-coverage/cliThe overall coverage in commit 8ca6b8c in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
6 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
5 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. 2 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: 2 warnings · 0 suggestionsWarningsWarnings do not block.
|
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head b5e18c7937cb52ba7ed598b44c04efa3687032f6 against accepted epic #8144 and delivery issue #8231. The staged PR-only build is in scope: #8231 explicitly requires both native architectures to build/validate without registry writes before publication. The lack of a current owned-image consumer is therefore expected at this intermediate step and is not itself my blocker.
Changes requested for the following contract failures:
-
The final runtime image deliberately retains and invokes
/bin/shin.github/workflows/llama-cpp-image.yaml, but #8231's image contract explicitly says the runtime must not include shell tools. The validation currently proves the opposite of the accepted boundary. Remove the runtime shell/tool surface (or obtain and link an explicit maintainer decision changing that contract), and validate user/license/UI properties without executing a shell in the image—for example via image metadata and an exported filesystem inspection. -
The manifest compiler is described and tested as fail-closed, but
digestReferenceaccepts any digest-pinned path underdocker.ioorghcr.io, including non-NVIDIA CUDA bases, and the schema permits unexpected outer fields. #8231 requires NVIDIA CUDA bases and fail-closed handling of mutable/changed dependencies and descriptors. Tighten the exact allowlist/shape validation and add table-driven rejection coverage for unauthorized base registries, invalid runners, malformed digests, duplicate platforms, and unexpected fixed-contract fields. The primary advisor's PRA-2 identifies the same gap. -
Exact-head
codebase-growth-guardrailsfails becausetest/llama-cpp-image-workflow.test.tsadds two conditional statements. Keep the tests linear by moving setup validation into a named non-test helper or collecting action references and asserting them without conditional branches. This is deterministic and should not be retried unchanged.
Please retain the current no-publication permissions boundary while addressing these items. The native image jobs and CodeRabbit review are still running; any findings there remain additional exact-head evidence.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
test/llama-cpp-image.test.ts (1)
60-67: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
split("=", 2)discards the tail of a value that contains=.
String.prototype.splitwith a limit does not keep the remainder. It returns only the first two segments. Formatrix={"include":[...]}the current values contain no=, so the parse happens to be correct today. If any emitted value gains an=, this helper silently truncates it and thematrixassertion on line 136 fails as a JSON parse error rather than as a clear mismatch.Split once on the first delimiter.
♻️ Proposed fix
function parseOutput(value: string): Record<string, string> { return Object.fromEntries( value .trim() .split("\n") - .map((line) => line.split("=", 2) as [string, string]), + .map((line) => { + const separator = line.indexOf("="); + return [line.slice(0, separator), line.slice(separator + 1)] as [string, string]; + }), ); }🤖 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/llama-cpp-image.test.ts` around lines 60 - 67, Update parseOutput so each line is split only at the first "=" while preserving the entire remainder as the value. Keep the existing Record<string, string> output and Object.fromEntries behavior unchanged..github/workflows/llama-cpp-image.yaml (2)
50-57: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a
concurrencygroup for this long build.
pr-buildhastimeout-minutes: 120and builds two platforms from source with no cache. Without aconcurrencygroup, each new push to a pull request starts another pair of two-hour CUDA builds while the previous pair still runs. The queue on theubuntu-24.04-armrunner pool grows quickly.Cancel superseded runs at the workflow level.
♻️ Proposed change
permissions: contents: read +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + jobs:🤖 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 @.github/workflows/llama-cpp-image.yaml around lines 50 - 57, Add a workflow-level concurrency group for the pr-build job, keyed to the pull request or workflow context, and enable cancellation of in-progress runs so superseded pushes do not continue occupying runners. Preserve the existing matrix, timeout, and fail-fast settings.
67-87: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a build cache for the CUDA compile step.
This step compiles llama.cpp with CUDA for three device architectures on amd64. It declares no
cache-fromorcache-to, so every run rebuilds from scratch against a 120-minute timeout..github/workflows/managed-images.yamlline 179 uses a registry build cache for the same reason.A registry cache needs credentials, which this read-only workflow must not have. Use the GitHub Actions cache backend instead, which works with the ambient runtime token and keeps the no-publication boundary intact.
♻️ Proposed change
provenance: false sbom: false + cache-from: type=gha,scope=llama-cpp-${{ matrix.arch }} + cache-to: type=gha,mode=max,scope=llama-cpp-${{ matrix.arch }}Confirm the runtime measured for a cold build before you rely on
timeout-minutes: 120.🤖 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 @.github/workflows/llama-cpp-image.yaml around lines 67 - 87, Add GitHub Actions build-cache configuration to the “Build native PR image without publishing” step using docker/build-push-action’s gha cache backend, with a stable scope shared appropriately across runs. Keep the workflow read-only by avoiding registry credentials and preserve the existing load, push, provenance, and SBOM settings; also verify the cold-build duration remains within the 120-minute timeout.scripts/checks/export-llama-cpp-image-config.mts (1)
79-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider validating shape instead of mirroring every manifest value.
expectedCmake,expectedBuildPackages, andexpectedRuntimePackagesrestate the full contents ofimage.yamllines 32-54 and 61-64. The exporter then rejects the manifest unless it matches that copy exactly. This inverts the intended direction:image.yamlis the declared source of truth, but any change to it fails the exporter until an engineer edits this file too. The same values also appear a third time as literals inmanaged-inference/images/llama-cpp/Dockerfile.Keep the fail-closed checks that the manifest cannot express (
digestReference,fullRevision,sha256, the runner and platform pairing, the runtime ID range). For the cmake flags and package pins, validate structure and lettest/llama-cpp-image.test.tscontinue to assert manifest-to-Dockerfile agreement.♻️ Suggested direction for the package pins
- const expectedBuildPackages = { - "build-essential": "12.10ubuntu1", - "ca-certificates": "20260601~24.04.1", - cmake: "3.28.3-1build7", - curl: "8.5.0-2ubuntu10.11", - "libcurl4-openssl-dev": "8.5.0-2ubuntu10.11", - "libssl-dev": "3.0.13-0ubuntu3.12", - }; - const expectedRuntimePackages = { - "ca-certificates": "20260601~24.04.1", - libcurl4t64: "8.5.0-2ubuntu10.11", - libgomp1: "14.2.0-4ubuntu2~24.04.1", - }; + const pinnedPackages = (value: unknown): boolean => + typeof value === "object" && + value !== null && + Object.keys(value).length > 0 && + Object.entries(value as Record<string, unknown>).every( + ([name, version]) => + /^[a-z0-9][a-z0-9+.-]*$/u.test(name) && + typeof version === "string" && + /^[0-9][A-Za-z0-9.+:~-]*$/u.test(version), + );Then replace the two
matchesExactRecordcalls for packages withpinnedPackages(...)checks.Based on the path instruction for
scripts/checks/**: "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift."🤖 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 `@scripts/checks/export-llama-cpp-image-config.mts` around lines 79 - 121, Update the validation in the exporter around expectedCmake, expectedBuildPackages, and expectedRuntimePackages to stop mirroring manifest values. Retain fail-closed checks for digestReference, fullRevision, sha256, runner/platform pairing, and runtime ID limits; validate only the required cmake shape, and replace both exact package-record comparisons with pinnedPackages(...) checks. Remove duplicated package and flag inventories while preserving the existing llama.cpp contract checks and relying on the manifest/Dockerfile agreement test for exact values.Source: Path instructions
🤖 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 `@managed-inference/images/llama-cpp/Dockerfile`:
- Line 99: Update the license directory COPY instruction to use a traversable
permission mode, changing --chmod=0444 to --chmod=0555 so the non-root workflow
can access /usr/local/share/licenses/llama.cpp/ while preserving license
readability.
- Around line 59-62: Update the Dockerfile’s CMake configuration to set
GGML_BACKEND_DIR to /opt/llama.cpp/lib when building with GGML_BACKEND_DL=ON,
matching the destination used by the backend modules. Add validation after
packaging that confirms the CUDA backend module is present and loadable by
llama-server, failing the build if CUDA backend availability is not proven.
In `@test/llama-cpp-image-workflow.test.ts`:
- Around line 40-43: Update required<T> to use a nullish-coalescing fallback
that throws the existing error, avoiding an if statement and treating null as
absent. In the action-pin verification loop around step.uses, collect all
declared uses values first, assert the collection is non-empty, then verify
every entry matches fullShaAction without conditional assertions.
- Around line 68-69: Replace the ineffective JSON substring assertion for
"packages:write" with an assertion against the parsed workflow permissions
object, verifying that package publishing permission is absent or not granted.
Keep the existing "docker/login-action" assertion unchanged, and update only the
no-publication-path checks in the workflow test.
In `@test/llama-cpp-image.test.ts`:
- Around line 15-20: Update the paths filter in the llama-cpp-image workflow to
include
managed-inference/recipes/llama-cpp.nemotron-3-nano-30b-a3b.spark-single.v1.yaml,
matching the recipePath used by the llama-cpp image test so recipe changes
trigger the workflow.
---
Nitpick comments:
In @.github/workflows/llama-cpp-image.yaml:
- Around line 50-57: Add a workflow-level concurrency group for the pr-build
job, keyed to the pull request or workflow context, and enable cancellation of
in-progress runs so superseded pushes do not continue occupying runners.
Preserve the existing matrix, timeout, and fail-fast settings.
- Around line 67-87: Add GitHub Actions build-cache configuration to the “Build
native PR image without publishing” step using docker/build-push-action’s gha
cache backend, with a stable scope shared appropriately across runs. Keep the
workflow read-only by avoiding registry credentials and preserve the existing
load, push, provenance, and SBOM settings; also verify the cold-build duration
remains within the 120-minute timeout.
In `@scripts/checks/export-llama-cpp-image-config.mts`:
- Around line 79-121: Update the validation in the exporter around
expectedCmake, expectedBuildPackages, and expectedRuntimePackages to stop
mirroring manifest values. Retain fail-closed checks for digestReference,
fullRevision, sha256, runner/platform pairing, and runtime ID limits; validate
only the required cmake shape, and replace both exact package-record comparisons
with pinnedPackages(...) checks. Remove duplicated package and flag inventories
while preserving the existing llama.cpp contract checks and relying on the
manifest/Dockerfile agreement test for exact values.
In `@test/llama-cpp-image.test.ts`:
- Around line 60-67: Update parseOutput so each line is split only at the first
"=" while preserving the entire remainder as the value. Keep the existing
Record<string, string> output and Object.fromEntries behavior unchanged.
🪄 Autofix
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: d8f2d6fb-c4a6-40a2-b7a6-a7605d800f50
📒 Files selected for processing (6)
.github/workflows/llama-cpp-image.yamlmanaged-inference/images/llama-cpp/Dockerfilemanaged-inference/images/llama-cpp/image.yamlscripts/checks/export-llama-cpp-image-config.mtstest/llama-cpp-image-workflow.test.tstest/llama-cpp-image.test.ts
| function required<T>(value: T | undefined, message: string): T { | ||
| if (value === undefined) throw new Error(message); | ||
| return value; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the two if statements to clear the growth guardrail, and make the action-pin loop non-vacuous.
CI fails on this file: "this changed test file adds 2 if statements, up from 0 at the base. Keep test bodies linear." The two statements are line 41 and line 112.
Line 112 is also a vacuous assertion. if (step.uses) expect(...) asserts nothing when no step declares uses. The test then passes while proving no pin at all. Collect the uses values first, assert the collection is not empty, then assert each entry matches fullShaAction.
Line 41 can express the same guard without a statement.
🐛 Proposed fix
function required<T>(value: T | undefined, message: string): T {
- if (value === undefined) throw new Error(message);
- return value;
+ return (
+ value ??
+ (() => {
+ throw new Error(message);
+ })()
+ );
} it("pins actions and validates the native non-root read-only image (`#8231`)", () => {
- for (const job of Object.values(workflow.jobs ?? {})) {
- for (const step of job.steps ?? []) {
- if (step.uses) expect(step.uses).toMatch(fullShaAction);
- }
- }
+ const actionReferences = Object.values(workflow.jobs ?? {})
+ .flatMap((job) => job.steps ?? [])
+ .map((step) => step.uses)
+ .filter((uses): uses is string => uses !== undefined);
+ expect(actionReferences.length).toBeGreaterThan(0);
+ expect(actionReferences.filter((uses) => !fullShaAction.test(uses))).toEqual([]);Note that ?? treats null as absent while === undefined does not. YAML.parse produces null for an empty key, so the ?? form is the safer guard for this input.
Also applies to: 110-114
🤖 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/llama-cpp-image-workflow.test.ts` around lines 40 - 43, Update
required<T> to use a nullish-coalescing fallback that throws the existing error,
avoiding an if statement and treating null as absent. In the action-pin
verification loop around step.uses, collect all declared uses values first,
assert the collection is non-empty, then verify every entry matches
fullShaAction without conditional assertions.
Sources: Path instructions, Pipeline failures
|
Addressed the exact-head review findings in
Focused local evidence: 2 files / 13 tests passed; CLI typecheck, repository checks, test-title/size/conditional guards, Biome, Prettier, Hadolint, |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/llama-cpp-image-workflow.test.ts`:
- Around line 74-78: Update the permission validation in the workflow test to
handle both scalar and map forms for workflow and job permissions without
relying on Object.values for strings. Ensure scalar job permissions such as
write-all are detected and rejected, and add a job-level permissions: write-all
fixture to verify this case.
In `@test/llama-cpp-image.test.ts`:
- Around line 186-188: Update the duplicate-platform test case to change the
runner alongside the platform replacement, using a runner compatible with
linux/amd64 so validation reaches the duplicate-platform uniqueness check. Keep
the test focused on exercising duplicate-platform rejection rather than runner
validation.
🪄 Autofix
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: 8209efef-38ad-438e-bf57-2c03b0f90a52
📒 Files selected for processing (6)
.github/workflows/llama-cpp-image.yamlmanaged-inference/images/llama-cpp/Dockerfilemanaged-inference/images/llama-cpp/image.yamlscripts/checks/export-llama-cpp-image-config.mtstest/llama-cpp-image-workflow.test.tstest/llama-cpp-image.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- managed-inference/images/llama-cpp/image.yaml
- .github/workflows/llama-cpp-image.yaml
- managed-inference/images/llama-cpp/Dockerfile
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head 09e94660ed48f563c7349f69a67adb2593f4def2. The follow-up resolves my original blockers: the final image removes the shell paths, the workflow validates via exported filesystem/image metadata rather than invoking a shell, the compiler now enforces exact manifest shapes and NVIDIA-owned digest-pinned bases, the rejection matrix covers the requested cases, and the guardrail is green.
Two current fail-closed test gaps remain, matching CodeRabbit's exact-head findings:
-
test/llama-cpp-image-workflow.test.tstypes permissions only as maps and flattens them withObject.values. GitHub Actions also accepts scalar permission forms such aswrite-all;Object.values("write-all")yields characters, so this no-publication assertion would not detect a job-level scalar grant. Normalize/reject both scalar and map forms and add a mutated fixture provingpermissions: write-allfails. -
The duplicate-platform mutation changes
linux/arm64tolinux/amd64but leaves the ARM runner. Validation therefore rejects the runner mismatch before reaching the uniqueness check, so the test does not prove duplicate platforms are rejected. Change the runner in that candidate as well and assert the duplicate-platform error path.
Please retain the current read-only/no-publication boundary and native amd64/arm64 validation while fixing these two tests. The unrelated CLI shard failure and native image jobs are separate exact-head receipts and do not waive these deterministic gaps.
|
Exact-head remediation update for
This remains a read-only PR build: no publication, registry write, support activation, or default-provider change. |
2 participants
Summary
NemoClaw has no repository-owned llama.cpp image build. This PR adds a declarative build manifest and pull-request checks for native amd64 and DGX Spark arm64 images. The checks do not publish an image or change the managed inference support state.
Related Issue
Part of #8231.
Changes
ServerImageBuildYAML manifest for the image registry, pinned llama.cpp source archive, NVIDIA CUDA bases, package versions, native platform matrix, CMake settings, and runtime identity. The PR workflow consumes this manifest because the accepted managed inference design requires declarative configuration; focused contract tests reject drift between the manifest, serving recipe, Dockerfile, and workflow.llama-serverDockerfile. It verifies the pinned source archive, builds the CUDA server without UI, RPC, or subprocess surfaces, preserves upstream license files, and creates a non-root runtime image. The runtime removes command shells, and PR validation inspects its exported filesystem without executing a shell.linux/amd64andlinux/arm64builds. It has no package-write permission or registry login. Each lane checks the exact source and base labels, server revision, runtime user, license files, disabled UI assets, and read-only root filesystem behavior.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx --no-install vitest run test/llama-cpp-image.test.ts test/llama-cpp-image-workflow.test.ts— 2 files and 13 tests passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
New Features
Tests