ci(e2e): add an MI350P self-hosted E2E lane

[tomastola]

Summary

Adds per-PR and nightly E2E lanes on an MI350P (gfx950) runner, so the suite covers a
second Instinct generation alongside the existing MI300X lane. The runner is already
registered and idle; nothing targets it until this lands.

• runs-on: [self-hosted, linux, mi350p] — the label names the hardware rather than
  reusing the generic amd-gpu. GitHub matches a job to any runner whose labels are a
  superset, so a shared label would let the two Instinct generations pick up each
  other's work.
• The GPU preflight floor matches the other Instinct lane rather than scaling with the
  card's 144GB. It only has to be high enough to catch a leftover serve still holding
  the card, and low enough that a clean host passes on the first poll.
• Non-blocking (continue-on-error) like every other self-hosted lane, wired into both
  consolidated reports, and documented in the hardware-testing contract.

How close the new lanes are to the R9700 ones

Structurally: same step order, same budgets, same preflight shape. The functional
differences are five substitutions — job id, display name, runs-on label, dispatch
value, and artifact name — because the contract tests added in #319/#320 exist
precisely because these blocks drifted apart one line at a time.

The comments are not identical, deliberately: the new blocks describe the runner in
deployment-neutral terms where the older ones name specific cluster primitives. So
diffing the two blocks to check for drift will show prose differences well beyond those
five substitutions. The derived contract tests, not a byte-for-byte diff, are what hold
the structural parts together.

A platform-slug bug this surfaced

platform_hardware_slug matched the -dcgpu suffix, but two distinct data-center
families normalize to it: gfx94X-dcgpu and gfx950-dcgpu. A gfx950 host therefore
reported platform_slug: "mi300x", so a flattened single-artifact report download would
have filed MI350P results in the MI300X column of the cross-platform grid.

Fixed in its own commit by matching the family instead of the suffix. gfx94x is
unaffected and still slugs as mi300x. Nothing produced gfx950 in CI before this PR, so
there is no behaviour change for any existing lane.

Documentation guards this lane exposed

Adding a lane made two lists in tests/e2e-cucumber/README.md wrong, and neither was
caught:

• The nightly sentence was guarded, but by a literal string match against a copy of
  that sentence in the test source — so the guard and the doc could go stale together
  while the test stayed green. That is what happened when the R9700 lane landed, and it
  would have happened again here.
• The CI job table had no guard at all and was already missing e2e-gpu-rad3.

Both are now derived from the workflows, the way the surrounding assertions already
derive docs/ci-hardware-testing.md. Each new assertion was confirmed to fail with a
lane removed from the doc.

The report legend enumerated the self-hosted platforms by hand in two places and had
also fallen behind (R9700 was already absent). It no longer enumerates them at all —
the report's own rows and columns are the list.

Verification

• The gfx target is confirmed on the hardware, not inferred: rocm-smi --showhw reports
  GFX VER gfx950 (VBIOS 113-350P-01-1K1-0700).
• The lane has run green on the hardware: run 33764055728, job E2E tests (MI350P)
  on runner mi350p-0, ~18 min, every step green. Its platform.json carries
  platform_slug: "mi350p" / gfx_target: "gfx950", so the slug fix is confirmed by the
  artifact and not only by the unit test, and the consolidated report picked the new
  artifact up alongside the others.
• cargo test -p xtask (131), -p e2e-report (43), -p e2e-cucumber --lib (109) all
  pass, including the derived workflow-contract guards that fail until the doc table,
  runner labels, canonical artifact list, dispatch choices, and every prose enumeration
  follow a new lane.
• cargo fmt --check and clippy -D warnings clean on the changed crates.
• Regression tests pin the whole gfx→slug mapping, including the family-label and
  suffixed forms, so neither the suffix collapse nor a tightened starts_with can come
  back.

[rominf]

