Skip to content

ci: narrow the GPU serve matrix with a serve paths-filter + PR canary (EAI-7746) - #156

Open
fredespi wants to merge 1 commit into
mainfrom
e2e-task-9-narrow-serve-paths-filter-so-non
Open

ci: narrow the GPU serve matrix with a serve paths-filter + PR canary (EAI-7746)#156
fredespi wants to merge 1 commit into
mainfrom
e2e-task-9-narrow-serve-paths-filter-so-non

Conversation

@fredespi

Copy link
Copy Markdown
Collaborator

Summary

The changes job gated the three self-hosted GPU E2E jobs (e2e-gpu, e2e-gpu-strix-ubuntu, e2e-gpu-strix-windows) on the coarse heavy paths-filter, which trips on any **/*.rs change. So a Rust PR that can't affect serving (dashboard, unrelated crates) fired the whole real-GPU serve matrix on scarce serial hardware. This narrows that matrix and adds a fast pre-merge serve canary.

Changes

  • serve paths-filter (EAI-7746): a dedicated filter covering the serve code path (engines/**, crates/rocm-core, crates/rocm-engine-protocol, apps/rocm, apps/rocmd), the E2E harness (tests/e2e-cucumber/**, crates/e2e-report, xtask, **/*.feature), plus broad-dep safety nets (root Cargo.toml, Cargo.lock, rust-toolchain*, workflow files). Excludes the rocm-dash-* crates — they build into rocm but can't change serve behaviour, and their compile coverage stays on the always-on build/test lanes. The GPU jobs now gate on serve; forced true off-PR so the merge queue always runs the full matrix (required checks never starved).
  • merge_group gating + PR canary: the two Strix lanes run on merge_group/push only (skip on PR; required-but-continue-on-error, so the skip satisfies branch protection). The MI300X lane still runs on pull_request but in canary mode (E2E_PR_CANARY), serving only a single minimal real scenario as a pre-merge smoke; the full matrix runs in the merge queue.
  • @canary / @serves-on-gpu harness gate (mirrors @nightly): in canary mode every GPU-serving scenario except the canary resolves to Skip, so platform.json stays valid and the consolidated report reconciles. @serves-on-gpu tags the scenarios that real-serve on a GPU host without @requires-gpu (short-name expansion, chat behavioural) so they're skipped in canary mode but still mock-covered every PR.
  • report gate: the consolidated report now gates on heavy || serve so a serve-only change still produces the required E2E consolidated report check.

Test plan

  • Linux container gate green: clippy --workspace --all-targets --exclude e2e-cucumber -D warnings, workspace tests, e2e-cucumber --lib, and cargo xtask e2e mock lane (reconciliation: 3 xfail / 0 XPASS / 0 unexpected).
  • ci.yml YAML parses; serve gates exactly the 3 GPU jobs + report; heavy still gates the mock/build/test lanes.
  • On this PR: confirm every required GPU check is produced (run or green-skip), none left pending.
  • Scoped dispatch to confirm the canary path serves only the single @canary scenario.

…I-7746)

The `changes` job gated the three self-hosted GPU E2E jobs on the coarse
`heavy` filter, so any Rust change (dashboard, unrelated crates) fired the
whole real-GPU serve matrix on scarce serial hardware.

- Add a dedicated `serve` paths-filter (serve code path + engines + E2E
  harness + broad-dep safety nets; excludes the rocm-dash crates) and gate
  the GPU jobs on it, so non-serve Rust PRs skip the matrix. Forced true
  off-PR so the merge queue always runs the full matrix.
- Gate the two Strix lanes to merge_group/push only; keep the MI300X lane
  on pull_request in canary mode (E2E_PR_CANARY) running a single minimal
  real serve as a pre-merge smoke.
- Add a `@canary` / `@serves-on-gpu` harness gate (mirrors `@nightly`): in
  canary mode every GPU-serving scenario except the canary resolves to Skip,
  keeping platform.json valid so the consolidated report reconciles.
- Gate the consolidated report on `heavy || serve` so a serve-only change
  still produces it.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
@fredespi
fredespi requested a review from a team as a code owner July 31, 2026 16:02
@rominf

rominf commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Automated review pass over the diff against main (CI workflow + harness + features reviewed in full, plus surrounding context). No code changed. Findings prioritized; nothing here is a hard blocker.

Findings

1. tests/e2e-cucumber/README.md:98-106 — tag table is stale (please fix in this PR).
The table is the documented contract for the tag vocabulary (@id, @requires-gpu, @requires-engine, @requires-os, @serve-timeout, @nightly, @lifecycle). @canary and @serves-on-gpu are missing. Failure scenario: a contributor adds a scenario that real-serves on a GPU host without @requires-gpu, has no way to learn @serves-on-gpu exists, omits it — and it launches an extra real vLLM serve on every PR canary run, re-inflating exactly the cost this PR removes. Same file: line 91-94 ("Each CI job runs the whole suite … no -t filter") and the job table at 125-136 are now inaccurate — e2e-gpu on a PR runs one scenario, and the two Strix lanes don't run on PRs at all.

2. tests/e2e-cucumber/src/expectation.rs:373-381 + features/model_serving.feature:84 — nothing enforces "exactly one @canary".
Verified exactly one scenario carries it today, and that no code, test, or CI step checks cardinality. Failure scenario: a later refactor of scenario 6b drops or renames the tag. E2E_PR_CANARY=1 then skips every GPU-serving scenario with nothing left to serve — the lane goes green in ~2 minutes, platform.json is well-formed, reconciliation is clean, and the pre-merge serve smoke has silently stopped testing anything real, with no signal anywhere. A unit test that parses the real .feature files and asserts a single @canary would close this cheaply.

