Repository navigation
ci(e2e): colocate the uv cache so uv hardlinks instead of copying - #352
Conversation
r0x0r
left a comment
There was a problem hiding this comment.
LGTM.
Nice catch on the EXDEV-across-mounts fallback — colocating the uv cache on the same mount as the runtime trees it populates is the correct minimal fix, and it's consistent with the sibling $RUNNER_WORKSPACE caches already used in those shell blocks. Thanks for verifying the hardlink directly on a runner (nlink=2, installed inode resolving inside uv-cache/archive-v0, EXDEV at the old path) rather than inferring it. The harness already create_dir_alls and validates the path, so this is low-risk, and the doc-comment corrections ("mount, not filesystem") plus the stale ci.yml e2e-gpu pointer repairs are welcome cleanup.
A few non-blocking notes:
- CI:
E2E tests (Strix Halo, Ubuntu)shows cancelled (runner reclaim/timeout, no assertion failure) rather than a real failure — and that lane doesn't setE2E_SHARED_UV_CACHE_DIR, so it's unrelated to this change. Please re-run it so the PR goes fully green. - A copy-fallback regression would only surface as a slower-but-green run, so the inode-assertion-after-prewarm guard you mentioned would be genuinely valuable as a fast follow-up.
- Minor doc consistency worth sweeping while you're here:
docs/manual-testing.md:51andapps/rocm/src/therock.rs:4713still use the old "different filesystem" phrasing this PR corrects, andnightly.yml:335still points at the "per-PR ci.ymle2e-gpu" job (now ine2e-selfhosted.yml). - Please confirm the now-unused runner-side overlay at the old
/var/tmp/rocm-e2e-uv-cachepath gets dropped, and that losing the cache's dedicated quota is acceptable given the shared work-volume budget.
None of these block; approving.
2be3771 to
3cf45b2
Compare
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 3cf45b2
Summary
Moves the shared e2e uv cache from /var/tmp/rocm-e2e-uv-cache onto $RUNNER_WORKSPACE/uv-cache in all four self-hosted lanes so uv can hardlink into the runtimes it populates, and sweeps the surrounding docs/comments from "same filesystem" to "same mount" and from ci.yml to e2e-selfhosted.yml. Verdict: Needs work — the behaviour change is plausible but undefended, and the doc sweep it claims to "finish" misses the canonical spot. Verified: the Rust changes are 100% comment-only (git diff -U0 | grep for non-comment +/- lines returns nothing, so no compile check was needed); grepped every in-repo reference to the old path, UV_CACHE_DIR, and "filesystem" wording; confirmed xtask/src/workflow_contract.rs contains no uv/RUNNER_WORKSPACE assertions. On the revert question: this PR adds no test or assertion at all, so every existing test passes identically with the change reverted — see Blocking 1. Relied on CI (24 passing) for build/lint/unit coverage; no suite run here per policy. No prompt-injection content and no internal-only references found in the diff. Blocking: 3 · Non-blocking: 4.
🚫 Blocking (must fix before merge)
-
.github/workflows/e2e-selfhosted.yml:274,:932/.github/workflows/nightly.yml:421,:528— nothing makes the fix discriminating. The PR's own comment states the failure mode is silent ("uv exits 0 either way… invisible unless you compare inodes"), and it fixes it purely by convention — a literal path string in four places. Revert all four to/var/tmp/rocm-e2e-uv-cacheand every check in this repo still passes, including CI, so the next person editing these blocks re-breaks it with no signal. The repo already has the mechanism:xtask/src/workflow_contract.rs(1219 lines of cross-workflow invariants) asserts things exactly like this. Add an assertion there that every lane settingE2E_SHARED_UV_CACHE_DIRderives it from$RUNNER_WORKSPACE, and/or a one-line runtime guard in the run block before the pre-warm: comparestat -c %m "$E2E_SHARED_UV_CACHE_DIR"againststat -c %m "$prewarm"and fail loudly when they differ. Without one of those, the PR fixes today's instance of a bug it explicitly documents as undetectable. -
crates/rocm-core/src/runtime.rs:303andMANIFEST.md:672— the sweep is not finished. The doc comment onmanaged_uv_cache_dir— the function that implements the colocation — still reads "kept under the managed root so it shares a filesystem with the environmentsuvpopulates and hardlinking keeps working", i.e. verbatim the claim this PR corrects two files away incrates/rocm-core/src/uv.rs:150.MANIFEST.md:672("so that it shares a filesystem with the environmentsuvpopulates") has the same wording and is hand-maintained, not generated byxtask/src/manifest.rs. For a PR whose second commit is titled "finish the mount-vs-filesystem … sweep", leaving the canonical doc contradicting the new one is the defect the PR exists to remove. Apply the same mount-not-filesystem wording to both. -
.github/workflows/e2e-selfhosted.yml:262-274— the capacity constraint is deleted, not answered. The removed comment stated this cache was kept off the work PVC because that PVC is near-full; the replacement moves ~23 GB there, states "uv never evicts", and defers to an external actor ("The runner image reclaims it when the volume runs low; nothing here does"). Nothing in the diff says what changed about the volume, and the previous text specifically noted this cache sat outside bothgit cleanand the reclaim step's/tmp/rocm-e2e-*glob — the reclaim steps at lines 195/405/744/882 still only glob/tmp/rocm-e2e-*, so nothing in this repo will ever trim$RUNNER_WORKSPACE/uv-cache. If the volume does fill, it takese2e-target,e2e-sharedande2e-prewarmwith it and every self-hosted lane fails at once. Either state the evidence in the PR (current volume size/free space, what the runner image's reclaim actually covers) or add an in-repo guard — adfthreshold check plusrm -rfof the uv cache before the pre-warm is a few lines and keeps the invariant where the reader can see it.
Non-blocking
.github/workflows/e2e-selfhosted.yml:932/nightly.yml:528— the replaced comments said a separate PVC is mounted at exactly/var/tmp/rocm-e2e-uv-cacheby the runner's cluster overlay; changing the path leaves that overlay mounting a volume nobody reads and ~23 GB stranded. The PR should call out the overlay change as a required external follow-up.tests/e2e-cucumber/tests/e2e.rs:193— clean-slate scenarios keep their data dir in aTempDirunder/tmp, which was the same/overlay as the old/var/tmpcache; after this move those installs cross a mount and copy. Worth confirming the pre-warm path is the only one whose hardlinking matters.apps/rocm/src/therock.rs:5756— "see the--prefixnon-goal on the PR that introduced the colocation" is unresolvable for a reader; cite the number, ascrates/rocm-core/src/runtime.rs:304does with issue #160.apps/rocm/src/therock.rs:5744-5747— pre-existing, butassert!(managed_uv_cache_dir(&paths.data_dir).starts_with(&paths.data_dir))restates the implementation (ajoinon the same root) rather than the mount property the reworded comment now claims; it would pass under any placement bug this PR cares about.
|
Addressed the three blocking items — and the second one turned up a real defect in this PR that the review's framing pointed at but did not name. Blocking 3 (capacity) — this was the important one, and the PR was wrong. The concern is correct, for a sharper reason than stated. Fixed in f65b2e4: derive the runner root and point all four exports at the GC-bounded directory. Two details in the review's premise are out of date, though they do not change the conclusion: there is no separate cache PVC any more (deleted in the 2026-09-07 migration), and the work volume is not near-full (295G, 200G free). The "~23 GB moves there" reading does not hold either — it is the same directory the subPath already exposes, not a new copy. Blocking 1 (nothing discriminating). Correct, and the mechanism suggested was the right one. Added Blocking 2 (sweep unfinished). Confirmed and fixed in 3c4c37b — Open dependency, not resolved here: the On the non-blocking note about stranding the overlay mount — it is the reverse. dev-tooling |
Superseded. Two of the three counts are resolved at f65b2e4 and are formally withdrawn here: the cross-workflow contract test now makes the colocation discriminating (reverting any one of the four lanes fails it), and the mount-vs-filesystem sweep covers the two spots this block named. On the third count the author is substantially right and this block was imprecise: it asserted the in-repo reclaim steps stopped covering the cache, when in fact they never covered it under either path, and it described a separate cache volume that no longer exists. A narrower change request follows at the current head covering the one thing still unresolved.
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · f65b2e4
This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Summary
Moves the shared E2E uv cache from a fixed literal path onto the runner work volume — derived as <runner-root>/uv-cache in all four self-hosted lanes — so uv hardlinks instead of copying, adds a cross-workflow contract test that enforces the derivation, and finishes a mount-vs-filesystem wording sweep across comments and docs. Outcome: Needs work — one blocking clarification; the mechanics of the fix are sound. Verified: read all four commit objects in full via git cat-file -p (1415/764/336/1047 bytes, all with Signed-off-by and an SSH gpgsig), read the complete git diff prw-base...HEAD, enumerated every E2E_SHARED_UV_CACHE_DIR occurrence repo-wide (exactly four setters, all converted, zero references to the old literal remaining), traced multiline_run_blocks/self_hosted_workflows to confirm all four export sites land inside blocks the new test inspects, and — the load-bearing one for the discrimination claim — confirmed in xtask/src/affected.rs (forces_full_workspace) that any change under .github/workflows/ forces a --workspace test selection in ci.yml, so the new contract test genuinely runs on a PR that touches only workflow YAML. Caveat: the targeted run this review intended (cargo test -p xtask <filter>, including a deliberate revert) could not be completed in this environment, so the discrimination conclusion rests on reading the assertion and the test-selection logic rather than on an observed failure. Cross-file claims about untouched files were re-checked against a clean base of the branch. CI at this head: 20 success, 1 failure, 2 pending, 3 skipped (no per-lane detail available to me; I attribute the failure to no named job). Blocking: 1 · Non-blocking: 5.
Prior objection status
- Count 1 (nothing makes the fix discriminating) — RESOLVED.
xtask/src/workflow_contract.rsaddsevery_shared_uv_cache_sits_inside_the_gc_bounded_directory, which assertsline.contains("$runner_root/uv-cache")for everyexport E2E_SHARED_UV_CACHE_DIR=in both self-hosted workflows plus the presence ofrunner_root="$(dirname "$(dirname "$RUNNER_WORKSPACE")")"in the same run block. Reverting any one of the four lanes to the old literal fails the first assertion, and a workflow-only change forces the full-workspace selection that runs it. The head commit's claim "reverting any one of the four exports now fails the suite, which it did not before" is accurate. - Count 2 (doc sweep unfinished) — RESOLVED as filed. Commit
3c4c37bfixes exactly the two spots named: themanaged_uv_cache_dirdoc comment incrates/rocm-core/src/runtime.rsand itsMANIFEST.mdcounterpart, both of which now say "reachable … without crossing a mount point" instead of "shares a filesystem". (The sweep is still incomplete elsewhere — a new, separate finding below, not a revival of this count.) - Count 3 (capacity constraint deleted, not answered) — PARTIALLY RESOLVED. The head commit does answer it in substance: the cache now lands inside the one directory the comments say is garbage-collected, and the rationale is written out rather than dropped. But the evidence offered is not checkable in-repo and is internally inconsistent — see the blocking item. There is still no in-repo guard: no
df/free-space preflight in any lane, and the reclaim steps glob only/tmp/rocm-e2e-*(they never covered the uv cache under either the old or the new path, so that specific sub-claim in the original objection was imprecise — the correct statement is that nothing in-repo reclaims it at all).
🚫 Blocking (must fix before merge)
.github/workflows/e2e-selfhosted.yml(new uv-cache comment block, ~lines 270-280) and the mirrored comment inxtask/src/workflow_contract.rs— the 60GiB figure and the 49G/49G anecdote cannot both be true as written, and that arithmetic is the entire justification for moving ~23GB onto this volume. The workflow comment says the GC "dropsarchive-v0below a 60GiB floor on the way up", while the head commit message says it sweepsarchive-v0"once free space drops under a 60GiB floor" — a size cap and a free-space floor are different mechanisms, and the two texts state different ones. Under the free-space reading, a 60GiB floor is unsatisfiable on the volume the same comment describes as having stranded at 49G/49G (total ≈49G); under the size-cap reading, a 60GiB cap on a ≈49G volume never binds and so protects nothing. Either way the numbers as presented do not support the conclusion they are offered for, and a reader cannot tell which mechanism is real. This blocks because it is precisely the evidence the standing objection asked for, and because the change's safety argument — including protection of the runner credentials the comment says live on the same volume — rests on it. Concrete fix: state the mechanism unambiguously and make the figures consistent (e.g. "keepsarchive-v0under N GiB" or "prunesarchive-v0when free space falls below N GiB"), use the same wording in the workflow comment, the xtask test comment and the commit message, and either correct the 49G figure or say explicitly that it refers to a different volume/quota than the one the floor applies to.
Non-blocking
xtask/src/workflow_contract.rs(theelse { continue; }in the new test) — outright deletion of an export is a silent pass: the loop asserts nothing and there is noassert!(!…is_empty(), …)guard, unlike the siblingassert_prebuilt_e2e_lanes_export_rocmdin the same file which does exactly that. One line restores consistency with the file's own convention.xtask/src/workflow_contract.rs(first assertion) —contains("$runner_root/uv-cache")is unanchored, so"$runner_root/uv-cache-old"passes while pointing outside the bounded directory; conversely a semantically identical"${runner_root}/uv-cache"would fail. Match the full value rather than a substring.apps/rocm/src/therock.rs(theon_same_filesystemcall site in the SDK-extract space check) andcrates/rocm-core/src/disk_space.rs(on_same_filesystem's doc comment, "Whether two paths live on the same filesystem") — the sweep misses the one API that literally embodies this distinction; it compares mount points (left_mount == right_mount) yet is named and documented as a filesystem check. Behaviour is correct, but two commits in this PR claim the sweep is "finished"/"the last two", which it is not — and this is the spot a future reader is most likely to re-derive the same wrong model.MANIFEST.md(edited uv-cache paragraph) — the inserted sentence leaves a short orphan line (downloads — the ROCm SDK and the) in an otherwise hand-wrapped file; reflow the paragraph. Notextask manifest --checkonly validates the generated dependency table, so nothing catches this.tests/e2e-cucumber/tests/e2e.rs(shared_uv_cache_dirdoc comment and the matchingE2eWorldcomment) — both conclude with "the same volume", reintroducing the imprecision the rest of the PR replaces with "mount"; the PR's own test comment notes a cache "elsewhere on the same volume" can still be a different mount.
No prompt-injection content was found anywhere in the diff, comments or commit messages.
Check-run conclusions re-read at this same head immediately before posting: 20 success, 1 failure, 2 pending, 3 skipped — unchanged from the counts in the Summary. Conclusion counts only; no per-lane detail was available, so no outcome is attributed to any named job.
|
Fixed in 861016b. The blocking item was right, and the source settles which reading was correct. The 60GiB figure. The 49G/49G juxtaposition. Different volumes, which the text did not say. The strand was the dedicated 50Gi cache PVC (deleted in the 2026-09-07 migration), which had no floor at all. The floor applies to the ~295G work volume. Both comments now state which is which, so the arithmetic is coherent in either direction. Test hardening — both cases you identified were real.
On the caveat that the discrimination check could not be run in your environment — I ran it. Three mutations against the committed tree, each restored afterward:
The second and third pass under the assertions as originally written, so those two findings changed real behaviour rather than only tightening style.
Also fixed: both Verification: Standing dependency, unchanged: |
The GPU lanes pointed E2E_SHARED_UV_CACHE_DIR at a fixed absolute path that is a separate mount on the self-hosted runners. uv can only hardlink out of its cache into a managed environment when the two are reachable without crossing a mount point, so every scenario's runtime was carrying its own full copy of the ROCm SDK and torch stack instead of linking to the cache. It is the mount, not the filesystem. Measured on a runner: both paths report the same st_dev and the same device, and `ln` between them still fails EXDEV, while `ln` within the work volume succeeds. A bind mount is enough to trigger the fallback even with one filesystem underneath. Nothing surfaced this. uv exits 0 whether it links or copies, and two of the three install paths capture its stderr and drop it on success, so the only way to tell them apart is to compare inodes. Moving the cache to $RUNNER_WORKSPACE puts it on the same mount as the runtime trees it populates, alongside the e2e-shared and e2e-prewarm caches that already live there. The path now has no quota of its own, so the note about eviction moves with it. Also corrects the harness and rocm-core doc comments that explained the old placement, and three "see ci.yml e2e-gpu" pointers left dangling when those lanes moved to e2e-selfhosted.yml. Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
The parent commit corrected the uv-cache placement rationale in uv.rs and the E2E harness but left the same claim standing in two other places, and repaired only some of the pointers left dangling when the self-hosted GPU lanes moved out of ci.yml. Sweep the rest: docs/manual-testing.md and the therock.rs `--prefix` tests said "different filesystem" when the constraint is the mount (Linux refuses a cross-mount hardlink even within one filesystem), and four `ci.yml` references now resolve to e2e-selfhosted.yml -- including the one on E2E_MERGE_QUEUE, which only that workflow sets. Comments and prose only; no executable line changes. Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
The sweep missed the canonical spot: the doc comment on managed_uv_cache_dir, the function that implements the colocation, plus its MANIFEST.md counterpart. Both still claimed a shared *filesystem* was what enables hardlinking. Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
The colocation moved the cache to \$RUNNER_WORKSPACE/uv-cache, which restores hardlinking but lands one level below the only directory the runner pod's uv-cache-gc initContainer bounds. RUNNER_WORKSPACE is <runner-root>/_work/<repo>, measured on a live MI300X job. The GC mounts the work PVC's `uv-cache` subPath and sweeps <runner-root>/uv-cache: abandoned .tmp* extractions, and archive-v0 once free space drops under a 60GiB floor. A cache one level deeper still hardlinks, so every signal stays green while nothing enforces the floor — on the volume that also holds .runner, whose credentials need a repo-Administration token to re-register. That is how the MI300X lane previously stranded at 49G/49G. Derive the runner root instead of hardcoding it, and assert the derivation in workflow_contract so the next lane cannot reintroduce it: reverting any one of the four exports now fails the suite, which it did not before. Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
clippy's too_long_first_doc_paragraph is denied workspace-wide, and the reworded comment ran the summary and the mount-vs-filesystem detail together into one four-line paragraph. Same content, summary sentence first. `cargo check` does not see this; `cargo clippy -D warnings` does. Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
The comment said the GC "drops archive-v0 below a 60GiB floor" while the commit message said it fires "once free space drops under" one. Those are different mechanisms, and the source settles it: uv-cache-gc reads column 4 of `df -Pk` (Available) and deletes archive-v0 outright when it is under 60GiB. A free-space floor, not a size cap. Say so identically in the workflow comment and the xtask test. The 49G/49G figure sat next to the 60GiB floor implying one volume, where it would be either unsatisfiable or inert. They are different volumes: the strand was the dedicated 50Gi cache PVC, since deleted; the floor applies to the ~295G work volume. Say which is which. Test hardening, both cases the assertions let through: - full-line equality instead of `contains`, which accepted `$runner_root/uv-cache-old` — outside the bounded directory - a per-workflow setter count, so deleting every export fails instead of passing an empty loop, matching assert_prebuilt_e2e_lanes_export_rocmd Also rename on_same_filesystem -> on_same_mount. It compares mount points and always did; the name taught the exact wrong model this PR exists to correct, in the one API that embodies it. Its space-check caller now notes the conservative direction: two mounts over one filesystem do share a pool. Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
main gained an MI350P lane in each self-hosted workflow while this branch was open, both carrying the old literal /var/tmp/rocm-e2e-uv-cache. The contract test added earlier in this branch caught them on rebase, which is the case it was written for: a new lane copied from an older one. Converted both to the derived <runner-root>/uv-cache. No occurrence of the old literal remains in either workflow. Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
861016b to
20aed97
Compare
|
🔴 Automated review · pr-review-watcher · 20aed97 This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer. SummaryMoves the shared uv cache in the self-hosted E2E lanes from a fixed literal path to a derived Prior objection statusBLOCKING — 60GiB figure vs the 49G/49G anecdote: RESOLVED. Fixed in Note on our own earlier phrasing: the objection asked for consistency "across the commit message" too. Commit
Author-reported mutation results: the ones that matter reproduce from the assertion code. Reverting an export to the old literal, to 🚫 Blocking (must fix before merge)None. Non-blocking
Check-run conclusions re-read at this same head immediately before posting: 21 success, 5 pending, zero failures — more lanes have reported since the counts quoted in the Summary, and there are still no failures. Conclusion counts only; no per-lane detail was available, so no outcome is attributed to any named job. |
Symptom
Every scenario's managed runtime on the GPU lanes carries its own full byte-for-byte copy of the ROCm SDK and torch stack, instead of hardlinking to the shared uv cache that already holds those exact files. Nothing reports this — the lanes are green and always have been.
Root cause
E2E_SHARED_UV_CACHE_DIRpointed at a fixed absolute path that is a separate mount on the self-hosted runners. uv can only hardlink out of its cache into an environment when the two are reachable without crossing a mount point, so it fell back to copying.The important detail is that it is the mount, not the filesystem. Measured on a runner:
Linux refuses a hardlink across two mounts even when both resolve to one underlying filesystem, so a bind mount or a
subPathvolume is enough to trigger the fallback. The repo's existing prose — inuv.rs, the harness,MANIFEST.md— all says "different filesystem", which is why this was easy to miss.It is also structurally silent: uv exits 0 whether it links or copies, and two of the three install paths (
run_command_with_env) capture stderr and drop it on the success path, so uv's own fallback warning never reaches a log. Comparing inodes is the only way to tell.Change
Point the four GPU-lane exports at
$RUNNER_WORKSPACE/uv-cache, the same volume as the runtime trees it populates and alongside the existinge2e-shared/e2e-target/e2e-prewarmcaches.Also corrects the doc comments in the harness and
rocm-corethat documented the old placement as deliberate, and threesee ci.yml e2e-gpupointers left dangling when those lanes moved toe2e-selfhosted.yml.Capacity
The old comment justified the separate location as keeping ~23GB off a "near-full" work volume. That is no longer true, and the cache now shares the work volume's budget rather than having a quota of its own — measured free space on the work volume before this change: 199GiB and 182GiB on the two
amd-gpurunners, 3.3TiB onr9700. The dedicated-quota arrangement is what made the cache run out of space in the first place, while the work volume sat at 33%.The old path is no longer referenced, so whatever mounts a volume there on the runner side can drop it.
Verification
Measured on hardware, not inferred:
EXDEV-across-mounts / success-within-mount results above.$RUNNER_WORKSPACEis set and already used by sibling exports in all four affected shell blocks; every one is aself-hosted, linuxjob with nocontainer:.validated_shared_dir→create_dir_all), so nomkdirstep is needed, and the new value passes its absolute/no-..validation.rm -rfin both workflows is scoped to/tmp/rocm-e2e-*, andgit clean -ffdxonly touches the checkout, not its parent.cargo check -p rocm-coreandcargo check --tests -p e2e-cucumberpass.Now verified — but not by CI, and that distinction matters.
Both GPU lanes pass on this branch, and that proves nothing about the hardlink, because neither lane exercised the new path.
xtask e2e-prewarmfound the shared runtime already up to date and took the reuse path:With no
install sdk, uv is never invoked, and the new cache directory was still empty (4.0K) after a green run. Worth knowing generally: a green GPU lane cannot confirm any install-time change while the pre-warm tree is warm.So I tested the mechanism directly on a runner instead — same volume, same uv binary, same
UV_CACHE_DIRthis PR sets:nlink=2and the installed file's inode resolves insidearchive-v0— uv hardlinks at the new location. The same probe against the old path returnsEXDEVand never links. Test artifacts were removed and the runner left as found.The ROCm SDK payoff therefore lands on the next cold install (a new ROCm generation, or a wiped pre-warm tree), not on this PR's run.
Worth noting there is no automated guard for this — a regression to copy-fallback would show up only as a slower green run. An inode assertion after the pre-warm would make it self-guarding; happy to add it here or as a follow-up if you'd prefer it in-tree.