I went through this against the PR head and against what the lane actually did on the hardware, and it holds up well. One doc enumeration is left wrong by this change, though, and the guard that exists to prevent exactly that structurally can't see it. It's a one-line fix — details inline.

What I verified beyond reading the diff

• The lane really ran. Run 33764055728, job E2E tests (MI350P) on runner mi350p-0, every step green in ~18 min. Worth saying out loud because the lane is continue-on-error, so a green check by itself would not have told anyone that.
• The slug fix is confirmed by the artifact, not just the unit test. e2e-gpu-mi350p-report's platform.json from that run carries platform_slug: "mi350p", gfx_target: "gfx950", effective_serve_engine: "vllm". So the probe really does emit a bare gfx950, normalize_family really does land on gfx950-dcgpu, and that string is exactly the key label_for_root_report looks for. Without this commit the same run would have filed itself as mi300x — and since Grid::build dedups columns by slug and only overwrites a Missing outcome, the MI300X and MI350P results would have silently collided rather than merged. This half of the PR is load-bearing, not cosmetic.
• The consolidated report picked the new artifact up alongside the other five.

Things I chased that turned out fine

• Expectations aren't keyed by platform slug (Condition matches on engine / os / family / gpu / wsl), so the new slug doesn't orphan the lane from the xfail matrix. The artifact confirms it — the vLLM-conditioned rows resolved normally on the first run.
• serve-07 (the lemonade canary) fails on MI350P, but it fails identically on the existing MI300X lane in the same run, so that's the known lemonade bug rather than anything this lane introduces.
• Both new jobs satisfy the derived guards: preflight step present, 300s serve budget, per-PR/nightly platform parity, artifact name known to the report.

One thing I can't check from here

Both lanes assert a separate volume is mounted at /var/tmp/rocm-e2e-uv-cache, and the preflight comment cites 144GB of capacity. Neither is visible in this repo. Worth a sanity check against the runner deployment — if that mount isn't actually there, the ~23GB uv cache quietly lands on the work volume instead.

Two notes on the description

It still ends with "Draft until the lane has run on the hardware," which is no longer true — the PR is ready and the lane came back green. Worth trimming so a reader isn't left wondering which state it's in.

And "byte-identical to their R9700 siblings apart from five substitutions" undersells the diff: the comments were reworded throughout too (PVC/overlay → volume/deployment, ticket refs dropped). Not a problem in itself, but it does mean the next person diffing the two blocks to check for drift will find plenty of it.

[rominf]

Adding this nightly lane makes a sentence in tests/e2e-cucumber/README.md wrong, and the test whose whole job is to prevent that can't see it.

README line 185 still reads:

The nightly workflow runs non-blocking jobs — MI300X, Radeon R9700, and Strix Halo on Ubuntu, Windows, and WSL2 — with E2E_INCLUDE_NIGHTLY=1

With e2e-gpu-nightly-mi350p in nightly.yml, that list is now a platform short.

The guard is the final assert! in xtask/src/workflow_contract.rs's docs-coverage test, and it asserts the README contains that exact literal, copied into the test source. So it stays green while its own failure message — "E2E README must identify every nightly job platform" — has stopped being true. The four sibling assertions immediately above it are derived from the YAML (declared_ids, backticked_list_between), which is exactly why docs/ci-hardware-testing.md got updated correctly and this didn't. It's the one hand-copied hole in an otherwise derived net.

Minimum fix: add MI350P to the README sentence and to the literal in the test. Better fix, if you're up for it: derive it from self_hosted_e2e_jobs(&nightly) the way the assertions just above do, so the next lane can't repeat this.

Separately, the CI job table a few lines earlier in the same README (~line 168) lists e2e, e2e-gpu, e2e-gpu-strix-ubuntu, e2e-gpu-strix-windows, e2e-wsl — it's already missing e2e-gpu-rad3, and would now be missing e2e-gpu-mi350p too. Nothing guards that table at all; the derived one only reads docs/ci-hardware-testing.md. Worth adding both rows while you're in the file.

