Skip to content

fix(comfyui): pin managed torch stack during install (EAI-8051) - #298

Merged
fredespi merged 3 commits into
mainfrom
rocm-comfyui-install-replaces-managed-rocm
Aug 25, 2026
Merged

fredespi merged 3 commits into
mainfrom
rocm-comfyui-install-replaces-managed-rocm

Conversation

@fredespi

Copy link
Copy Markdown
Collaborator

Summary

rocm comfyui install installs ComfyUI's dependencies into the machine's managed ROCm runtime. It already filters torch/torchvision/torchaudio out of ComfyUI's direct requirement specs, but nothing scopes package resolution — so a transitive ComfyUI dependency can still resolve torch from PyPI, pull a CUDA build plus nvidia-* wheels, and displace the runtime's ROCm torch. The runtime the whole machine serves models with then becomes a CUDA build with no AMD GPU support: installing an optional app breaks the base runtime (and vLLM serving with it).

This pins the managed runtime's existing torch stack to its exact installed versions so the resolver can never swap in a CUDA build.

Changes

  • Before running uv pip install for ComfyUI's deps, probe the managed runtime's currently-installed torch/torchvision/torchaudio versions via importlib.metadata (no native import needed).
  • Write those as a uv --constraint file and pass it to the install. The resolver treats the stack as already satisfied and never replaces it; because ROCm torch has no nvidia-* dependencies, none are pulled in.
  • A genuine version conflict (a dep needing an incompatible torch) now fails the install loudly instead of silently corrupting the runtime.
  • Added unit tests for the constraint-line generation and the --constraint argument wiring.

Test plan

ComfyUI dependency install runs `uv pip install` into the machine's
managed ROCm runtime. Direct torch/torchvision/torchaudio specs are
filtered out, but nothing scopes resolution, so a transitive dependency
still resolves torch from PyPI and drags in a CUDA build plus nvidia-*
wheels, displacing the runtime's ROCm torch and leaving a runtime with
no AMD GPU support.

Probe the runtime's installed torch-stack versions via importlib
metadata (no native import) and pass a uv --constraint file pinning them
to those exact versions. The resolver treats them as satisfied and never
swaps in a CUDA build; ROCm torch has no nvidia-* deps, so none appear.
A genuine version conflict now fails the install loudly instead of
silently corrupting the runtime.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
@fredespi
fredespi requested a review from a team as a code owner August 22, 2026 12:09
@fredespi
fredespi requested a review from juhovainio August 22, 2026 12:09

@r0x0r r0x0r left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — the approach is sound. Pinning the runtime's torch stack to its exact installed versions (local +git… identifiers and all) via a uv --constraint file is the right lever: a transitive dep that wants a compatible torch is satisfied by what's already there (no reinstall), and a genuinely incompatible one fails loudly instead of silently swapping in a CUDA build. I checked the TORCH_STACK_VERSION_PROBE string — the Rust \-line-continuations strip the source indentation and the \x20 escapes re-supply it, so the emitted Python has valid, consistent block indentation. The pure helpers are unit-tested, and the end-to-end CUDA-rejection proof is the @requires-gpu @nightly comfyui-install-preserves-the-rocm-runtime scenario in #291 — a gated lane, named per AGENTS §3.

One substantive thread on the scope of the guarantee below.

Comment thread apps/rocm/src/comfyui.rs Resolved
Address review: make explicit why constraining only the installed
torch-stack packages is safe. A managed runtime installs
torch/torchvision/torchaudio atomically and refuses unless a mutually
compatible set is found, and ComfyUI install only targets such a
runtime — so the "torch present, torchvision/torchaudio absent" case in
which an unpinned CUDA torchvision could slip in alongside the pinned
ROCm torch cannot occur. Comment-only; no behavior change.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
…note (EAI-8051)

Review nit: the resolver is `select_matching_pip_package_versions`, not
`select_compatible_stack`. Comment-only.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

