ROCMAI-872: keep the update check from preparing a managed Python by jussielo-amd · Pull Request #951 · ROCm/rocm-cli

jussielo-amd · 2026-11-06T14:53:33Z

Reading the index for a wheel runtime needs the interpreter's wheel tag, and a runtime that records no usable Python made the update check go and get one: on a host with none it downloaded a managed uv, had uv install Python and saved that Python's path in the configuration. That ran for rocm update, for rocm update --json (the dashboard's check), for the check the CLI runs ahead of other commands, and for the plan behind rocm update --apply --dry-run, so a read-only look at the runtimes changed the host. It was found while fixing the same problem for the SDK install preview in #906.

These callers now go through a new runtime_update_check, which finds an interpreter with the search-only entry point #906 added and, when there is none, takes the wheel tag from the interpreter the layout requires. The text report adds a python_note: line saying an update would use uv to install the managed Python, which the check did not do; the JSON keeps its shape, since the dashboard parses it. The note's caveat about an unreadable wheel tag reuses the helper #906 introduced. runtime_update_plan, which rocm update --apply works from, still prepares the Python, because the install that follows needs it, so a real apply behaves as before. Which of the two plans an update takes is chosen in one small function, so a unit test pins it on every platform. The comment that documents the side effects of the assistant's read-only tools now says that update_check and install_sdk_dry_run prepare no Python.

Tests: a unit test drives the report, the JSON, the startup check, the check plan and apply_runtime_update with --dry-run on a host with no Python and a recording uv, with the apply plan and a real apply as the controls that show the stand-in is reachable. Scenarios update-09 (the report) and update-10 (the --dry-run preview) drive the built binary and check the note against the state it left: the recording uv was never run, and there is no managed-tools folder and no saved config. Both fail without the change. They run on Linux, which is the host the empty-PATH setup is built for. Known behaviours, not changed here: the progress line about a PATH Python that is not usable prints to standard output ahead of the report, as it already does for the install preview, and a ROCM_CLI_MANAGED_PYTHON_VERSION whose wheel tag does not fit makes the check fail with the planner's existing wording about what a real install would prepare. As for the install preview, resolving a ROCm 10 or newer runtime uses uv pip compile and can still fetch the managed uv, though never a Python. Not run locally: the full workspace gate, Windows and the GPU lanes.

On a host with no usable Python, rocm install sdk --dry-run (and the install_sdk_dry_run assistant tool) went on to download a managed uv, have it install Python and save that Python's path in the config before printing a plan, although a preview promises to change nothing. The preview only needs the interpreter's wheel tags, so it now reports that a real install would prepare a managed Python (python_note) and prepares nothing; every refusal the real resolution raises is raised for the preview too. A real install resolves and prepares exactly as before. Adds unit tests for the no-Python, PATH Python, saved Python, stale saved Python and bootstrap-disabled cases, and scenario runtime-22, which checks the preview's note together with the empty tools, cache and config folders and a recording uv that is never run. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

… refuse With ROCM_CLI_MANAGED_PYTHON_VERSION naming a version whose wheel tag is not the one the layout needs (3.13 against cp312), a dry run on a host with no usable Python said it would prepare that Python, but a real install prepares it, finds the wrong tag and refuses. The preview now applies the same tag rule, shared with the check an existing interpreter must pass, and fails with the install's message instead. A version whose tag cannot be read off its name is announced as before. Adds a unit test and scenario runtime-23, which asserts the refusal together with the empty tools, cache and config folders. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

The harness points the CLI at a cache shared across scenarios and, on CI, across runs. runtime-22 asserted that the preview had downloaded no tool archive by looking in that shared folder, so a uv archive left by an earlier run failed it although the preview downloaded nothing. The scenarios now set ROCM_CLI_CACHE_DIR to a folder under the scenario root and assert against it. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…y run The dry run's check of ROCM_CLI_MANAGED_PYTHON_VERSION read only plain major.minor versions, so 3.13t, cpython-3.13 and cpython@3.13 were announced as a plan although the installed interpreter reports cp313 and the real install refuses it. The version is now read in those spellings. A version whose tag still cannot be read is announced as unchecked, in both the note and the progress line, rather than as a promise. The progress line before the search no longer says a real install would prepare a Python, since the search can end in a refusal; the line naming the Python, or the refusal, follows it. runtime-23 asserts the whole output: no promise before the refusal. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…evidence ROCM_CLI_UV_BINARY makes the CLI return the recording uv before any download could begin, so the check for a tool archive in the scenario's cache cannot fail by itself. It stays as a backstop and says so; the scenario comments and docs/testing.md now name the uv log as what shows nothing was prepared. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Signed-off-by: Jussi Elo <jussi.elo@amd.com> # Conflicts: # apps/rocm/src/therock.rs # tests/e2e-cucumber/features/runtime_setup.feature

Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…sion With an unusable PATH Python, no usable saved interpreter and managed Python disabled, the preview said a real install would use managed Python and then refused; the saved-interpreter line made the same premature claim. The two lines now state only what was found. The one line that predicts the preparation is built from the decision itself and printed by python_for_install, so it exists only for a decision that passed the bootstrap policy and the version check; the Using lines moved there with it. A real install prints exactly what it did. Adds a state-matrix unit test over PATH Python, saved Python, bootstrap policy and version override that asserts each row's whole output and the state left behind, and scenario runtime-25 for the policy refusal. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

A preview of a ROCm 10 or newer wheel resolves its package versions with uv pip compile, so on a clean host it can still download the managed uv and create the data folder's tools folder, although it prepares no Python. docs/testing.md, docs/manual-testing.md and the dash-TUI tool-group comment now say so, and describe the state matrix and the refusal scenarios the preview is checked by. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

The doc comment said a wildcard yields None, but "3.13.*" reads as cp313: whatever follows the minor is not validated, because the major and minor alone decide the wheel tag. A wildcard that leaves the minor open ("3.*", "*") is what yields None. Reword the comment to say so and pin both unreadable wildcards in the existing reader test. Also say in the comment that this hand-rolled reader can drift from what uv accepts, and that the failure is benign: an unreadable spelling is announced as unchecked rather than refused. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

A preview that would prepare a managed Python whose wheel tag cannot be read off its name said so twice, in the python_note line and in the progress line, each with its own sentence. Build the caveat from one function so the two cannot drift. The note is unchanged; the progress line now reads "... would prepare Python pypy@3.10 (its wheel tag cannot be checked until it is installed; cp312 is required). This preview does not." instead of using a "whose ... " clause. The unit tests that pin both lines are updated to match. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

PythonLauncher::source was a bare &'static str ("env", "path", "managed", "manifest") that three places matched or compared by spelling, with a catch-all arm in the progress line that would hide a new spelling. Make it a PythonSource enum with an as_str for the install plan's python_source line, and match it exhaustively in the progress text. The printed output is unchanged. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…naged-Python policy PythonResolverEnv already injects the two inputs that locate an interpreter (ROCM_CLI_PYTHON and the search directories), but the two inputs that decide what happens when none is found were still read from the process environment inside the decision: whether the managed bootstrap is disabled and which managed Python version is requested. A test of that decision therefore had to mutate process-wide environment variables under a lock. Both now travel in PythonResolverEnv (read once by from_process_env, which the real resolver still uses), and find_python_launcher_in, plan_python_launcher_in and the real install's ensure_managed_python take them from it, so a preview and an install made from one environment cannot disagree about the version or the policy. managed_python_version keeps its signature for the progress line in therock.rs and still reads the process environment. No behaviour change: the existing tests, which build an environment from the process one, pass unchanged. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

The tests of what a dry run decides about Python drive it with shell-script interpreters and are cfg(unix), and the scenarios that cover it are Linux-only, so the Windows CI job ran none of it although a fresh Windows install is one of the hosts the preview is for. The decision needs no interpreter: with nothing to find, python_for_install is a function of the resolver environment (now carrying the managed-Python policy) and the dry_run flag. The new test builds that environment directly and runs python_for_install through it, so it runs on the Windows job. It asserts, per row, the preview's complete progress output together with the state it leaves behind (no config, manifest, or folder created): the default, a patch release, a blank override and an unreadable spelling are announced (the last as unchecked); a version whose wheel tag is not the layout's is refused in every spelling that reads as one; a disabled bootstrap is refused with the install's own message. A control runs the real install on the same host and checks it reaches the managed uv, which is a file that cannot be launched, so nothing is downloaded or run and the preview's silence about uv is shown to mean something. The search order (python3 before python on Linux, python, python3, py with PATHEXT on Windows) and the interpreter probes are still covered by the Unix tests only. RestoredEnv and snapshot lose their cfg(unix) because the new test shares them. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…l tag PlannedPython::WouldPrepareManaged carried a tag_checked bool next to the requirement it was checked against. The bool is one of two named cases, so it is now a TagCheck enum (Checked, Unchecked), and unchecked_tag_caveat matches on it. The preview's note and progress line are unchanged; the existing tests, updated to the enum, pass as before. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

… install and a preview An install and a preview word the two findings about a PATH Python and a saved managed Python that cannot serve the install differently (the install says what it goes on to do, the preview states the finding only), but only the preview's wording was pinned, by the state matrix on Unix hosts. This pins both, on every platform, before the two parallel match blocks that print them are merged into one. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

find_python_launcher_in printed the finding about a PATH Python and the one about a saved managed Python from two parallel Install/Preview match blocks that differed only in a trailing clause. LookupPurpose::unusable_python_line now builds both lines: both purposes state the finding, an install adds what it goes on to do. The output is byte-identical, as the test added in the previous commit pins for both purposes. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…d uv venv line is not verbatim docs/testing.md and the runtime-23 scenario comment now name the unit test that runs the preview's Python decision on every platform, the Windows CI job included, and say what stays uncovered (the Windows search order and any Windows run of the preview). docs/testing.md and docs/manual-testing.md also say that the plan's command line prints uv venv --python 3.12, a version rather than a path, so it is not copy-pasteable verbatim: uv resolves 3.12 itself and may choose a different Python 3.12 than the managed one a real install prepares. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Re-wrap the manual-testing bullet and the testing.md paragraph on the dry-run Python preview to the surrounding 80-column style. No words change. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Reading the index for a wheel runtime needs the interpreter's wheel tag, and a runtime that records no usable Python made the check go and get one: on a host with none it downloaded a managed uv, had uv install Python and saved that Python's path in the configuration. That ran for `rocm update`, for `rocm update --json` (the dashboard's check), for the check the CLI runs ahead of other commands, and for the plan behind `rocm update --apply --dry-run`, so a read-only look at the runtimes changed the host. These callers now go through `runtime_update_check`, which finds an interpreter with the search-only entry point the install preview already uses and, when there is none, takes the wheel tag from the interpreter the layout requires. The text report adds a `python_note:` line saying an update would use uv to install the managed Python, which the check did not do; the JSON keeps its shape. `runtime_update_plan`, which `rocm update --apply` works from, still prepares the Python, since the install that follows needs it. A unit test drives the report, the JSON, the startup check and the check plan on a host with no Python and a recording uv, with the apply plan as the control that shows the stand-in is reachable. Scenarios update-09 and update-10 drive the report and the dry-run preview through the built binary and check the note against the state it left. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Copilot

🟡 Changes recommended

The tool side-effect comments still claim Python is installed and its path saved, so they need to reflect search-only resolution while retaining the ROCm 10 caveat about acquiring uv.

1 open issue

Correct update_check side-effect documentation · New

What changed in this PR

This PR extends #906’s search-only Python planning to runtime update checks, preventing checks and dry runs from preparing managed Python while preserving preparation for real updates.

Changes:

Routes text, JSON, startup checks, and update previews through search-only planning, adding a text-only python_note without changing the JSON shape.
Adds regression tests and Linux scenarios that check output alongside evidence that no Python was prepared.
Updates architecture and testing documentation; the stacked diff also includes #906’s SDK-preview changes.

Tests were not run in this review environment.

File	Description
tests/e2e-cucumber/tests/e2e/therock_steps.rs	Adds no-Python fixtures and output/state assertions.
tests/e2e-cucumber/src/bin/fake-uv.rs	Records invocations to detect unwanted preparation.
tests/e2e-cucumber/features/update.feature	Adds update-report and dry-run regression scenarios.
tests/e2e-cucumber/features/runtime_setup.feature	Adds SDK-preview and refusal scenarios from #906.
docs/testing.md	Documents coverage, commands, and resolution caveats.
docs/manual-testing.md	Adds manual checks for Python-free previews and updates.
docs/architecture.md	Describes separate update-check and apply planning paths.
crates/rocm-dash-tui/src/agent/tools.rs	Revises comments documenting read-only tool side effects.
apps/rocm/src/update_cmd.rs	Selects search-only planning for dry runs.
apps/rocm/src/therock/update.rs	Introduces search-only update checks and regression tests.
apps/rocm/src/therock/test_support.rs	Adds scoped environment restoration for tests.
apps/rocm/src/therock/python.rs	Separates Python discovery from preparation and generates planning notes.
apps/rocm/src/therock.rs	Integrates SDK-preview planning and exports the update-check entry point.

_{Review effort: Balanced}

jussielo-amd · 2026-11-06T16:07:38Z

+// `update_check` can also, on a host with no usable Python on PATH, download a
+// managed `uv` and Python into the data folder and record that Python in the
+// saved config; `install_sdk_dry_run` prepares no Python and saves no Python
+// path: on such a host it reports the Python a real install would prepare. (A
+// ROCm 10 preview still resolves package versions with `uv` and can fetch the
+// managed `uv` into the data folder.) The boundary type


Fixed in 96f121a. You are right that the comment still described the old behaviour. It now says that neither update_check nor install_sdk_dry_run prepares a Python or saves a Python path: on a host with no usable Python they report the Python a real update or install would prepare, and update_check runs rocm update, whose text report carries a python_note: line saying so. Only a real apply or install prepares it. The ROCm 10 caveat stays: a check or preview there can still fetch the managed uv, though never a Python. I found no other place that repeated the old claim.

The base pull request merged, so this branch now carries only its own change on top of main. main's version of everything that pull request owns is kept as merged, including its later review fixes; the update check, its plan function, the note, the shared test helper, the scenarios and the docs are the only content added here. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

… prepare The side-effects note on the read-only ROCm tools still said that `update_check` can download a managed `uv` and Python and record that Python in the saved config, which is the behaviour the update check no longer has: it runs `rocm update`, which now only reports the Python a real update would prepare, in a `python_note:` line of its text report. The note now says that neither `update_check` nor `install_sdk_dry_run` prepares a Python or saves a Python path, that only a real apply or install does, and keeps the caveat that a ROCm 10 or newer check or preview can still fetch the managed `uv`. No other file repeated the old claim. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Copilot

🟢 Approval recommended

No active review issues were identified for the change separating search-only update checks and dry-run previews from Python preparation during apply.

1 open issue

Correct update_check side-effect documentation

_{Review effort: Balanced}

main moved after the base pull request merged, and one line of docs/architecture.md, the entry for therock/update.rs, was changed on both sides. The resolution keeps main's wording (including the root-payload clause) and adds only this change's two additions: the sentence about runtime_update_check and its name in the list of functions the entry covers. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

main moved again (the chat.rs split) and the entries for therock/update.rs, therock/test_support.rs and tool_policy.rs in docs/architecture.md, which sit next to each other, were changed on both sides. The resolution keeps main's text for the entries this change did not touch and this change's wording for the therock/update.rs entry, which main had left as it was. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd added 21 commits November 5, 2026 10:26

Merge origin/main into fix-install-sdk-dry-run-without-side-effects

56179c6

Signed-off-by: Jussi Elo <jussi.elo@amd.com> # Conflicts: # apps/rocm/src/therock.rs # tests/e2e-cucumber/features/runtime_setup.feature

Merge origin/main into fix-install-sdk-dry-run-without-side-effects

87ea1ce

Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Merge origin/main into fix-install-sdk-dry-run-without-side-effects

f342fce

Signed-off-by: Jussi Elo <jussi.elo@amd.com>

docs: wrap two over-long lines in the install sdk dry-run notes

fc2fa6e

Re-wrap the manual-testing bullet and the testing.md paragraph on the dry-run Python preview to the surrounding 80-column style. No words change. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd requested a balanced review from Copilot November 6, 2026 14:53

Copilot started reviewing on behalf of jussielo-amd November 6, 2026 14:54

View session

Copilot AI reviewed Nov 6, 2026

View reviewed changes

jussielo-amd added 2 commits November 6, 2026 15:38

jussielo-amd requested a balanced review from Copilot November 6, 2026 16:07

Copilot started reviewing on behalf of jussielo-amd November 6, 2026 16:08

View session

Copilot AI reviewed Nov 6, 2026

View reviewed changes

jussielo-amd marked this pull request as ready for review November 6, 2026 16:15

jussielo-amd requested a review from a team as a code owner November 6, 2026 16:15

jussielo-amd requested a review from juhovainio November 6, 2026 16:15

jussielo-amd added 2 commits November 6, 2026 16:53

jussielo-amd mentioned this pull request Nov 6, 2026

fix(install): preview a ROCm 10 SDK install without fetching uv #954

Open

Navigation Menu

Repository navigation

ROCMAI-872: keep the update check from preparing a managed Python - #951

ROCMAI-872: keep the update check from preparing a managed Python#951
jussielo-amd wants to merge 25 commits into
mainfrom
fix-update-check-prepares-no-python

jussielo-amd commented Nov 6, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

jussielo-amd Nov 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jussielo-amd commented Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

jussielo-amd Nov 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jussielo-amd commented Nov 6, 2026 •

edited

Loading