[tomastola]

Fixed in 0d3d644, taking the "better fix" — the sentence is derived now, not
hand-copied.

The README nightly sentence names the nightly job ids in backticks, and the guard
compares them against self_hosted_e2e_jobs(&nightly), exactly like the four
assertions above it. The literal copy in the test source is gone, so the failure mode
you describe — the guard staying green while its own message stops being true — can't
recur for this sentence.

You were right that the CI job table was worse: no guard at all, and already missing
e2e-gpu-rad3 before this lane. Both rows added, and that table is derived too, against
e2e-selfhosted.yml — the mock row is matched by workflow so only the self-hosted rows
have to track the YAML, plus a check that none of them is documented as blocking.

I verified both new assertions actually bite rather than trusting a green run: removing
e2e-gpu-nightly-mi350p from the sentence fails with

the E2E README must name every nightly self-hosted lane, in workflow order
  left:  [..., "e2e-gpu-nightly-strix", ...]
  right: [..., "e2e-gpu-nightly-mi350p", "e2e-gpu-nightly-strix", ...]

and removing the e2e-gpu-mi350p table row fails the table assertion the same way.

[rominf]

Non-blocking, and it predates this PR — but the new lane makes it a third omission, and the text is rendered by the very report the new column now shows up in.

The legend enumerates the hardware platforms by hand in two places in this file: the Markdown step summary ("MI300X / Strix Halo run on real self-hosted GPU hardware with real engines") and the HTML legend (b { "MI300X / Strix Halo" }). R9700 was already absent; MI350P makes three lanes the legend implicitly disclaims while the grid displays their columns.

Since it's hand-maintained prose with no guard on it, the durable fix is to stop enumerating — something like "the non-Mock platforms run on real self-hosted GPU hardware; non-blocking while proven out" — rather than adding a name every time a lane lands. Happy either way on whether that belongs here or in a follow-up.

[tomastola]

Agreed, and done here rather than as a follow-up — it's two lines and the alternative is
remembering to add a name every time a lane lands, which is the habit that produced the
omission in the first place. daaa985.

Both places now say "Every other platform — real self-hosted GPU hardware; non-blocking
while proven out" and enumerate nothing.

One wrinkle worth noting since it changed the wording I first wrote: the two surfaces
have opposite orientations. The HTML grid puts platforms in columns (Grid::build
dedups columns by slug), but the Markdown step summary puts them in rows
(| Platform | OS | Total | ... |). So "every other column" would have been wrong in the
Markdown summary. "Platform" is correct on both, which is a small argument for the
non-enumerating phrasing beyond just staleness.

[rominf]

Suggestion rather than a blocker. This assertion pins the happy path, but the branch the change actually restructured is the if / else if chain in platform_hardware_slug, and a few of its inputs stay uncovered:

• gfx94X-dcgpu (the family-label form, which is what the effective_serve_engine tests further up already use) still mapping to mi300x. That's the case the reordering could regress, and only the raw gfx942 form is asserted.
• gfx950-dcgpu itself, which reaches the new arm via normalize_family's early return rather than the starts_with path.
• The suffixed real-world form gfx950:sramecc+:xnack-. It works today because normalize_family uses starts_with, but nothing pins that, and a starts_with is easy to "tidy" into an == later. (The probe currently hands over a bare gfx950 — I checked the artifact — so this is belt-and-braces, not a live bug.)

A small table over ["gfx950", "gfx950-dcgpu", "gfx950:sramecc+:xnack-"] → "mi350p" and ["gfx942", "gfx94X-dcgpu"] → "mi300x" would cover all three in about the same number of lines.

One thought worth writing down somewhere while this is fresh: mi350p names one specific part for the whole gfx950 family, and MI350X/MI355X are gfx950 too — this repo's own catalog groups them (model_catalog.json: "AMD Instinct — MI300X, MI350X, MI355X"). That's the same trade-off mi300x already makes for gfx94x, so it's consistent and I'm not asking you to change it. But if a second gfx950 box ever joins the fleet it collides into this column, which is the bug class this PR just fixed one level up.

