ci(e2e): refresh the shared pre-warmed runtime when a newer one exists (EAI-8057) - #275
Open
tomastola wants to merge 1 commit into
Open
ci(e2e): refresh the shared pre-warmed runtime when a newer one exists (EAI-8057)#275tomastola wants to merge 1 commit into
tomastola wants to merge 1 commit into
Conversation
tomastola
marked this pull request as ready for review
August 19, 2026 12:59
The self-hosted E2E lanes guarded their pre-warm install on directory existence alone, so it never reinstalled. The tree lives on the runner's persistent workspace, which meant that after the first run ever every lane served against whatever runtime happened to be installed that day — 16 days old on both MI300X runners when measured. Drift between the shared tree and what a fresh install produces was untested and widened silently. Keep the cache, but invalidate it when the channel index has actually published something newer, reusing what the CLI already ships rather than reimplementing version resolution in workflow shell: `rocm update` reports per-runtime freshness, `rocm update --apply --activate` installs the newer runtime side-by-side, and `rocm storage remove-old-installs` bounds the result. Side-by-side matters — `install sdk` bakes absolute paths into the runtime manifest, so a runtime must be created in place and never moved. Anything that cannot be read as "a newer version exists for our channel" reuses the existing tree: an unreachable index must not turn a lane red or trigger a multi-GiB download on every run. The decision lives in `xtask e2e-prewarm` because the pre-warm block is duplicated across eight jobs in two shells; inline logic would exist eight times in two languages and be untestable. The Strix Windows lanes lose their PowerShell copy rather than gaining a second implementation. Unit tests cover each freshness verdict, including the degraded paths and the `update_surfaces` summary line that sits one character away from a real runtime entry. Because a fixture can drift from the renderer it imitates, the `runtime-update-reports-freshness` scenario runs the real command on hardware and pins the output shape the parser depends on. Refs EAI-8057 Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
tomastola
force-pushed
the
ci/e2e-prewarm-invalidation
branch
from
August 20, 2026 15:14
bb90bf5 to
6311cc7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Nearly every GPU E2E scenario serves against one shared, pre-warmed managed
runtime, so a multi-GiB
rocm install sdkhappens once per runner instead ofonce per scenario. That tree lives on the runner's persistent workspace and
survives
git clean.Cause. Every self-hosted lane guarded the install on directory existence
alone:
A persistent directory plus an existence-only guard means that branch runs
exactly once in the lifetime of a runner. After the first run ever, every
subsequent run served against whatever runtime happened to be installed that
day — 16 days old on both MI300X runners when measured. Two consequences: the
lanes validate an increasingly historical SDK, and drift between the shared tree
and what a fresh
install sdkproduces is never exercised at all, so a break inthe install path can only be discovered by a new runner.
Fix. Keep the cache, but invalidate it when the channel index has actually
published something newer.
cargo xtask e2e-prewarmreplaces the guard in alleight self-hosted lanes and resolves to one of four outcomes:
install sdkup_to_date/ahead_of_indexWhy it is shaped this way
Reuse the CLI's primitives instead of reimplementing version resolution.
rocm updatealready reports per-runtime freshness against the channel index,rocm update --apply --runtime <key> --activatealready installs a newer runtimeside-by-side, and
rocm storage remove-old-installs --keep Nalready bounds theresulting multi-version cache. Comparing versions in workflow shell would have
duplicated logic that exists — and is tested — in the product. It also means the
GPU lanes now exercise
updateandstorageon real hardware, which nothingdid before.
Side-by-side, never move the tree.
install sdkbakes absolute paths intothe runtime manifest, so a runtime has to be created in its final location. This
is why the update path uses
--apply --activaterather than installingelsewhere and swapping.
In
xtask, not in the workflows. The pre-warm block is duplicated acrosseight jobs in two shells (bash on the Linux lanes, PowerShell on Strix Windows).
Inline logic would exist eight times in two languages and be untestable; the
Strix Windows lanes lose their PowerShell copy rather than gaining a second
implementation to keep in sync. Same reasoning as the existing
xtask e2e.Anything unclear reuses. Only a report that positively reads as "a newer
version exists for our channel" triggers work. An unreachable package index
surfaces per-runtime as
status=error, which reuses and warns — a brieflyoffline index must not turn a GPU lane red, nor start a multi-GiB download on
every run. Install and update failures do propagate; a failed prune only warns.
A scenario pins the output shape — partially, and the gap is worth naming.
The decision is parsed out of
rocm updatetext, so a fixture can drift from therenderer it imitates. The new
runtime-update-reports-freshnessscenario runs thereal command on hardware and asserts the shape the parser depends on: a
runtime <key> …line carryingchannel=and a recognisedstatus=. That coversstructural drift — a renamed prefix or a dropped
channel=filters every line outand makes the lane reinstall on every run, which is loud; a renamed
status=field fails the scenario outright.
What it does not cover is a rename of the value
update_available. A runnerthat is currently up to date reports
up_to_date, the assertion still passes, andthe decision quietly falls through to "reuse" — the exact defect this PR fixes.
Catching that reliably would need a runner that happens to be stale. The real
cause is that this is an unversioned text contract with more than one parser:
rocmdreads the same field by hand inupdate_output_reports_update_available, also against hand-written fixtures, soit carries the same exposure today. A machine-readable form of
update, or ashared constant for the status values, would close both. That is product-code
work and deliberately out of scope here.
Test plan
Verified locally:
cargo test --workspace --all-targets— 2215 passed, 0 failed. Includes 12 newunit tests covering every freshness verdict and the degraded paths.
cargo clippy --workspace --all-targets -- -D warningsandcargo clippy --locked -p e2e-cucumber --test e2e -- -D warningsclean;cargo fmt --all --checkclean.hand-written. That caught a real near-collision: the
update_surfacessummaryline is one character away from parsing as a runtime entry, and there is now a
test pinning that it does not.
rocmon the PATH, so the sequencingis verified without a multi-GiB download:
update→update --apply --runtime <key> --activate→storage remove-old-installs --keep 2 --yes, withROCM_CLI_{CONFIG,DATA,CACHE}_DIRcorrectly scoped to the pre-warm root.What the GPU lanes on this PR actually did
Every self-hosted lane reached the new pre-warm and reported the same verdict:
Two things follow, one of which contradicts what I expected:
rocm updateoutput contract holds on hardware. The newruntime-update-reports-freshnessscenario passed all three steps on StrixHalo/Windows against a real index — the parser's assumption about the report
shape is confirmed, not just imitated by fixtures.
the 16-day-old MI300X trees would take the update path. They did not, because
the Release channel currently resolves nothing newer than what those runners
already hold:
therock_index_urls()tries the classic per-familywhl/{family}index first, which still succeeds but tops out at ROCm 7.13.0(rocm install sdk: release channel never resolves ROCm 7.14+ (stuck on 7.13.0) #271, fix open in fix(therock): resolve ROCm releases from the current multi-arch pip index #272).
up_to_dateis an honest answer to a capped index,so reuse is right — but it means the install, update, activate and prune
paths are still unexercised on hardware, and remain covered only by unit
tests and the stubbed-binary run above.
Worth being plain about the consequence: on the Release channel as it resolves
today, this change is a no-op every run. Its value is that freshness is now
consulted at all — the previous guard would have kept a tree frozen for the life
of a runner no matter what the index published, whereas this self-corrects on the
first run after the index moves. That is also why it has to land before the
nightly-channel follow-up:
therock-nightlydoes move daily, and pointing a laneat a different channel under an existence-only guard would have been a silent
no-op.
CI status
All required checks are green. Two self-hosted lanes are not, and neither is
caused by this change:
E2E tests (Strix Halo, Windows)— red, pre-existing. 8 of 41 scenariosfail here against 8 of 40 on
main, and the two failure sets are identical; the+1 is the new scenario, which passes. That lane has failed on every
mainrunsince 2026-08-12 (the visible cause is a
llama-server.exedownload failingafter 6 attempts); tracked in #260 and #247.
E2E tests (Strix Halo, Ubuntu)— cancelled at its 35-minute cap. The tworunners behind that label are not equivalent:
strix-halo-ubuntufinishes in13.6–15.9 min, while
strix-halo-ubuntu-2takes 31.0–31.5 min when it passes andhas hit the cap three times today — the other two on unrelated branches,
including
main's own merge queue. This run drew runner-2. The pre-warm was notthe cost: it reused, downloaded nothing, and every serve scenario on that box ran
1.5–20× slower than on runner-1, including a refusal-path scenario that never
serves at all (371s vs under 20s).
One honest caveat on that second point: this PR does add a scenario to a lane
that, on runner-2, already sits within ~3.5 min of its cap, and the new
scenario's shared
a managed runtime is activeprecondition is expensive onthat box specifically (0.6s on MI300X; still running at 140s on runner-2 when
the job was killed). So it is a small marginal push on an already-overcommitted
budget. The runner-2 disparity looks worth its own issue rather than a timeout
bump smuggled into this PR — happy to raise one, or to widen the cap here if a
maintainer would rather see that.
Risk
Low-to-moderate, and concentrated in one place: the lanes can now download.
Previously the pre-warm was a no-op after a runner's first run; now a genuinely
newer index costs one install before the suite. That is the intended behaviour,
but it does mean a lane can be slower than it was, and the conservative bias
exists so this happens only when the index has actually moved. In practice, on
the Release channel today, it has not — see above.
The failure modes are bounded the other way. Every ambiguous outcome reuses, so
the common bad day (index unreachable) behaves exactly like today. The lanes are
continue-on-error: true, so even a hard failure in the pre-warm cannot gate amerge. No product code changes — this is CI orchestration, one new xtask
subcommand, and one new scenario.
Follow-up, tracked separately: with the tree no longer frozen, a scheduled lane
can track the
therock-nightlychannel, which is currently never installed byCI (EAI-8056). That needs this first, for the reason given above.
tests/e2e-cucumber/expectations.tomlforEAI-8057; no xfailrows reference it. The defect was in CI orchestration, not in any scenario's
expected outcome, so there is nothing to narrow.