@r0x0r r0x0r left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The transitive-torch defect (EAI-8051) is fixed at the right layer: the managed runtime's exact torch/torchvision/torchaudio versions are pinned via a uv --constraint file, so the resolver treats the ROCm stack as satisfied and can never swap in a CUDA build, while a genuine conflict fails loudly instead of silently corrupting the runtime. The \x20-escaped Python probe reconstructs to valid indentation, and the pure helpers are unit-tested. My one thread -- whether "constrain only what's installed" leaves a partial-stack gap -- is resolved: the follow-up commits document (and I verified) that therock installs the full trio atomically as a mutually-compatible set (select_matching_pip_package_versions bails otherwise) and ComfyUI's select_runtime only ever targets a managed wheel runtime, so the partial-stack case cannot arise. End-to-end CUDA-rejection proof is the @requires-gpu @nightly scenario in #291, named per AGENTS. 24/24 CI green.

@fredespi
fredespi added this pull request to the merge queue Aug 25, 2026
Merged via the queue into main with commit 3e59435 Aug 25, 2026
24 checks passed
@fredespi
fredespi deleted the rocm-comfyui-install-replaces-managed-rocm branch August 25, 2026 12:01
fredespi added a commit that referenced this pull request Aug 25, 2026
The ComfyUI fix landed in 3e59435 (#298), pinning the managed runtime's torch stack through uv constraints. The scenario now asserts behavior main implements; remove its xfail row so a pass is not a fatal stale XPASS. Keep the scenario as the regression guard.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
fredespi added a commit that referenced this pull request Sep 10, 2026
main's #305 (f2c3477) already fixed EAI-8053 — the driver install no
longer prepends `sudo` when it is already running as root — and shipped
unit tests over the install plan covering both the root and non-root
cases. The e2e scenario this branch carried is therefore redundant, and
no CI lane runs as uid 0, so it could never have executed anyway.
Remove the scenario, its `@requires-root` tag, the `is_root` capability
probe, the `mock-root` fixture and the expectations row.

What remains is the EAI-8051 ComfyUI contract:

- Register `comfyui.feature` in `FEATURE_KEYS` and renumber its scenario
  to `comfyui-01`, per the per-feature naming guard main added.
- Require the install to exit 0 before asserting runtime health. Since
  #298 it does; without that premise a bail-out (no managed runtime, a
  failed download) would leave the runtime trivially unchanged and the
  scenario would report green having installed nothing.
- Correct the now-stale comments claiming the install exits non-zero,
  and describe `find_venv_python` accurately: the documented layout is
  the first frontier entry, so the depth-capped walk is a fallback, not
  a cost the normal path pays.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
fredespi pushed a commit that referenced this pull request Oct 7, 2026
Past-tense the #298 comment, document that the rc check catches a #298
revert, rename the torch step to 'unchanged', surface probe errors, assert
a single runtime root, and fix the walk doc.

Signed-off-by: fredespi-agent-app[bot] <266462846+fredespi-agent-app[bot]@users.noreply.github.com>
fredespi pushed a commit that referenced this pull request Oct 7, 2026
Past-tense the #298 comment, document that the rc check catches a #298
revert, rename the torch step to 'unchanged', surface probe errors, assert
a single runtime root, and fix the walk doc.

Signed-off-by: fredespi-agent-app[bot] <266462846+fredespi-agent-app[bot]@users.noreply.github.com>
fredespi pushed a commit that referenced this pull request Oct 7, 2026
Past-tense the #298 comment, document that the rc check catches a #298
revert, rename the torch step to 'unchanged', surface probe errors, assert
a single runtime root, and fix the walk doc.

Signed-off-by: fredespi-agent-app[bot] <266462846+fredespi-agent-app[bot]@users.noreply.github.com>
Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
mikeroySoft pushed a commit to mikeroySoft/rocm-cli that referenced this pull request Nov 6, 2026
…OCm#291)

* test(e2e): pin driver-install-as-root and comfyui runtime contracts (EAI-8071)

Two test-only scenarios pinning walkthrough defects, each registered as an
xfail in expectations.toml citing its ticket so CI stays green and the row
goes stale the day the bug is fixed.

