fix(inference): restore loopback port publishing in the llama.cpp qualification runner - #8676
fix(inference): restore loopback port publishing in the llama.cpp qualification runner#8676AzeelSajjad wants to merge 6 commits into
Conversation
…#8667) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sh runner strings (NVIDIA#8667) Delete the differential --publish assertion in the agent-qualification argv test: both operands are already pinned to exact literals by adjacent toEqual calls, so it cannot fail independently and its stated mutation-catching rationale was wrong (only the hostPort: 18_081 row catches that mutation). Also collapse withQualificationLoopbackPublish's return to Biome's canonical single-line form (biome.json includes **/*.mts even though the pre-commit hook's regex misses it), document what an absent vs. present hostPort produces, and drop "ephemeral" from resolveLoopbackPort's error message since the agent-qualification path uses a deliberately fixed port. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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 (2)
📝 WalkthroughWalkthroughThe qualification runner now adds a loopback-only Docker publish mapping before the candidate image. It supports ephemeral, configured, and custom host ports. Tests verify argument ordering, mapping count, loopback binding, and YAML-bound request-guard behavior. ChangesQualification loopback publishing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Closing as superseded by #8672, which fixed #8667 with stricter image-boundary validation, Docker publish-alias rejection, explicit port validation, and broader regression coverage. I compared the remaining delta here: the only unique code is a minor resolveLoopbackPort() error-message wording cleanup plus an additional arbitrary-host-port test; neither changes required behavior or warrants keeping this conflicting PR open. Thank you for the careful investigation and implementation. |
2 participants
Summary
The DGX Spark llama.cpp qualification runner builds its server container arguments with no
--publishentry, sodocker portcannot resolve a loopback address and the runner cannot probe the server it just launched. This adds a qualification-only loopback mapping in the runner itself: an ephemeral127.0.0.1::8081by default, and the requested fixed127.0.0.1:8081:8081when the runner asks for a deterministic port. The shared host-local materializer and the Docker lifecycle provider are unchanged, so ordinary product launches still leave host bridging to the lifecycle provider.Related Issue
Fixes #8667
Changes
scripts/checks/run-llama-cpp-dgx-spark-qualification.mts: add module-privatewithQualificationLoopbackPublish(), which splices one--publish <mapping>pair into the argv returned by the shared request-guard builder, immediately before the image reference.buildServerContainerArgv()wraps the shared result and forwards its optionalhostPort.scripts/checks/run-llama-cpp-dgx-spark-qualification.mts: drop the wordephemeralfromresolveLoopbackPort()'s error message, which this change makes inaccurate on the fixed-port path.test/llama-cpp-dgx-spark-qualification-runner.test.ts: pin both mapping forms, the mapping's position ahead of the image reference, and that exactly one mapping is emitted.This adds no configuration, fallback, migration, or compatibility layer.
withQualificationLoopbackPublish()is one private helper with one current consumer,buildServerContainerArgv(), and the tests named under Verification protect its contract. ThehostPortoption it reads already existed onbuildServerContainerArgv()and is still forwarded into the sharedLlamaCppHostLocalRuntimeBindings, wherepositiveInteger(bindings.hostPort, "llama.cpp host port", 65_535)continues to bound it.Why the runner needs its own argument rather than a change to the shared materializer: #8615 deliberately removed publishing from the shared path because product launches delegate host bridging to the Docker lifecycle provider, which rejects a container that publishes ports (the
must not publishownership rollback from #8544). The qualification runner does not use that provider — it runsdocker rundirectly, thendocker port, then probes the resolved port. Publishing therefore belongs to the runner alone. Both emitted mapping strings are byte-identical to what the pre-#8615 materializer produced, and the runner already created its network with--internalat that time, so this restores a configuration this repository's CI is known to have run rather than introducing a new one.Type of Change
Quality Gates
.github/workflows/e2e.yamlbehindallow_dgx_spark_runner_queueor manual dispatch. It is not a CLI, configuration, default, or other supported product surface. A documentation writer subagent independently confirmed that nodocs/page references the script or its functions, and that everydocs/occurrence of "qualification" refers to host and platform hardware qualification or the protected qualification and activation gates, which are separate already-documented concepts.127.0.0.1, adds no egress, and leaves the credential path untouched — the API key is still delivered by bind-mounted file, and the test continues to assert the key never appears on argv.Documentation Writer Review
no-docs-neededdocs/forrun-llama-cpp-dgx-spark-qualification,withQualificationLoopbackPublish,resolveLoopbackPort, andqualification, finding no page that owns this script; confirmed from.github/workflows/e2e.yamlthat the runner is CI-internal; and reviewed the changed prose againstWRITING.mdand the controlled word list — the helper's doc comment,resolveLoopbackPort()'s amended error message, the test titles, and the commit subjects. No blocking findings.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 vitest run --project integration test/llama-cpp-dgx-spark-qualification-runner.test.ts— 12/12 pass. Without this change the suite fails withexpected [] to deeply equal [ '127.0.0.1::8081' ].npx vitest run --project cli src/lib/inference/llama-cpp/host-local-runtime.test.ts— 16/16 pass, confirming the shared materializer still publishes nothing, including whenhostPortis set.npx vitest run --project cli src/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.ts— 41/41 pass, confirming the lifecycle provider'smust not publishrollback is undisturbed.npm run checks:repository— all checks pass, includingtest-title-style.npm run validate:pr— exit 0, no autofixes.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not applicable. This change is confined to one CI-only check script and its test, and alters no runtime or test-harness behavior. For transparency: a localnpm testattempt on the development host produced 37 timeout failures out of 14,702 in thecliproject, allTest timed out in 5000msunder memory pressure on an 8 GB machine with cumulative setup time of 1,113 s. The affected files pass 53/53 in 13.66 s when run in isolation, and none of them import either changed file. Deferring the broad gate to CI.npm run docsbuilds without warnings (doc changes only)Notes for Reviewers
Two items found during this work that are outside the fix and are not addressed here:
prekbiome-formathook'sfilespattern ends in.*\.ts$, which does not match a.mtsextension, whilebiome.jsonincludes**/*.mts.npm run validate:prand theprek run --from-refinvocation in.github/workflows/pr.yamltherefore both pass over formatting in every.mtsfile underscripts/checks/andtools/e2e/. This branch's helper was left unformatted by that gap until it was caught by review and corrected in7af908ab4;npm run formatis now a no-op on the file. Happy to open a separate issue if maintainers agree it is worth fixing.resolveLoopbackPort()parsesdocker portoutput with/^127\.0\.0\.1:([1-9][0-9]{3,4})$/u, which accepts only 4-and-5-digit ports, whilepositiveInteger(contract.serve.port, ..., 65_535)permits any port from 1. A recipe declaring a shorter port would publish successfully and then fail to resolve. Not reachable today — the pinned recipe declaresport: 8081— and deliberately not changed here to keep this fix minimal.The fixed-port availability race called out in the issue's acceptance criteria is likewise not addressed here, as the issue directs. The runner's existing pre-flight
tcpOpen(plan.recipe.serve.port)refusal already narrows it to a TOCTOU window.Signed-off-by: Azeel Sajjad aasajjad05@gmail.com
Summary by CodeRabbit
Bug Fixes
Tests