Skip to content

fix(uv): colocate the uv Python install dir with the ROCm CLI data dir - #240

Open
jgmelber wants to merge 1 commit into
ROCm:mainfrom
jgmelber:fix/uv-python-install-dir-colocation
Open

fix(uv): colocate the uv Python install dir with the ROCm CLI data dir#240
jgmelber wants to merge 1 commit into
ROCm:mainfrom
jgmelber:fix/uv-python-install-dir-colocation

Conversation

@jgmelber

Copy link
Copy Markdown

Summary

Follow-on to #170. That PR pointed uv at a cache inside the ROCm CLI data directory; this one does the same for the standalone CPython interpreters uv python install downloads, which still land in $HOME.

Root cause

uv_command_env() now sets UV_CACHE_DIR, but never set UV_PYTHON_INSTALL_DIR. ensure_managed_python shells out to uv python install, so the interpreter falls back to uv's own default — $HOME/.local/share/uv/python/ on Linux/macOS, %USERPROFILE%\.local\share\uv\python\ on Windows — regardless of where ROCM_CLI_DATA_DIR points.

That is ~100MB of standalone CPython per interpreter version, outside the managed root. Measured with uv 0.12.3 against a throwaway HOME:

$ uv python install 3.12          # UV_PYTHON_INSTALL_DIR unset
$ du -sh $HOME/.local/share/uv/python/cpython-3.12.12-linux-x86_64-gnu
104M

With the variable set to a managed location, the same 104M lands under the data dir and $HOME keeps only uv's 12KB bin/python3.12 shim — a symlink into the managed interpreter, not a copy.

This is the same class of bug as #160 and it bites the same users: a split /home, a container overlay, or anyone who set ROCM_CLI_DATA_DIR to a larger disk and expects the CLI to stay there. rocm uninstall also could not reclaim it, since it only removes directories it knows about.

Technical decisions

Keyed off data_dir, for the same reason as the cache: uv hardlinks interpreters into the environments it creates, and ROCM_CLI_CACHE_DIR can point at a different filesystem from those environments.

Mirrors UvCacheSource exactly. UvPythonInstallDirSource has the same Managed / Override / Inherited variants, the same path() / is_override() surface, and the same precedence: namespaced ROCM_CLI_UV_PYTHON_INSTALL_DIR > ambient UV_PYTHON_INSTALL_DIR > managed default. It reuses meaningful_cache_dir for trimming rather than duplicating it. The two variables should read as one design applied twice, not two designs.

uv_command_env(&AppPaths) keeps its signature. Both env vars are composed inside it, so therock.rs, comfyui.rs, and the vLLM engine pick up the fix with no call-site edits. This matters for correctness beyond tidiness: ensure_managed_python calls uv python install and uv python find through the same helper, so find cannot disagree with where install put the interpreter.

A one-shot notice, not a migration, following maybe_notice_legacy_uv_cache with its own marker file. Nothing is moved or deleted — the legacy directory may hold interpreters other uv projects depend on.

Non-goal: --prefix installs — #187

Same gap as the cache, same reason. --prefix relocates install_root only; the interpreters stay keyed off data_dir. Documented in docs/manual-testing.md and pinned by uv_python_install_dir_does_not_follow_a_prefix_install_root, mirroring the existing cache test.

Consequences worth noting

  • rocm uninstall now reclaims the interpreters too, since they sit under the data directory. --keep-data / --keep-cache help text updated to say so; --keep-cache does not cover them.
  • Where the install dir has been pointed outside the data directory, uninstall now reports it as a shared cache it is not removing, extending fix(uninstall): report the caches it does not remove #175's reporting rather than silently leaving ~100MB behind.
  • The interpreters are no longer shared with other uv projects. ROCM_CLI_UV_PYTHON_INSTALL_DIR restores that.
  • Existing installs re-download the interpreter once.

Tests

Six new resolution tests mirroring the cache suite — unset, empty, whitespace-only, trimmed, inherited, namespaced-override, and precedence between the two — driving the pure resolver, so they need no set_var and stay parallel-safe. managed_python_install_dir_tracks_rocm_cli_data_dir goes through AppPaths::with_managed_root.

The uninstall-reporting test is the exception: it drives the real shared_cache_candidates() behind the existing env lock, because a version calling the pure helper with hand-built inputs still passed when the production block was deleted. Each new test was mutation-checked — production code removed, test confirmed failing, code restored.

cargo fmt --all --check and cargo clippy --locked --workspace --all-targets -- -D warnings are clean. Full workspace suite passes.

Manual verification

uv python install 3.12 under a throwaway HOME with both variables set: the 104M interpreter lands under the managed data dir, and $HOME receives only the 12KB shim symlink.

Follow-on to ROCm#170/ROCm#160: uv_command_env() colocated UV_CACHE_DIR with the
managed data dir but left UV_PYTHON_INSTALL_DIR unset, so `uv python install`
(invoked by ensure_managed_python) still fell back to uv's own default of
$HOME/.local/share/uv/python/, leaking a ~100MB standalone CPython interpreter
outside the managed root.

Mirrors ROCm#170's UvCacheSource design for the interpreter install dir:
UvPythonInstallDirSource with the same managed/override/inherited precedence, a
namespaced ROCM_CLI_UV_PYTHON_INSTALL_DIR escape hatch, a one-shot notice for
the legacy location, and updated uninstall help text.

Also extends the ROCm#175 uninstall reporting: now that the interpreters are the
other large thing rocm-cli causes to be downloaded, an overridden install dir
outside the data dir is reported as "not removed" rather than silently left
behind.

Signed-off-by: Joseph Melber <jmelber@amd.com>
@jgmelber
jgmelber requested a review from a team as a code owner August 12, 2026 18:27
@fredespi

Copy link
Copy Markdown
Collaborator

Reviewed at c340bbe0 (diff against f8e9629, 7 files, +346/-29). One thing to fix, and it's narrow. The rest is careful work that does what the description says.

The uninstall report doesn't honour the override this PR introduces

shared_cache_candidates() resolves the interpreter location with:

let uv_python = env_path("UV_PYTHON_INSTALL_DIR").or_else(|| home/.local/share/uv/python);

but uv_python_install_dir_source() — the thing that actually decides where interpreters go — puts the namespaced variable first: ROCM_CLI_UV_PYTHON_INSTALL_DIR > UV_PYTHON_INSTALL_DIR > managed. So with ROCM_CLI_UV_PYTHON_INSTALL_DIR=/mnt/big/pythons set and the ambient variable unset — which is precisely the configuration this PR documents for restoring sharing with other uv projects — the interpreters land in /mnt/big/pythons, while rocm uninstall reports $HOME/.local/share/uv/python: a path that is stale, may not exist, and isn't where the ~100MB is. The real location is never mentioned.

That contradicts the stated consequence in the description ("uninstall now reports it as a shared cache it is not removing ... rather than silently leaving ~100MB behind"). It holds for the ambient variable and fails for the namespaced one.

The new test pins only the ambient path (ScopedEnvVar::set_path("UV_PYTHON_INSTALL_DIR", …)), so it passes without covering the gap.

Cleanest fix is to stop re-deriving the location in main.rs and call the resolver that already encodes the precedence — uv_python_install_dir_source(&paths).path() — with the same treatment for the cache entry above it. Worth noting the cache entry has the identical gap on main today (UV_CACHE_DIR read directly, ROCM_CLI_UV_CACHE_DIR ignored), so this isn't a regression you introduced — but this PR adds a second instance of it and makes a reporting promise that depends on it, which is what makes it worth closing now rather than inheriting.

What I checked and found sound

The two variables really are one design applied twice. UvPythonInstallDirSource mirrors UvCacheSource variant-for-variant, reuses meaningful_cache_dir for blank/whitespace trimming instead of re-implementing it, and the tests cover the same matrix — managed default, inherited, namespaced-wins-over-ambient, blank-is-not-an-override, whitespace-trimmed, and tracking a moved ROCM_CLI_DATA_DIR.

Keeping uv_command_env(&AppPaths)'s signature is load-bearing, not just tidy. Because both variables are composed inside it, uv python install and uv python find are guaranteed to agree on the location — a real correctness property, since a find that disagreed with install would silently re-download or resolve the wrong interpreter. Call sites in therock.rs, comfyui.rs and the vLLM engine pick it up untouched.

The --prefix non-goal is pinned, not just documenteduv_python_install_dir_does_not_follow_a_prefix_install_root mirrors the existing cache test, so the caveat can't quietly drift.

The legacy notice is correctly conservative — gated on the managed dir existing, skipped under an override, one-shot behind a marker file, and it neither moves nor deletes anything. Right call given other uv projects may depend on those interpreters.

Non-blocking

maybe_notice_legacy_uv_python_install_dir reads HOME/USERPROFILE directly, while the shared_cache_candidates hunk in this same diff uses rocm_core::runtime_home_dir(), which additionally handles the HOMEDRIVE+HOMEPATH fallback on Windows. The direct read is faithful to the sibling maybe_notice_legacy_uv_cache it's modelled on, so it's consistent with what's there — but the PR now contains both spellings a few hundred lines apart. Worst case is a missed advisory line on an unusual Windows setup, so it's cosmetic; just easier to unify while both are being touched.

Diff review only — I haven't run a real uv python install against a relocated data dir, so the ~104MB figure and the bin/python3.12 shim behaviour are taken from your measurement, not reproduced. CI is green (23/23).

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