EAI-8053: `install driver --dkms` prefixes every plan command with a literal
`sudo`, so as root on a host without sudo the first command dies with
`sudo: not found`. New driver.feature runs the install with a PATH that omits
sudo and asserts the run does not fail merely because sudo is absent. Adds a
`@requires-root` tag (skips off root, where the prefix is correct) so the row
stays honest on non-root lanes.

EAI-8051: `comfyui install` installs ComfyUI's deps into the managed ROCm
runtime with no index scoping, so a transitive dep can pull CUDA nvidia-*
wheels in and displace the runtime's ROCm torch. New comfyui.feature installs
an isolated runtime, installs ComfyUI, and asserts the runtime's torch is
still a ROCm build with no nvidia-* distributions. GPU/lifecycle/nightly and
isolated, since it may corrupt the runtime it runs against.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): fix EAI-8051 comfyui scenario gating and package probe

Address pre-PR review of the ComfyUI runtime-preservation scenario:

- Drop @lifecycle from the scenario. No lane sets E2E_INCLUDE_LIFECYCLE on a
  GPU host, so @requires-gpu @lifecycle @nightly together made the scenario
  unreachable on every lane. @requires-gpu @nightly matches
  runtime-install-sdk-active, the other scenario that does a real install sdk;
  this scenario mutates only its own isolated runtime prefix, not the OS.
- Enumerate installed distributions via importlib.metadata instead of
  `python -m pip list`. uv-created managed runtimes have no pip module, so the
  pip probe exited non-zero with empty stdout, which the old code read as "no
  nvidia packages" and passed the check on a runtime it never inspected. The
  probe now fails loudly on any error rather than treating it as a clean result.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): fix clippy on the e2e test targets

CI's clippy lane checks the e2e harness test target with -D warnings, which
surfaced two lints the mock-lane gate did not:

- comfyui_steps.rs: map(str::trim).unwrap_or_else(panic) -> map_or_else.
- e2e.rs: split run_rocm_with_only_tools' overlong first doc paragraph.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): make run_rocm_with_only_tools compile on Windows

The helper and its which_on_path were #[cfg(unix)], but driver_steps.rs calls
them unconditionally — cucumber step functions are registered on every platform,
so the Windows build failed with E0425 (cannot find function). The driver
scenario is @requires-os:linux and never runs on Windows, but the step code must
still compile there. Drop the cfg gates and gate only the symlink line (Unix
symlink; a plain copy on other platforms), so the call resolves everywhere.

Verified: Linux clippy --all-targets -D warnings and a Windows cross-check
(x86_64-pc-windows-gnu) both pass.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): find the runtime via runtimes list, not examine Folder:

The EAI-8051 scenario located the managed runtime's venv by parsing `Folder:`
from `rocm examine`, but examine only prints that line for the ACTIVE runtime and
takes a different branch otherwise. On the MI300X GPU lane the baseline step
panicked with "no active-runtime Folder: line" — a miswired step, reported as an
unexpected failure rather than the intended xfail. The isolated `install sdk`
itself succeeded. Read `install_root:` from `rocm runtimes list`, which is printed
unconditionally for every installed runtime, so discovery no longer depends on
which runtime examine considers active.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): judge torch build from dist metadata, not a live import

The EAI-8051 baseline step imported torch in the runtime venv to read
torch.version.hip, but importing torch loads the ROCm shared libraries, which
need the runtime's LD_LIBRARY_PATH/ROCM_PATH — the product sets that env when it
probes torch, our bare interpreter does not. On the MI300X lane the import failed
and a runtime with a perfectly good ROCm torch read as "not a ROCm build".

Judge the build from the torch distribution's version label via
importlib.metadata (`2.x+rocm...` vs `2.x+cu...`) instead — no import, no native
load, so a bare interpreter suffices. The baseline and post-install assertions now
also surface the actual version string for self-diagnosis on failure.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): judge ROCm torch as "not a CUDA build", grounded on the runner

The GPU lane showed TheRock's managed torch labels its local version with a git
hash (2.11.0+gitd0c8b1f), not +rocm, so requiring "rocm" in the label wrongly
failed the baseline on a healthy ROCm runtime. A CUDA wheel, by contrast, always
carries +cu. The EAI-8051 corruption replaces the ROCm/TheRock torch with a CUDA
one, so judge "still ROCm" as "torch present and NOT a +cu build" — which is
exactly the flip the defect causes and needs no positive ROCm label.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): require the runtime's torch version to be unchanged (EAI-8051)

