Skip to content

fix(install): restore the engine's pinned deps after an SDK install - #264

Open
volen-silo wants to merge 1 commit into
mainfrom
fix/sdk-install-preserves-engine-pinned-deps
Open

fix(install): restore the engine's pinned deps after an SDK install#264
volen-silo wants to merge 1 commit into
mainfrom
fix/sdk-install-preserves-engine-pinned-deps

Conversation

@volen-silo

Copy link
Copy Markdown
Collaborator

Summary

rocm install sdk could leave the managed runtime holding a torch build the installed vLLM engine does not accept, while every status surface still reported the runtime ready. The first symptom a user gets is a serve failure whose traceback names neither torch nor the SDK.

Root cause. The runtime venv has two owners. install sdk writes TheRock's torch stack into it, and the vLLM engine installs into the same interpreter, pinning a torch build from its own index. A second install sdk writes the SDK's torch back over that pin. The engine auto-install that runs immediately afterwards should have restored it, but its short-circuit tested whether vLLM resolved rather than whether vLLM's requirements were met — and since only torch had moved, vLLM still resolved, so the pin was never re-asserted.

The two torch builds cannot be reconciled by picking differently on the SDK side: the engine pins an exact local version (+git<sha>) that no TheRock build satisfies under PEP 440. The engine deliberately wins; the bug is that the SDK silently un-wins it.

What changed

  • A dependency-consistency primitive in rocm-core over uv pip check: report the unsatisfied Requires-Dist entries in an environment, filtered to the distribution that declares them.
  • The vLLM engine's install short-circuit is now gated on its own requirements being met, so a violated pin falls through to an install that restores it. This makes rocm engines install vllm self-heal too, and leaves reinstall: false meaning "don't redownload a healthy engine".
  • install sdk states the outcome as dependency_check: satisfied | violated | not_verified and records it in the audit log — on every auto-install outcome, including a failed one, which is where a violated pin is most likely.

Non-obvious decisions

  • The check runs with --color never. uv colourises its findings under FORCE_COLOR/CLICOLOR_FORCE even when stderr is a pipe. The escape prefix pushes every finding past the parser, which reads as "could not verify" and silently restores the old behaviour — i.e. the fix would quietly do nothing on such a host. There is a regression test for the colourised line specifically.
  • The parser requires the full frame. uv pip check reports several other conditions under the same The package \x`opening — a missingWHEEL/METADATAfile, an unsatisfiedRequires-Python, multiple installed distributions. None is a Requires-Dist` that reinstalling the requirer can clear, so accepting them would force a futile multi-gigabyte reinstall and print a remedy that cannot help.
  • The repair targets the interpreter that was assessed. The two resolvers disagree — one walks the candidates until it finds the one that actually has vLLM, the other takes the first unconditionally — and runtime ids match by prefix, so several candidates routinely qualify. Repairing a different interpreter than the one found broken would leave the broken one broken and report it fixed.
  • install sdk stays exit-0 when a violation survives. The SDK itself installed correctly, and abandoning a multi-gigabyte install costs the user more than the warning, which names the one command that clears it. Say so if you'd rather it were fatal.
  • Only the engine's own violations are reported. These environments carry chronic unrelated conflicts between third-party packages; surfacing them all would bury the one finding the user can act on.

Scope boundaries

Deliberately not included: teaching examine / runtimes list / engines list to stop reporting a runtime with a violated engine pin as ready. That is tracked separately; the primitive added here is what that work will build on.

Also known and not addressed here: the report is wired into the SDK auto-install path only, so a host where vLLM is not the auto-preferred engine but was installed by hand still gets the overwrite without a repair or a report. Happy to fold that in if reviewers would rather have it in one change.

Test plan

  • Unit coverage for the parser (real multi-line uv pip check bodies, the clean body, a colourised line, the four other diagnostic shapes that share the opening frame), the repair predicate, the assessed-interpreter selection, and each rendered dependency_check outcome.
  • New Gherkin scenario @id:runtime-sdk-reinstall-keeps-engine-consistent covering a second install sdk on a runtime that already has the engine. Tagged @requires-gpu @requires-engine:vllm @nightly: it needs a real SDK install, a real engine install and a second SDK install, so it cannot run in default CI — it runs on the nightly GPU lane. vLLM is gated because only it shares the runtime environment; Lemonade manages its own.
  • cargo test --workspace, cargo clippy --workspace --all-targets, cargo fmt, prek all clean locally.
  • The uv behaviours the change depends on were probed directly against uv 0.9.30 rather than assumed: the stdout/stderr split and exit codes, all five line shapes, that --color never defeats FORCE_COLOR, and that a plain uv pip install <pkg>==<pin> does restore a dependency that was downgraded underneath it (uv's satisfaction fast-path is recursive over the closure, so the repair is not a no-op).

Not verified here: the original reproduction needs a gfx94X host. The install sdkinstall sdk → compare-against-Requires-Dist sequence, the follow-up serve, and the new scenario all still need a run on that hardware.

Risk

Medium. On a healthy host the short-circuit behaves exactly as before, so install sdk is unchanged; only an already-violated environment takes the new path. The real exposure is that a violated pin now always triggers a uv resolve where previously nothing ran — on a host where that resolve fails, a repeat install sdk that used to be quiet will now surface the engine-install failure. That failure is reported rather than swallowed, which is the intended direction, but it is a visible behaviour change.

The managed runtime venv has two owners. `rocm install sdk` writes
TheRock's torch stack into it, and the vLLM engine installs into the
same interpreter, pinning a torch build from its own index that no
TheRock build satisfies. A second `install sdk` therefore wrote the SDK's
torch back over the engine's pin.

The auto-install that runs afterwards should have restored it, but its
short-circuit tested whether vLLM resolved rather than whether vLLM's
requirements were met. Only torch had moved, so vLLM still resolved and
the pin was never re-asserted. Nothing downstream noticed: runtime
validation checks paths and the rocm_sdk probe, so every surface kept
reporting the runtime ready, and the first symptom was a serve failure
whose traceback named neither torch nor the SDK.

Gate the short-circuit on the engine's own `Requires-Dist` instead, via a
`uv pip check` primitive in rocm-core, so a violated pin falls through to
an install that restores it. The repair targets the interpreter that was
assessed, which is not always the one the other resolver would pick. The
check runs with colour disabled because uv colourises its findings under
FORCE_COLOR even into a pipe, and an escape prefix would read as "could
not verify" and silently restore the old behaviour.

`install sdk` now states the outcome as `dependency_check:` and records
it in the audit log, on every auto-install outcome including a failed
one, where a violated pin is most likely. It stays exit-0 when a
violation survives: the SDK installed correctly, and the warning names
the one command that clears it.

Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
@volen-silo
volen-silo requested a review from a team as a code owner August 14, 2026 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant