Repository navigation
ROCMAI-826: keep the cache folder under an overridden data folder - #834
Conversation
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
The fix is at the right layer: the cache now follows ROCM_CLI_DATA_DIR unless ROCM_CLI_CACHE_DIR says otherwise. The pure from_overrides helper plus the uninstall-04 scenario cover it well, and the spec matches ROCMAI-826. I found nothing broken, but I'd like a few things closed before merge:
- Docs sync. Only README.md changed. Please grep docs/testing.md, docs/manual-testing.md and the storage/uninstall
--helptext for the old "cache is always~/.rocm/cache" claim and update or confirm each. - Behavior change in the PR body. Anyone with only
ROCM_CLI_DATA_DIRset now gets a fresh<data>/cache; the old~/.rocm/cacheis orphaned (re-download, and uninstall no longer cleans it). Please state this in the PR text. - Evidence the new tests bite. Please show the unit tests and
uninstall-04go red with theor_else(managed_runtime_cache_dir)line reverted, and say so in the PR text. - PR text. Name the scenario id (
@id:uninstall-moved-data-folder-keeps-its-own-cache) and the lane that exercises it. - Vacuous unit test.
cache_dir_keeps_its_default_without_a_data_overridereturns early whendefault_cache_dir()isNone, so it can pass without asserting anything (inline). - Please confirm
discover_from_paths/with_managed_rootdoesn't re-root a cache derived from the data folder:cache_dir_override.is_some()stays false in that case. Thediscover()wiring is covered only by the e2e scenario, not the unit tests.
CI: all completed checks are green; the GPU hardware lanes were still pending when I looked.
| data_dir: Option<PathBuf>, | ||
| cache_dir: Option<PathBuf>, | ||
| ) -> Result<Self> { | ||
| let cache_dir = cache_dir.or_else(|| data_dir.as_deref().map(managed_runtime_cache_dir)); |
There was a problem hiding this comment.
This is the behavior change: a user with only ROCM_CLI_DATA_DIR set now gets a new empty <data>/cache, and the old ~/.rocm/cache is orphaned (re-download, and uninstall no longer cleans it). Worth a line in the PR body. Also, please show a test fails when this line is reverted.
There was a problem hiding this comment.
Done. The behaviour change is now spelled out in the PR description (a user with only ROCM_CLI_DATA_DIR set gets a new empty <data>/cache; the old ~/.rocm/cache is orphaned from that install, so archives are downloaded again and uninstall no longer removes it). Red evidence is in the description too: with the or_else(managed_runtime_cache_dir) line removed, cache_dir_follows_an_overridden_data_dir, a_managed_root_does_not_move_a_cache_that_followed_the_data_dir and the e2e scenario (now uninstall-05, after a rebase onto main where uninstall-04 was taken) all fail; with it restored they pass. Pushed as 265fddd.
| // Only the config folder moved: data and cache both stay at their | ||
| // defaults, so the cache is not pulled under the config folder. | ||
| let (root, _) = temp_app_paths("cache-default"); | ||
| let Some(default_cache) = default_cache_dir() else { |
There was a problem hiding this comment.
This returns early when default_cache_dir() is None, so the test passes without asserting anything on such a host. Consider asserting (or expect) so it cannot pass vacuously.
There was a problem hiding this comment.
Fixed in 265fddd: it now expects a default cache folder instead of returning early, so it cannot pass without asserting. To the second point (that it does not exercise the new branch): agreed, and it is not meant to. It pins the unchanged default, i.e. that moving only the config folder does not pull the cache under it, which guards against an over-correction of this change. The tests that exercise the fix are the two above it, and the description says which tests fail without it.
| cache directory is `~/.rocm/cache`. `uninstall` removes a category's directory | ||
| cache directory is `~/.rocm/cache`. The `ROCM_CLI_CONFIG_DIR`, | ||
| `ROCM_CLI_DATA_DIR`, and `ROCM_CLI_CACHE_DIR` environment variables move them; | ||
| when only `ROCM_CLI_DATA_DIR` is set, the cache directory moves with it, to |
There was a problem hiding this comment.
Please also check docs/testing.md, docs/manual-testing.md and the storage/uninstall --help text for the old "cache is always ~/.rocm/cache" claim and update them in this change.
There was a problem hiding this comment.
Checked docs/testing.md, docs/manual-testing.md and the storage / uninstall --keep-cache help text for a claim that the cache is always ~/.rocm/cache: none makes it. Both docs set all three ROCM_CLI_*_DIR variables explicitly, which stays correct, and the help text only says "the cache directory". The README paragraph is the one place that stated the location, so it is the only doc changed; the PR description now says this.
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
Review — needs work
Full review of the whole change.
Implements ROCMAI-826 exactly as scoped: the cache directory now follows ROCM_CLI_DATA_DIR to <data>/cache unless ROCM_CLI_CACHE_DIR overrides it. The ticket's own suggested remedy ("the cache directory follows the configured data directory (or has its own documented override)") is implemented verbatim. The related ticket ROCMAI-465 (uninstall protected-roots guard) is correctly left out of scope and not claimed as closed.
Blocking
-
cache_dir_keeps_its_default_without_a_data_overridedoes not test the change, and can pass vacuously —crates/rocm-core/src/paths.rs:432-446
Traced both required verdicts for an added test: (1) would it still pass with the fix reverted? Yes — with bothdata_dirandcache_diroverrides absent,from_overridesnever reaches the newor_else(|| data_dir.as_deref().map(managed_runtime_cache_dir))branch either before or after the fix, so this test exercises no code path the change touches. (2) it can also assert nothing at all:let Some(default_cache) = default_cache_dir() else { return Ok(()) };(lines 436-438) silently short-circuits to a passingOk(())on any host wheredefault_cache_dir()resolves toNone. No other test in this file uses an early-return-on-Nonepattern, so this isn't an established repo convention. An independent pass over the same file reached the identical conclusion on verdict (1).
Confidence 85 · logic · Fix: either delete this test (the "config dir alone" invariant it checks was never at risk and isn't touched by this diff) or rewrite it to assert something only the new code path could produce, and drop the silent-Noneescape in favor of an explicit skip/log. -
PR text does not show the required red-test evidence for all new tests — PR description;
AGENTS.md:86-90,AGENTS.md:483
AGENTS.md§3 states plainly: "Every bug fix ships with a regression test ... test fails before fix and passes after fix" and "A new test is shown to fail when the code it covers is broken. Before pushing, break the branch under test ... and confirm the test goes red" — repeated as checklist item 14 ("New tests shown to fail when their target is broken"). The PR body states this only forcache_dir_follows_an_overridden_data_dir("The first fails without the fix (resolves to the home cache)"). It gives no such confirmation for theuninstall-04e2e scenario, which is the test that actually exercises the fixeddiscover()wiring end-to-end (the unit tests exercisefrom_overridesdirectly, notdiscover_from_paths/with_managed_root's interaction with it). Reasoning through it, the scenario would plausibly fail correctly if reverted (storage report would then name the simulated home cache, and the uninstall would remove it) — but the repo's own rule requires this to be shown, not reasoned about, before the PR is upstream-ready.
Confidence 80 · logic · Fix: state in the PR text thatuninstall-04was run with the fix reverted and went red (or do so and report it).
Non-blocking
docs/testing.mdanddocs/manual-testing.mdweren't touched despiteAGENTS.md:200-205's "update README.md, ... docs/testing.md, and docs/manual-testing.md in the same change" rule for an observable-behavior change. Checked both files directly: neither currently makes a stale or incorrect claim about where the cache directory resolves (docs/manual-testing.mdalways setsROCM_CLI_CACHE_DIRexplicitly;docs/testing.mdjust lists all three override variables without asserting a relationship between them), so there is no live inaccuracy today — only an unticked procedural step.
Decisions for the author
- Orphaning the old home-directory cache instead of migrating it — tradeoff
The PR explicitly chooses to leave~/.rocm/cachebehind rather than move its contents whenROCM_CLI_DATA_DIRalone is set, reasoning the cache only holds re-downloadable archives. That's a reasonable call, but it is a one-time, silent storage-usage increase (stale archives sit un-cleaned at the old location indefinitely, sinceuninstallof the isolated install no longer reaches them) for any existing isolated-data-dir user who upgrades. Confirm that's acceptable versus, e.g., a one-time migration or a note in the storage report about the orphaned folder.
Positive signals
from_overridesis a good extraction — pulling the override-resolution logic into a pure helper that takes overrides as arguments (rather than reading the environment inside the function) is exactly what let the new unit tests drive it directly without mutating process-global env vars, consistent with the pattern already used elsewhere in this file (e.g.engine_envs_dir_from).- The e2e step assertions are tied to the real production string formats, not re-implemented guesses —
storage_names_data_cache's expected(<path>)suffix anduninstall_skips_default_cache's"removed cache <path>"string both matchstorage.rs::render_reportanduninstall.rs'sreport(format!("removed {} {}", ...))exactly, verified by reading both sides.
Deployment notes
- Confirmed behavior change, consistent with what the PR states: any existing install that sets only
ROCM_CLI_DATA_DIR(notROCM_CLI_CACHE_DIR) will, after upgrading, resolve its cache to a fresh<data>/cacherather than the previous~/.rocm/cache; the old location is left on disk and is no longer reachable byrocm uninstallorrocm storagefor that install.
What this covered
Read completely: README.md's diff hunk, crates/rocm-core/src/paths.rs in full (both at bcd879d007 and at e51a0770e), the relevant functions in crates/rocm-core/src/runtime.rs (env_path_override, default_config_dir/default_data_dir/default_cache_dir, managed_runtime_cache_dir, managed_runtime_data_root, normalize_runtime_path_for_host), tests/e2e-cucumber/features/uninstall.feature, tests/e2e-cucumber/tests/e2e.rs (the isolation/env-override harness), tests/e2e-cucumber/tests/e2e/uninstall_steps.rs in full, the corresponding production string formats in apps/rocm/src/storage.rs and apps/rocm/src/uninstall.rs/uninstall_plan.rs, and AGENTS.md in full. Diff reviewed: bcd879d007ba2db287688147637af2386e979616…e51a0770e500732ea328fe22f624683316206ad7.
An independent fan-out worker reviewed paths.rs/runtime.rs blind and reached the same conclusions (no defect in the override-resolution logic across all four override combinations; the same test-quality gap on cache_dir_keeps_its_default_without_a_data_override). A second worker, scoped to the e2e harness files, failed to spawn twice due to a concurrency limit in this environment; that pass was performed directly by me instead of independently, and is disclosed as such. History pass: paths.rs has only one prior commit (its original extraction), nothing relevant surfaced. Prior-changes pass (review comments on earlier changes to these files) did not run — no access to historical review threads. Could not execute cargo test/cargo nextest locally (no Rust toolchain available in this environment); the production-logic and test-quality findings above are argued from reading the code paths directly rather than from a run, and CI's own E2E tests job (which exercises uninstall-04) is read from the PR's check-run list and reports green. Not reconciled against any discussion on this PR: this review was conducted from the code, the PR description, and the linked Jira ticket (ROCMAI-826) only, per this method's blind-review rule. Note for transparency: an existing review from another reviewer was unavoidably visible in this review's mandatory pre-publish duplicate-check query before this review began; its text was not used to derive, word, or bias any finding here, though some overlap exists because both reviews independently check the same code against the same repository rules.
e51a077 to
265fddd
Compare
|
Pushed 265fddd, rebased onto main (the old scenario number
On orphaning the old home cache rather than migrating it: kept as is, since the cache holds only re-downloadable archives; the description says so. |
All six points are addressed in 265fddd and the updated description; see the replies and the summary comment on this PR. Dismissing so the PR is not blocked on a review that no longer reflects its head; a re-review is welcome.
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
I went through the diff, the new unit tests and the uninstall-05 scenario. I didn't find a defect in the fix itself: resolving the cache folder at the AppPaths layer, reusing the managed-root layout, and keeping ROCM_CLI_CACHE_DIR as the explicit override all look right, and the scenario matches the "cache follows the data dir" option in ROCMAI-826.
Requesting changes only because of what I can't yet clear:
- CI: "E2E tests (Strix Halo, Windows, current)" failed after about 15 minutes, and I couldn't get its log. uninstall-05 has no OS tags and so runs on Windows too, so please check whether that failure comes from the new scenario before this merges. The two Strix Halo Ubuntu jobs hit the 45m timeout, which looks like infrastructure. The W7900 and MI300X jobs were still pending when I looked.
- Behavior change worth a note: users who set only
ROCM_CLI_DATA_DIRnow get a fresh, empty<data>/cache, and their old cache is orphaned. The PR body mentions this, but the README and release notes don't. A line in the release notes would save some confusion.
Two smaller things are in inline comments (a vacuous assertion in the scenario step, and positional Option<PathBuf> arguments).
| ); | ||
| // Whatever cache it did remove is the isolated install's own. | ||
| let expected = format!("removed cache {}", data_cache(world).display()); | ||
| for line in stdout.lines().filter(|l| l.starts_with("removed cache ")) { |
There was a problem hiding this comment.
If no removed cache line is printed, this loop runs zero times and the check passes. Consider also asserting that exactly one such line exists, so the check can't pass without anything having been removed.
There was a problem hiding this comment.
You were right, and it was worse than a loose loop: when I required exactly one removed cache line the scenario failed on Windows, because it never created the data folder's cache, so the uninstall printed "cache path not present" and the old loop saw zero lines and passed. The step now requires exactly one removed cache <data>/cache line, and a new Given plants a file in that cache and a new Then asserts the folder is gone. With the or_else(managed_runtime_cache_dir) line removed, uninstall-05 goes red at the storage-report step (so the new removal steps are not reached in that red run), and it passes with the fix. Pushed as 4e91f44.
| /// | ||
| /// Takes the overrides as arguments so tests can drive it without | ||
| /// mutating the process environment. | ||
| fn from_overrides( |
There was a problem hiding this comment.
Minor: three positional Option<PathBuf> parameters (config/data/cache) compile fine if transposed, and the tests would still read plausibly. A small struct or named setters would make a mix-up a compile error. Related nit: managed_runtime_cache_dir is now also used for the plain data-dir case, so the name is slightly misleading.
There was a problem hiding this comment.
Done: from_overrides now takes a private PathOverrides { config, data, cache } struct, and discover and the unit tests use named fields. The fields are not named *_dir because clippy::struct_field_names rejects a shared postfix under -D warnings. I left managed_runtime_cache_dir as it is: it is the canonical <data>/cache layout function that the managed-root path also uses.
jussielo-amd
left a comment
jussielo-amd
left a comment
There was a problem hiding this comment.
Review of 265fddd: needs work (one red check, plus a few non-blocking points)
The fix itself looks right to me. from_overrides makes the cache follow ROCM_CLI_DATA_DIR to <data>/cache, the same place it sits by default and where with_managed_root already puts it, and ROCM_CLI_CACHE_DIR still wins. I read the diff completely, ran the new paths:: tests and the full rocm-core suite on the PR head (942 passed), and mutated each branch of the change one at a time to see which tests notice.
Blocking
E2E tests (Strix Halo, Windows, current)is red on this head. The job's own reconciliation reports one unexpected failure,runtime-resolve-canonical-release(runtime-09). The preview printed a runner-local pip mirror ascanonical_sourceandindex_url, where the step assertshttps://repo.amd.com/rocm/whl-multi-arch. I see nothing tying that to this change, anduninstall-05passed in the same log with all four of its steps, but I can't show it is red onmaintoo, because everyE2E self-hostedrun onmainis still queued. Please rerun the job, or have a maintainer confirm the mirror is the cause. The MI300X and W7900 jobs are still queued and the two Strix Ubuntu jobs were cancelled, so they have not covered this head either. The requiredCI / E2E testsjob, which carriesuninstall-05on the mock lane, is green, as are clippy and the Windows debug build.
Non-blocking
-
discover()'s wiring of the "data folder was overridden" flag is untested, but the PR text says it is pinned. The comment ina_managed_root_does_not_move_a_cache_that_followed_the_data_dirsays it is "the pathdiscover()takes", and the PR body says it covers "thediscover()path". The test callsdiscover_from_paths(paths, true, false)with hand-supplied flags. When I changeddiscover()to passfalsefor the data-override flag, all ofpaths::stayed green (11 passed) and so did the wholerocm-coresuite. With that mutation,ROCM_CLI_DATA_DIRplus asetup.therock_venvconfig would re-root the data folder and the cache with it, which is exactly what the PR says cannot happen. The other two mutations were caught: dropping the newor_else(managed_runtime_cache_dir)failscache_dir_follows_an_overridden_data_dirand the managed-root test, and dropping the!data_dir_overriddenguard fails the managed-root test andapp_paths_apply_configured_managed_root_when_unoverridden. Either givediscover()and the test one shared seam that takes the three overrides, or reword the comment and PR body to say the test coversdiscover_from_paths. -
--keep-cacheno longer protects the cache on an install with onlyROCM_CLI_DATA_DIRset, and the README scope phrase is now too narrow. With this change<data>/cacheis the cache for those installs.build_uninstall_planlists data and cache as separate actions, andremove_pathrunsfs::remove_dir_allon a real directory, sorocm uninstall --keep-cachewithout--keep-dataremoves the cache along with the data folder, while the plan still prints "cache removal disabled by command line flag" under "Left alone". Default installs already behave this way, since~/.rocmcontains~/.rocm/cache, and the README says so with "on a default installation a flag doesn't protect its files by itself". After this PR a second class of installs behaves the same way, so that phrase is incomplete. I traced this from the code and did not run it. Suggest widening the README sentence to say when a folder sits inside another, as the cache does by default and whenever onlyROCM_CLI_DATA_DIRis set. The "Left alone" line being false for nested folders is pre-existing and could be its own ticket. -
The PR body describes an earlier revision of one test. "It no longer returns early on a host without a default cache folder" has no referent in a one-commit diff, since
cache_dir_keeps_its_default_without_a_data_overrideis new here and uses.expect(...). Worth dropping the clause so the body matches the final diff.
Decisions for the author
- The old cache is orphaned silently. A user with only
ROCM_CLI_DATA_DIRset gets a new empty<data>/cache, and~/.rocm/cacheis no longer reported byrocm storageor removed byrocm uninstallfor that install, with no notice. The PR body discloses this and notes the cache only holds re-downloadable archives. The alternative is a one-time notice or a migration, at the cost of extra code for a low-value cache. Please confirm "document it, don't migrate" is the intent. - The cache now lives wherever
ROCM_CLI_DATA_DIRpoints. Anyone who put the data folder on a small volume will now accumulate large archive caches there.ROCM_CLI_CACHE_DIRstill wins and the README now says so.
What I liked
from_overridestakes the three overrides as arguments, so the unit tests need noset_var.- The managed-root test asserts its config fixture really is one
configured_managed_root_from_configreads, so it cannot pass vacuously. uninstall-05asserts both the message and the resulting state: the storage row must end with the data folder'scache, the uninstall must not name the home cache, and the planted file must survive. By reading the code, reverting the fix makes the storage assertion fail. I did not run the scenario, because that needs a fullrocmbuild.- The step helper sets
HOMEandUSERPROFILEand usesenv_removerather than an empty value, so a regression deletes only the scenario's own home.
Not covered
I did not run the uninstall-05 scenario itself, the cargo nextest and manual-Windows claims in the PR body, the full workspace tests, clippy or smoke_local.py; I relied on CI for those. I also did not retrieve ROCMAI-826. A merge-tree check against current main shows no conflicts and no scenario-number collision.
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🤖 Automated review (Claude Code) · 265fddd
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Review — no blocking findings
Full review of the whole change.
Implements ROCMAI-826 exactly as scoped: the cache directory now follows ROCM_CLI_DATA_DIR to <data>/cache unless ROCM_CLI_CACHE_DIR overrides it, matching the ticket's own stated remedy verbatim. Ticket status is "In Progress"; the PR does not claim to close it.
Blocking
None
Non-blocking
None
Decisions for the author
- Orphaning the old home-directory cache instead of migrating it — tradeoff
Traced independently fromfrom_overrides/default_cache_dir: a user who set onlyROCM_CLI_DATA_DIRnow gets a fresh, empty<data>/cache, while the old~/.rocm/cacheis left on disk, no longer reported bystorageor reachable byuninstallfor that install. The PR body already discloses this and argues it's acceptable because the cache only holds re-downloadable archives. Reasonable call; confirm it's still the intended tradeoff versus a one-time migration or a storage-report note about the orphaned folder.
Positive signals
from_overridesis a clean extraction: taking the three directory overrides as arguments (rather than reading the environment inside the function) is what lets the new unit tests drive precedence logic directly, without mutating process-global env vars.- The new e2e scenario's step assertions are tied to the real production string formats rather than re-implemented guesses — verified
storage_names_data_cache's expected(<path>)suffix againststorage.rs::render_report's actual" - {label}: {size} ({path})"line format, and it matches exactly. cache_dir_keeps_its_default_without_a_data_overridefails loudly (.expect(...), a panic) rather than silently short-circuiting to a vacuous pass on a host wheredefault_cache_dir()resolves toNone— good test hygiene for this class of guard.- The PR body documents an explicit per-test fail-without-fix/pass-with-fix table and states the orphaned-cache behavior change with its own reasoning, rather than leaving either to be inferred.
Deployment notes
- Confirmed behavior change, consistent with what the PR states: any existing install with only
ROCM_CLI_DATA_DIRset (noROCM_CLI_CACHE_DIR) will, after upgrading, resolve its cache to a fresh<data>/cacherather than the previous~/.rocm/cache; the old location is left on disk and is no longer reachable byrocm uninstallorrocm storagefor that install. - CI at commit
265fddd2e74e4eb39d63b22ff04ccf0d34ed272f: the requiredE2E testsjob (mock lane, which carries the newuninstall-05scenario) is green, as are the Linux build/test, Windows debug/release builds, clippy, and CodeQL jobs.E2E tests (Strix Halo, Windows, current)is red — read directly from its job log: all 7 steps ofuninstall-05passed cleanly in that run; the one failure is a different, pre-existing scenario (runtime-09, "Previewing a release SDK install resolves the canonical aggregate for this GPU") asserting a canonicalrepo.amd.compackage-index URL where the run observed an internal mirror hostname — unrelated to this diff. Two Strix Halo Ubuntu lanes showcancelledand the W7900/MI300X hardware lanes were stillqueuedat review time, so hardware-lane coverage for this change was incomplete when reviewed. - GitHub reports
mergeable_state: blockedfor this PR independent of the above.
What this covered
Diff reviewed: 6eeec620c3d451a52bd8f98ef218fb9e8a0549a4...265fddd2e74e4eb39d63b22ff04ccf0d34ed272f (the PR's one commit, 5 files, +244/-17). Read completely: crates/rocm-core/src/paths.rs (full file + diff), crates/rocm-core/src/runtime.rs (full file, ~1823 lines, for every helper paths.rs calls into), README.md (diff), tests/e2e-cucumber/features/uninstall.feature (full + diff), tests/e2e-cucumber/tests/e2e.rs (diff plus the isolation/env-override harness it extends), tests/e2e-cucumber/tests/e2e/uninstall_steps.rs (full + diff), the relevant sections of apps/rocm/src/storage.rs (to verify step-assertion strings against production output), and AGENTS.md (full). Impact analysis: grepped every call site touching cache_dir/default_cache_dir/managed_runtime_cache_dir repo-wide; every production consumer reads AppPaths.cache_dir through the single discover() path this change fixes, so no other call site needs updating. CI read directly for this exact commit via check-runs/status, and the one failing job's log fetched and read in full rather than inferred. Ticket ROCMAI-826 retrieved directly by key; its description matches this change's scope exactly.
Fan-out: three worker groups by coupling (production logic + its unit tests + README; e2e test files + the production string formats they assert against; agent-instruction/history/prior-changes). Two ran as independent sub-agents and returned no findings ≥70 confidence. The e2e-test-group worker failed to spawn twice under a concurrency limit shared across this account's active sessions; per the fallback for that case, I performed that pass myself directly rather than spawning a third time, so it was not independent of the driver — disclosed here rather than silently absorbed.
Disclosure: this task's mandatory duplicate-review checks (run both before this review began and again immediately before publishing, to avoid double-posting against a stale commit) return full review bodies rather than metadata only. They exposed four prior reviews from earlier commits on this PR, plus — on the final pre-publish check — a fifth review from a different human reviewer submitted at this exact commit while this review was already in progress. That content was read as an unavoidable side effect of the required checks; none of it was used to derive, word, or bias any finding in this report, and it was not reconciled against this report's conclusions. Could not run cargo test/clippy locally (no Rust toolchain in this environment); the conclusions above are argued from tracing the code directly, cross-checked against the exact strings storage.rs emits and against CI's own completed runs for this commit, rather than from a local run.
🤖 Generated with Claude Code
With ROCM_CLI_CONFIG_DIR and ROCM_CLI_DATA_DIR set but no ROCM_CLI_CACHE_DIR, AppPaths::discover fell back to the default ~/.rocm/cache. `rocm storage` then reported the default install's cache, and `rocm uninstall` of the isolated install listed it and deleted it. The cache folder now follows ROCM_CLI_DATA_DIR to <data>/cache, the place it sits by default (~/.rocm/cache under ~/.rocm) and where a setup.therock_venv managed root already puts it. ROCM_CLI_CACHE_DIR still wins when set. A managed root is applied only when ROCM_CLI_DATA_DIR is not set, so it never re-roots a cache that followed the data folder. Covered by unit tests on the override resolution and on its interaction with a managed root, and by uninstall-05 (@id:uninstall-moved-data-folder-keeps-its-own-cache), which runs storage and uninstall without ROCM_CLI_CACHE_DIR, with a file in the data folder's cache, and checks that cache is removed (exactly one `removed cache` line) while the default install's cache is neither named nor removed. The README notes that a cache an earlier version kept at ~/.rocm/cache with only ROCM_CLI_DATA_DIR set is no longer used or removed. Signed-off-by: Eugene Volen <eugene.volen@amd.com>
265fddd to
4e91f44
Compare
|
Pushed 4e91f44 (rebased onto main). On the review:
|
The CI question is answered (the failing Windows scenarios are unrelated; details in the description) and the two inline points and the behaviour note are done in 4e91f44; see the replies and the summary comment. A re-review is welcome.
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Reviewed at 4e91f44. No blocking findings; the change does what ROCMAI-826 asks. The cache directory follows ROCM_CLI_DATA_DIR unless ROCM_CLI_CACHE_DIR is set, and uninstall leaves the default ~/.rocm/cache alone. The uninstall-05 scenario asserts that outcome (exactly one removed cache <data>/cache line, home cache untouched), and the unit tests cover the override precedence. CI checks that had completed were green when I looked; E2E and a few build jobs were still pending.
Non-blocking:
- README: the new text splits the inline
rocm uninstallcode span across two lines and sits mid-paragraph in front of the original sentence tail. Reflowing it, with the orphaned-cache note as its own sentence, would read better. discover()passing the data/cache override flags intodiscover_from_pathsis covered only by the e2e scenario; the unit test hard-codestrue, false. Acceptable, just noting the gap.- The ticket's exact command,
rocm uninstall --dry-run, is not exercised; the scenario uses--yes --keep-binaries. Same code path, so low risk.
For awareness: users who set only ROCM_CLI_DATA_DIR get a fresh empty cache under <data>/cache and their old ~/.rocm/cache is orphaned. That is documented in the README and the PR body. The uninstall-05 id also collides with the open protected-roots branch, which the PR body already flags.
| when only `ROCM_CLI_DATA_DIR` is set, the cache directory moves with it, to | ||
| `<data directory>/cache`. A cache that an earlier version kept at `~/.rocm/cache` | ||
| while only `ROCM_CLI_DATA_DIR` was set is no longer used or removed by `rocm | ||
| uninstall`; delete it by hand if you don't need it. `uninstall` removes a category's directory |
There was a problem hiding this comment.
The inline code span rocm uninstall is split across lines 1294-1295, and the original sentence tail now starts mid-line after the added text. Consider reflowing and putting the orphaned-cache note in its own sentence.
jussielo-amd
left a comment
jussielo-amd
left a comment
There was a problem hiding this comment.
Thanks Volen, this is a clean fix. I read the whole change and checked the PR's claims against the code (by reading; I did not run tests).
Checked and holds:
- The cache resolves as
ROCM_CLI_CACHE_DIR, else<data>/cache, else the default. A configured managed root cannot move it whenROCM_CLI_DATA_DIRis set (thediscover_from_pathsguard). - With the
or_else(managed_runtime_cache_dir)line reverted, the two unit tests anduninstall-05go red.uninstall-05fails at the storage-report step, as you describe. - The orphaned
~/.rocm/cachenote in the PR body and README is accurate. Hosted CI is green on this head; the self-hosted GPU lanes have not reported yet.
Non-blocking:
- README (~1289-1297): please reflow so
rocm uninstallisn't split across lines and the original sentence stays together. "On a default installation a flag doesn't protect its files" is now also true when onlyROCM_CLI_DATA_DIRis set. The cache sits inside data, so--keep-cachealone no longer keeps it. Suggest "when the cache sits inside the data folder, as by default or with onlyROCM_CLI_DATA_DIRset". paths.rs:a_managed_root_does_not_move_a_cache_that_followed_the_data_dirnever removes its temp root (siblings callfs::remove_dir_all(root).ok()). The older test near line 405 buildscache_dirasroot/cache, which is no longer whatdiscover()returns for a data override. Considerdata.join("cache")there.uninstall_steps.rs:data_cache_gonecannot fail on its own, because data is removed first and the cache is nested in it. The raw line break in its assert message could be\n. The "exactly oneremoved cacheline" check also relies on the plan printing a nested-cache line after data is already gone; a one-line comment on that coupling would help.- The managed-root test hand-feeds
(true, false)todiscover_from_paths, so a wrong flag insidediscover()itself would stay green (only the e2e scenario covers that wiring). The test comment and PR body say it pins "thediscover()path", which is slightly stronger than what it does. - PR body: "it no longer returns early on a host without a default cache folder" refers to an earlier revision; the test is new in this diff.
docs/testing.md(~876): theuninstall-NNdescription could nameuninstall-05.
FYI, the Strix Halo Windows lane showed runtime-09 failing on the earlier head. That same single scenario is red on other open PRs today, so it looks unrelated to this change.
🤖 by agenthub on AMD AgentHub
Problem
With
ROCM_CLI_CONFIG_DIRandROCM_CLI_DATA_DIRset but noROCM_CLI_CACHE_DIR,AppPaths::discoverfell back to the default~/.rocm/cache.rocm storagethen reported the default install's cache, androcm uninstallof the isolated install listed it and deleted it. Found while testing the v0.2.1 release on Windows from an isolated config/data folder.Change
The cache folder now follows
ROCM_CLI_DATA_DIRto<data>/cache: where it sits by default (~/.rocm/cacheunder~/.rocm) and where asetup.therock_venvmanaged root already puts it.ROCM_CLI_CACHE_DIRstill wins when set. The installers only readROCM_CLI_CONFIG_DIRand are unaffected. The README's uninstall section now names the three variables and says where the cache goes.A
setup.therock_venvmanaged root does not re-root this cache:discover_from_pathsapplies a managed root only whenROCM_CLI_DATA_DIRis not set, so a cache derived from the data folder is never moved.a_managed_root_does_not_move_a_cache_that_followed_the_data_dirpins this.Behaviour change
(The README's uninstall section carries a line about this; the repo has no changelog or release-notes file, so this PR text is where it is recorded.)
A user who set only
ROCM_CLI_DATA_DIR(noROCM_CLI_CACHE_DIR) now gets a new, empty<data>/cache. Their previous cache at~/.rocm/cacheis orphaned from that install: archives it held are downloaded again into the new folder, androcm uninstallof that install no longer removes~/.rocm/cache. The cache holds only re-downloadable archives, so nothing is lost; the old folder can be deleted by hand.Docs
README (uninstall section) is updated.
docs/testing.mdanddocs/manual-testing.mdset all three variables explicitly, which stays correct, and neither they nor the--helptext forstorageanduninstall --keep-cachestate where the cache lives, so they need no change.Tests
crates/rocm-core/src/paths.rs(from_overridestakes a small namedPathOverridesstruct rather than three positionalOption<PathBuf>arguments):cache_dir_follows_an_overridden_data_dir: the cache follows the data folder; an explicit cache override still wins.a_managed_root_does_not_move_a_cache_that_followed_the_data_dir: thediscover()path with a data override and a managed root in the config.cache_dir_keeps_its_default_without_a_data_override: moving only the config folder leaves the default cache. It no longer returns early on a host without a default cache folder.uninstall-05,@id:uninstall-moved-data-folder-keeps-its-own-cache, intests/e2e-cucumber/features/uninstall.feature. It has no OS, GPU or link tags, so it runs on the mock lane in the requiredE2E testsjob on every PR, and passes there. It plants a file in the default home cache and one in the data folder's own cache, runsstorage reportanduninstall --yes --keep-binarieswith config and data moved but no cache override, and asserts that the uninstall prints exactly oneremoved cache <data>/cacheline, that the data folder's cache is gone, and that the home cache is neither named nor removed. (An earlier version looped over whateverremoved cachelines were printed, so it passed when none were: the data folder's cache did not exist, so the uninstall printed "cache path not present". It is tightened and now plants that cache.)The tests fail without the fix. With the
or_else(managed_runtime_cache_dir)line removed fromfrom_overrides:cache_dir_follows_an_overridden_data_dira_managed_root_does_not_move_a_cache_that_followed_the_data_dircache_dir_keeps_its_default_without_a_data_overrideuninstall-05cargo nextest run -p rocm-core(886 passed), clippy forrocm-coreande2e-cucumber(including--test e2e),cargo fmt --check: pass. Manual check on Windows with the built binary: uninstall removed only the isolated data folder; the planted home-cache file survived.Note for the protected-roots uninstall work
An open branch on uninstall protected roots reports the cache's source as
DefaultwheneverROCM_CLI_CACHE_DIRis unset. After this change, a cache that followedROCM_CLI_DATA_DIRshould be reported as coming from the data override instead. That branch also uses scenario numberuninstall-05, so one of the two will need renumbering (uninstall-04is taken by #823 on main, so this PR usesuninstall-05).CI on the Windows lane
On the previous head
E2E tests (Strix Halo, Windows, current)failed 4 of 174 scenarios:serve-01andserve-02(no "resolved model" line),runtime-02(noFolder:line in the examine output) andruntime-09(the SDK install preview declared a mirror source rather than the canonical release aggregate).uninstall-05passed there, and none of the four exercises the cache folder. I could not compare against a green run on main, because the other Windows runs were skipped or still running. The twoStrix Halo, Ubuntulanes hit the 45-minute cap, which #852 addresses.