Review feedback (tomastola): "torch is still a ROCm build" only checked the
post-install version was not a `+cu` CUDA build, which the MI300X run showed is
too weak — ComfyUI replaced torch==2.11.0+gitd0c8b1f with plain torch==2.13.0 and
that step PASSED; only the nvidia-* assertion caught the corruption. A CPU-only or
non-`+cu` CUDA wheel would satisfy both torch assertions.

Capture the exact baseline torch version before the install and require the
post-install version to be byte-for-byte identical. That directly tests the
invariant that ComfyUI must not replace the managed runtime's torch; the nvidia-*
assertion stays as an additional diagnostic. The baseline premise check
(is_cuda_torch) is unchanged.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): graduate the EAI-8051 scenario to expected-pass

The ComfyUI fix landed in 3e59435 (ROCm#298), pinning the managed runtime's torch stack through uv constraints. The scenario now asserts behavior main implements; remove its xfail row so a pass is not a fatal stale XPASS. Keep the scenario as the regression guard.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): require the ComfyUI install to actually install something

A zero exit code alone still permits a vacuous pass: `comfyui::install`
guards the whole `uv` step with `if !packages.is_empty()`, so an empty
filtered requirement list skips the dependency install and still exits
0, leaving the runtime untouched and every runtime-preservation
assertion passing having exercised nothing.

Capture the runtime's distribution set alongside the torch version in
the baseline step, and require it to have grown after the install.
Asserting growth rather than naming an expected ComfyUI dependency
keeps the check from rotting as ComfyUI's requirements drift upstream,
while still establishing the premise the invariants need.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): address review of the comfyui-05 scenario

Past-tense the ROCm#298 comment, document that the rc check catches a ROCm#298
revert, rename the torch step to 'unchanged', surface probe errors, assert
a single runtime root, and fix the walk doc.

Signed-off-by: fredespi-agent-app[bot] <266462846+fredespi-agent-app[bot]@users.noreply.github.com>
Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): tighten comfyui-05 names, docs and install flags per review

Reuse the argument-less install When step, pass --yes to install sdk,
rename the premise/identity steps to what they check, fix two doc comments.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): compare the whole torch stack and use user-facing step text

Snapshot torch, torchvision and torchaudio before and after the install,
drop harness wording from the comfyui-05 steps, fix the layout doc.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): simplify comfyui-05 probes and correct its docs

One torch-stack probe for premise and baseline, unconditional sdk install,
accurate growth message, ComfyUI-specific success step, docs matching the
stack comparison.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): look only at install_root for the comfyui-05 interpreter

Drop the fallback walk that could baseline an unrelated interpreter, and
note the nvidia check only has teeth on Linux.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): tidy comfyui-05 docs, listing failure and nvidia predicate

Say 'torch stack' in the docs, surface a failing runtimes list, share the
nvidia filter and name offenders in the baseline message, widen the feature
title.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): correct the comfyui-05 runtimes-list rationale

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): gate comfyui-05 to Linux, normalise nvidia names, fix isolation wording

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): exclude WSL2 from comfyui-05 and fix isolation wording

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): drop unreachable Windows arm, name the nvidia check for what it matches

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): name the OpenMPI and engine install in comfyui-05's host-impact comment

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): say comfyui-05 does the same host-level work as runtime-01

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* test(e2e): correct the torch-pin doc comment and the WSL2 gate rationale

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* docs(e2e): document comfyui-14 and untangle its Windows/WSL2 gate comment

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* docs(e2e): stop recommending -n for comfyui-14; note -n skips the OS gate

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

* docs(e2e): warn that name_filter bypasses comfyui-14's gates too

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

---------

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
Signed-off-by: fredespi-agent-app[bot] <266462846+fredespi-agent-app[bot]@users.noreply.github.com>
Co-authored-by: fredespi-agent-app[bot] <266462846+fredespi-agent-app[bot]@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
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.

2 participants