[jussielo-amd]

Summary

Fixes ROCMAI-878. Three places turned a Python interpreter's path into "the environment folder this interpreter belongs to", and they gave different answers for the same path. The ComfyUI runtime environment returned a folder only when the interpreter sat in a bin or Scripts folder (any case) and otherwise left VIRTUAL_ENV unset. rocm runtimes adopt and the vLLM launch both took the interpreter's grandparent whatever its own folder was called: the adopt helper had a bin/Scripts check, but both of its branches returned the same value, so it was dead code, and the vLLM copy never had a check at all. An interpreter in some unrelated folder was therefore refused by one caller and guessed at by the other two, and a guess names a folder that is not an environment.

This replaces the three private copies with one rule in rocm-core, runtime_python_env_root_for_executable, next to the existing runtime_python_env_bin_dir and runtime_python_executable_in_env that it inverts. An interpreter belongs to an environment only when it sits directly in a bin or Scripts folder, in any letter case and on any host, and the answer is that folder's parent. Anything else returns None: another folder, a bare file name, a file at the filesystem root. The engine crate cannot depend on apps/rocm and rocmd must never depend on rocm, so rocm-core is the one place all three callers can reach (cargo xtask check-crate-edges passes, no new edge).

I chose the ComfyUI behaviour as the winner because refusing is the only answer that cannot be wrong. The grandparent of an interpreter in /opt/tools/python is /opt, which adopt would have registered as a runtime and the vLLM launch would have searched for lib/python3.*/site-packages. Each caller already had a way to say no, and the two that were guessing now use it.

What moved

Before	After
infer_python_env_root in apps/rocm/src/runtime_adopt.rs (grandparent, dead bin/Scripts branch)	deleted; resolve_adopt_python_input calls rocm_core::runtime_python_env_root_for_executable
infer_python_env_root in apps/rocm/src/comfyui/runtime_env.rs (bin/Scripts or None)	deleted; both venv_root sites call the shared function
python.parent().and_then(Path::parent) in vllm_venv_root, engines/vllm/src/process.rs	calls the shared function; the existing "is not inside a venv (bin/Scripts) layout" error is kept
(new)	runtime_python_env_root_for_executable in crates/rocm-core/src/runtime.rs, re-exported at the crate root

User-visible behaviour

Only the two callers that guessed change, and only for an interpreter that is not directly inside a bin or Scripts folder. For a valid venv layout (<env>/bin/python, <env>/Scripts/python.exe, any letter case) every caller returns exactly what it returned before. The complete list of differences:

1. rocm runtimes adopt --python <file> without --root, where the file's folder is neither bin nor Scripts. Before: the interpreter was probed and its grandparent folder was adopted as the runtime's install root. Now: the command stops with the existing message could not infer the Python environment folder from <path>; pass --root and registers nothing. Passing --root still adopts that same interpreter, as before. The --root help text and the README's adopt paragraph now say this (the help text used to call --root "Optional SDK root path").
2. The vLLM launch on a ROCm 10.x discovery install, where the runtime's recorded interpreter is not in a bin or Scripts folder. Before: the grandparent was used as the venv, so the launch either failed with a read error on a lib folder that does not belong to the environment or, worse, found a python3.*/site-packages under the wrong folder. Now: it fails up front with the error that was already written for this case, python executable ... is not inside a venv (bin/Scripts) layout. A runtime recorded by rocm install sdk or by adopt always has its interpreter in bin/Scripts, so this is reachable only through a hand-edited or imported manifest.
3. ComfyUI: no difference. VIRTUAL_ENV is set for bin, Scripts, SCRIPTS and BIN, and left unset otherwise, before and after.

A relative interpreter path such as bin/python still yields an empty folder from all three callers, as before; the adopt command makes its input absolute first and the manifests store absolute paths, so I left that alone.

There is a scenario for item 1: runtime-23 (@id:runtime-adopt-outside-a-venv-asks-for-root, Linux, ordinary E2E lane). It plants a stand-in interpreter in a tools folder, adopts it without --root, and asserts the refusal asks for --root and that rocm runtimes list still shows no runtime. runtime-07 (adopting /usr/bin/python3 with --root /opt/rocm) is unaffected and still passes. Item 2 is internal to a launch path that needs a real vLLM install, so it is covered by the unit test below rather than a scenario. The "--root clears the refusal" half of item 1 is covered by the unit test adopt_request_without_an_inferred_root_asks_for_root, because adopting for real needs a genuine ROCm SDK environment.