[tomastola]

Taken as written — af7f28f table-drives it over exactly the inputs you listed:

for (gfx_target, expected) in [
    ("gfx950", "mi350p"),
    ("gfx950-dcgpu", "mi350p"),
    ("gfx950:sramecc+:xnack-", "mi350p"),
    ("gfx942", "mi300x"),
    ("gfx94X-dcgpu", "mi300x"),
]

The suffixed form turned out to be the one worth having most, so I checked it rather
than assuming: tightening normalize_family's starts_with("gfx950") to == "gfx950"
now fails with

gfx target `gfx950:sramecc+:xnack-` must slug as `mi350p`
  left: "gfx950:sramecc+:xnack-"

Note the left side — under that change the target doesn't fall back to mi300x, it
falls through to the raw string as its own slug, which would have quietly created a
fourth grid column named after the sramecc flags rather than colliding with an existing
one. Worse than the bug this PR fixes, and nothing else would have caught it.

On the naming trade-off: agreed on all counts, and I've left mi350p as-is. Recording
your point here since you're right that it should be written down somewhere — mi350p
names one part for the whole gfx950 family, MI350X/MI355X are gfx950 too, and a second
gfx950 box joining the fleet would collide into this column. That is the same bug class
this PR fixes one level up, just deferred rather than removed. It's cheap to fix when it
happens (the slug function is the only place that decides) and speculative to fix now,
so the trade-off seems right, but it is a real edge and not a theoretical one.

[tomastola]

Thanks — the doc-guard catch was the valuable one; that assertion had been quietly
decorative since the R9700 lane. All four points addressed, each in its own commit,
replies inline:

• 0d3d644 — README nightly sentence and the CI job table, both derived from the
  workflows now, both confirmed to fail with a lane removed.
• daaa985 — report legend no longer enumerates platforms.
• af7f28f — slug mapping table-driven over all five gfx forms.
• Description rewritten (below).

On the uv cache mount — checked, and it's about to move

You were right to flag it, and it's worth being precise about one thing first: the lanes
don't actually assert the mount, they only export the path with a comment claiming
it. So a green run is not evidence either way — if the mount were missing, the ~23GB
cache would land on the work volume and nothing would say so. I went looking in the
MI350P job log and there's no trace of it, as expected.

The mount is real, though, and confirmed independently: #352 measured it on a runner
(/var/tmp/rocm-e2e-uv-cache and the work volume report the same st_dev yet ln
between them still fails EXDEV, so it is a genuine bind mount).

Which leads to the more useful finding: #352 removes this path entirely. That
separate mount is exactly what stops uv hardlinking out of its cache into the managed
environments, so every scenario was copying the whole ROCm SDK and torch stack instead
of linking. It moves the cache to $RUNNER_WORKSPACE/uv-cache.

So I've deliberately not added a mount assertion here — it would guard a path that is
being deleted. The real interaction is that #352 and this PR touch the same
E2E_SHARED_UV_CACHE_DIR lines, and this PR adds a sixth occurrence of the old path.
Whichever lands second needs the trivial rebase; flagging it so it isn't a surprise. If
#352 goes first I'll fold the new lane onto the new path before merging this.

On the description

Both fair. The "Draft until the lane has run on the hardware" line is gone — it was
stale the moment the run came back green. And the "byte-identical apart from five
substitutions" claim is replaced with something accurate: the comments differ throughout
because the new blocks describe the runner in deployment-neutral terms rather than
naming cluster primitives, so anyone diffing the two blocks for drift will find plenty of
prose differences. The description now says that outright, so the next person isn't
misled into thinking a clean diff was ever the invariant — the derived contract tests
are.

[siloteemu]

🔴 Automated review · pr-review-watcher · 36dce2d