3. .github/workflows/ci.yml:962-979, 1122-1139 (with continue-on-error at 777/979/1139) — "the full matrix runs in the merge queue" implies protection that isn't there.
All three GPU jobs are continue-on-error: true, including under merge_group. So a real Strix (lemonade / Windows) serve regression now: skips on the PR, runs in the merge queue, fails without blocking, and lands on main. Previously it at least showed up as a red non-blocking check on the PR, in front of the author. Effective detection moves to nightly, a day later, decoupled from the causing PR. That may well be the trade you want given serial hardware — but the comments and PR body read as if the merge queue is a backstop, and it isn't a gating one. Suggest either saying so plainly in the comment, or dropping continue-on-error on the merge_group path.

4. .github/workflows/ci.yml:131-157 — the crates/rocm-dash-* exclusion rests on an unverified runtime-isolation assumption.
apps/rocm/Cargo.toml:20-31 shows rocm depends directly on rocm-dash-daemon / rocm-dash-tui (transitively rocm-dash-core, rocm-dash-collectors), none of which are in the serve filter. Compile risk is genuinely covered (heavy/rust still match **/*.rs), so this is only about shared process state in the single rocm binary — global tracing/config/signal init. Failure scenario: a dash-only PR perturbs shared init, the GPU matrix skips on the PR, and it's caught one stage later when merge_group forces serve=true. Low severity, and the exclusion itself is structurally sound: root-only Cargo.toml is the right call, and I confirmed no other glob (scripts/**, xtask/**, **/*.feature) re-includes the dash crates. I did not trace apps/rocm verb dispatch to confirm isolation.

5. tests/e2e-cucumber/src/expectation.rs:373-381 — gate ordering yields a misleading skip reason.
The canary gate sits before the requires_gpu && !cap.has_amd_gpu check, so a @requires-gpu scenario on a GPU-less host in canary mode would report "skipped in PR canary mode" rather than "requires an AMD GPU". Verified unreachable today (E2E_PR_CANARY is set only on the MI300X lane) and harmless if reached — crates/e2e-report reconciles on the expected label and never parses reason. Swapping the two gates is a one-line defensive fix.

6. tests/e2e-cucumber/src/expectation.rs:356-364resolve() is now three trailing bools deep (include_nightly, include_lifecycle, canary_mode), all same-typed and positional, with ~20 call sites passing false, false, false. A future swap of two of them compiles clean and silently mis-resolves. Cross-PR note: #155, #156 and #157 are each independently growing this parameter list — one shared RunMode-style struct beats three separate fixes, so it's probably worth agreeing where that lands rather than each PR adding a bool.

Also cross-PR: the README.md tag table is stale in both this PR and #157 — whichever lands second should be the one to fix it.

Verified clean

  • @serves-on-gpu tagging is complete. Traced every non-@requires-gpu scenario across all 9 feature files into its step definitions. chat.feature 5/6 (serving_steps.rs:555-576, branches on has_amd_gpu) and model_serving.feature 1/2 (serving_steps.rs:590-604, unconditional real rocm serve) are the only real-servers, and all four are tagged. No over-tagging either. This was the highest-risk item in the change and it's closed.
  • Serve dependency closure is covered: engines/** catches rocm-engine-lemonade/-vllm, apps/rocmd/** catches rocmd, plus rocm-core and rocm-engine-protocol. Only the dash crates fall outside (see Fix CI: prek hooks, headless/flaky test fixes, and drop vendored Codex #4).
  • Off-PR forcing (ci.yml:87 plus the all/forced step) genuinely resolves serve to true on merge_group/push/dispatch.
  • serve really is not a subset of heavy, so the || serve report gate is load-bearing, not cosmetic: crates/rocm-core/src/model_catalog.json matches serve but not heavy. Traced that path — build-and-test still reports success (its job-level if is trigger-only; the steps are heavy-gated), so e2e-gpu runs and produces the artifact the report job needs.
  • Branch-protection reasoning holds: the required contexts on main include "E2E tests (GPU)", "E2E tests (Strix Halo, Ubuntu)", "E2E tests (Strix Halo, Windows)" and "E2E consolidated report", and a job-level if: skip reports as satisfied. So the PR-skip of the Strix lanes does not stall the queue.
  • Canary expectation claim checked against expectations.toml:64-69: serve-default-engine-inference xfails only on lemonade+linux (EAI-7423), so ExpectPass on MI300X, exactly as the feature comment says.
  • No stale call sites: resolve() has one non-test caller (tests/e2e.rs:829), updated. nightly.yml is independent of the paths-filter. No tag allowlist or linter to update.
  • cargo test -p e2e-cucumber --lib passes locally (57/57).

Not verified

  • ci.yml was not machine-parsed here (no YAML module in this environment); expressions and indentation were checked by hand and all needs.changes.outputs.* references are valid.
  • The two unchecked boxes in your test plan — every required GPU check produced on this PR, and a scoped dispatch confirming the canary serves only the single scenario — need real CI runs and remain the right gate before merge.

Nice change overall: the filter comments explain the reasoning rather than just the rule, and the new unit tests cover canary-on/canary-off for both @requires-gpu and @serves-on-gpu paths.

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.

2 participants