Skip to content

ci(e2e): colocate the uv cache so uv hardlinks instead of copying - #352

Open
tomastola wants to merge 2 commits into
mainfrom
ci/colocate-uv-cache-for-hardlinks
Open

ci(e2e): colocate the uv cache so uv hardlinks instead of copying#352
tomastola wants to merge 2 commits into
mainfrom
ci/colocate-uv-cache-for-hardlinks

Conversation

@tomastola

@tomastola tomastola commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

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_DIR pointed 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:

stat -c '%d %n'  →  both paths report the SAME st_dev, and df shows the same device
ln <cache>/probe <work>/probe  →  Invalid cross-device link
ln within the work volume      →  succeeds

Linux refuses a hardlink across two mounts even when both resolve to one underlying filesystem, so a bind mount or a subPath volume is enough to trigger the fallback. The repo's existing prose — in uv.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 existing e2e-shared / e2e-target / e2e-prewarm caches.

Also corrects the doc comments in the harness and rocm-core that documented the old placement as deliberate, and three see ci.yml e2e-gpu pointers left dangling when those lanes moved to e2e-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-gpu runners, 3.3TiB on r9700. 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:

  • The EXDEV-across-mounts / success-within-mount results above.
  • $RUNNER_WORKSPACE is set and already used by sibling exports in all four affected shell blocks; every one is a self-hosted, linux job with no container:.
  • The harness creates the directory (validated_shared_dircreate_dir_all), so no mkdir step is needed, and the new value passes its absolute/no-.. validation.
  • No cleanup step can reach the new path: every rm -rf in both workflows is scoped to /tmp/rocm-e2e-*, and git clean -ffdx only touches the checkout, not its parent.
  • cargo check -p rocm-core and cargo check --tests -p e2e-cucumber pass.

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-prewarm found the shared runtime already up to date and took the reuse path:

pre-warm: reusing the shared release runtime (runtime is up to date with the channel index)

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_DIR this PR sets:

$ uv pip install numpy    # UV_CACHE_DIR=$RUNNER_WORKSPACE/uv-cache, venv on the work volume
probe: libscipy_openblas64_-f48b354e.so
inode=103416195  nlink=2
HARDLINKED <- .../uv-cache/archive-v0/tB9Gu8OYoAaCi3gh/numpy.libs/libscipy_openblas64_-f48b354e.so

nlink=2 and the installed file's inode resolves inside archive-v0 — uv hardlinks at the new location. The same probe against the old path returns EXDEV and 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.

@r0x0r r0x0r left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 set E2E_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:51 and apps/rocm/src/therock.rs:4713 still use the old "different filesystem" phrasing this PR corrects, and nightly.yml:335 still points at the "per-PR ci.yml e2e-gpu" job (now in e2e-selfhosted.yml).
  • Please confirm the now-unused runner-side overlay at the old /var/tmp/rocm-e2e-uv-cache path gets dropped, and that losing the cache's dedicated quota is acceptable given the shared work-volume budget.

None of these block; approving.

@tomastola
tomastola force-pushed the ci/colocate-uv-cache-for-hardlinks branch from f451d2d to 2be3771 Compare September 9, 2026 14:53
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>
@tomastola
tomastola force-pushed the ci/colocate-uv-cache-for-hardlinks branch from 2be3771 to 3cf45b2 Compare September 9, 2026 16:03

@siloteemu siloteemu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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, :528nothing 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-cache and 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 setting E2E_SHARED_UV_CACHE_DIR derives it from $RUNNER_WORKSPACE, and/or a one-line runtime guard in the run block before the pre-warm: compare stat -c %m "$E2E_SHARED_UV_CACHE_DIR" against stat -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:303 and MANIFEST.md:672the sweep is not finished. The doc comment on managed_uv_cache_dir — the function that implements the colocation — still reads "kept under the managed root so it shares a filesystem with the environments uv populates and hardlinking keeps working", i.e. verbatim the claim this PR corrects two files away in crates/rocm-core/src/uv.rs:150. MANIFEST.md:672 ("so that it shares a filesystem with the environments uv populates") has the same wording and is hand-maintained, not generated by xtask/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-274the 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 both git clean and 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 takes e2e-target, e2e-shared and e2e-prewarm with 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 — a df threshold check plus rm -rf of 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-cache by 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 a TempDir under /tmp, which was the same / overlay as the old /var/tmp cache; 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 --prefix non-goal on the PR that introduced the colocation" is unresolvable for a reader; cite the number, as crates/rocm-core/src/runtime.rs:304 does with issue #160.
  • apps/rocm/src/therock.rs:5744-5747 — pre-existing, but assert!(managed_uv_cache_dir(&paths.data_dir).starts_with(&paths.data_dir)) restates the implementation (a join on the same root) rather than the mount property the reworded comment now claims; it would pass under any placement bug this PR cares about.

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.

3 participants