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

fix(comfyui): pin managed torch stack during install (EAI-8051)#298
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
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>
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