Verification

Ran: cargo fmt --all --check; cargo clippy -p rocm -p rocm-core -p rocm-engine-vllm -p e2e-cucumber --all-targets -- -D warnings; cargo test --no-fail-fast for rocm-core, rocm-engine-vllm and rocm; cargo xtask manifest --check; cargo xtask check-crate-edges; the runtime-07 and runtime-23 scenarios through cargo xtask e2e -- -n "runtime-(07|23)"; the leak scan against origin/main. No test was removed.

The tests fail when the rule re-diverges. The shared rule has its own table in rocm-core (bin, BIN, Scripts, SCRIPTS, scripts, an unrelated folder, a bare file name, a root-level file, and a round trip through runtime_python_executable_in_env; paths are built with join, so they mean the same on Linux and Windows). Each caller has its own test that goes through the shared function: runtime_environment_exports_virtual_env_only_for_a_bin_or_scripts_folder (ComfyUI), adopt_python_file_infers_a_root_only_from_a_bin_or_scripts_folder and adopt_request_without_an_inferred_root_asks_for_root (adopt), vllm_venv_root_needs_a_bin_or_scripts_folder (vLLM). I checked each one goes red by breaking the code it covers; the evidence is in the hand-back notes rather than repeated here.

Not run: the full workspace gate, scripts/smoke_local.py and the rest of cargo xtask e2e (CI carries them). The cfg(windows) side was not separately exercised beyond the shared function's host-independent table, which runs on the Windows job too.

Notes for reviewers

Read the two commits in order. The first adds only tests that pass on unmodified origin/main and pin each caller's behaviour, including the divergences; the second changes the rule, flips the two assertions that move (adopt and vLLM for the unrelated-folder case), adds the shared function's own tests, the scenario, the docs and the docs/architecture.md entry. runtime-23 is the next free scenario number at the time of writing and will need renumbering if another scenario lands first: #906 is still open and adds runtime-23 to runtime-25, so whichever of the two merges second renumbers its scenarios. runtime_adopt.rs, comfyui/runtime_env.rs and engines/vllm/src/process.rs are also touched by refactors in flight, but in different regions.

[Copilot]

🟢 Approval recommended

No active review issues remain for the change centralizing Python environment-root inference.

No open issues 🎉

What changed in this PR

Centralizes Python environment-root inference in rocm-core so runtime adoption, ComfyUI, and vLLM use the same rule instead of separate implementations.

Changes:

• Adds a shared helper accepting interpreters directly inside bin or Scripts, case-insensitively.
• Stops adoption and vLLM from guessing roots for other layouts, while preserving ComfyUI behavior.
• Adds regression tests and an adoption-refusal scenario, and updates help and documentation.

File	Description
tests/​e2e-cucumber/​tests/​e2e/​runtime_steps.rs	Adds interpreter fixtures and refusal/registration assertions.
tests/​e2e-cucumber/​features/​runtime_setup.feature	Adds the missing-root adoption refusal scenario.
README.md	Explains inferred roots and when --root is required.
engines/​vllm/​src/​process.rs	Uses shared inference and tests invalid layouts.
docs/​testing.md	Documents adoption behavior and scenario coverage.
docs/​architecture.md	Documents the shared helper and its callers.
crates/​rocm-core/​src/​runtime.rs	Implements and tests the shared inference rule.
crates/​rocm-core/​src/​lib.rs	Re-exports the helper.
apps/​rocm/​src/​runtime_adopt.rs	Replaces local inference and tests root handling.
apps/​rocm/​src/​main.rs	Clarifies the adopt --root help text.
apps/​rocm/​src/​comfyui/​runtime_env.rs	Uses shared inference and tests VIRTUAL_ENV behavior.

_{Review effort: Balanced}

[jussielo-amd]

Superseded by #934: this pull request's commits are included in that branch (head 6f043a8), and #934's description covers this change. Closing so the review happens in one place.