Summary

Adds an MI350P (gfx950) self-hosted E2E lane to e2e-selfhosted.yml and nightly.yml, fixes platform_hardware_slug so gfx950 no longer files its results in the MI300X column, and replaces two hand-copied doc enumerations with guards derived from the workflow YAML. Approve — the lane is genuinely wired (dispatch option added, if: byte-equivalent to the rad3/e2e-gpu siblings, present in both consolidated-report needs: lists, artifact name round-trips through label_for_root_report → parse_descriptor and is caught by the existing every_uploaded_e2e_artifact_has_a_name_the_report_can_label guard), and the new tests are real regression guards rather than tautologies. Verified: traced normalize_family by hand for all five gfx forms the new table covers and confirmed deleting the family == "gfx950-dcgpu" branch makes the test fail; confirmed both new guards run on PRs via ci.yml's build-and-test → cargo test --workspace --all-targets (tests/e2e-cucumber and xtask are workspace members), and ran two targeted tests locally (platform_slug_derivation, hardware_testing_docs_cover_all_self_hosted_platforms) — both pass. Per this run's policy the full suite, the cucumber e2e suite and the GPU/Windows lanes were left to CI rather than run here. Leak scan on the diff is clean and no prompt-injection content was found in any reviewed file. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• xtask/src/workflow_contract.rs:1069 — the Blocking-column check asserts row[3] == "no" as a literal instead of reading continue-on-error: from the YAML, so a lane that stopped being advisory would leave the README saying "no" and the guard still green: the exact hand-copied hole the PR's own comment says it is closing, relocated one column over (job_scalar at :382 could derive it).
• xtask/src/workflow_contract.rs:40 — SELF_HOSTED_LABELS gained no mi350p entry alongside mi300x/r9700; harmless today because every lane also carries self-hosted, but it is the hardware-specific defense-in-depth list and is now one lane short.
• crates/e2e-report/src/lib.rs:1284 — "Every other platform is a real self-hosted GPU host running real engines" overclaims for e2e-wsl, where docs/ci-hardware-testing.md:71 says GPU availability is advisory and GPU scenarios resolve to not-applicable; the overclaim predates the PR but now auto-extends to every future lane.
• tests/e2e-cucumber/src/capability.rs:526 — gfx950 covers MI350X/MI355X too (the repo's own catalog string in apps/rocm/src/main.rs:29428 lists all three), so an MI355X host would be columned as "MI350P" — the same one-family-many-parts mislabel this PR fixes; the rad3 lane sidestepped it by slugging the target (gfx1201) rather than the product.
• .github/workflows/e2e-selfhosted.yml:968 / nightly.yml:555 — timeout-minutes does not bound queue time, so if no runner registers the mi350p label the job stays pending and both consolidated-report jobs (which needs: it) never run; not new to this lane, but worth confirming the runner is registered before merge.

[rominf]

Rechecked at 36dce2dd. All three points are addressed, and I verified them against the code rather than the replies.

The README nightly sentence and the CI job table are derived from the workflows now, not literal-matched — I checked job-definition order in both files against both documented lists, and the prefix/suffix pairs the assertions split on all resolve. The legend no longer enumerates platforms on either surface. platform_hardware_slug matches the family before the suffix arm, and the table-driven test covers all five forms; I confirmed normalize_family's early -dcgpu return keeps gfx94X-dcgpu on mi300x.

I also looked for the omission class a new lane tends to cause and didn't find one: expectations.toml derives platform from probed capability rather than a hardcoded slug list, and gfx950 already exists in normalize_therock_family and model_catalog.json, so vLLM preference is right on the new box.

Taking the derived fix over the minimum one was the right call — the hand-copied literal was the actual hole, and deriving it means the next lane can't reopen it. "E2E tests (MI350P)" is green, so the lane has now run on the hardware and the draft note no longer applies.

This reflects a read of the diff plus the checks above, not a run of the suite on my side.