From 937c064e134b79b1e3f120359acd5d8464aa7e77 Mon Sep 17 00:00:00 2001 From: fredespi Date: Fri, 31 Jul 2026 17:51:24 +0200 Subject: [PATCH 1/2] ci: narrow GPU serve matrix with a serve paths-filter + PR canary (EAI-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 --- .github/workflows/ci.yml | 66 ++++++-- tests/e2e-cucumber/features/chat.feature | 9 +- .../features/model_serving.feature | 15 +- tests/e2e-cucumber/src/expectation.rs | 147 +++++++++++++++--- tests/e2e-cucumber/tests/e2e.rs | 14 +- 5 files changed, 210 insertions(+), 41 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6c55847d..f685a5bc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -82,6 +82,9 @@ jobs: # filter step is skipped off-PR), otherwise the forced 'true'. rust: ${{ steps.filter.outputs.rust || steps.all.outputs.forced }} heavy: ${{ steps.filter.outputs.heavy || steps.all.outputs.forced }} + # Narrow GPU-serve gate; forced true off-PR so the merge queue always runs + # the full matrix and its required checks are never starved. + serve: ${{ steps.filter.outputs.serve || steps.all.outputs.forced }} lint: ${{ steps.filter.outputs.lint || steps.all.outputs.forced }} tpn: ${{ steps.filter.outputs.tpn || steps.all.outputs.forced }} steps: @@ -125,6 +128,33 @@ jobs: # `cargo xtask verify-pinned-keys`. - 'docs/keys/**' - '.github/workflows/**' + # The real-GPU serve matrix (e2e-gpu*, non-blocking). Narrower than + # `heavy`: only paths that can change serve BEHAVIOUR or the GPU E2E + # harness — NOT a blanket `**/*.rs`. Compile coverage for every crate + # already runs on the always-on build/test lanes, so a dash-only or + # unrelated-crate Rust PR need not fire the heavy serve matrix. Err + # toward inclusion — Cargo.lock, the toolchain, and the workflow + # itself are broad safety nets so a transitive-dep or CI change still + # runs the matrix. (Excluded on purpose: crates/rocm-dash-* — they + # build into `rocm` but cannot change serve behaviour.) + serve: + - 'engines/**' + - 'crates/rocm-core/**' + - 'crates/rocm-engine-protocol/**' + - 'apps/rocm/**' + - 'apps/rocmd/**' + - 'tests/e2e-cucumber/**' + - 'crates/e2e-report/**' + - 'xtask/**' + - '**/*.feature' + - 'scripts/**' + # Root manifest only (NOT `**/Cargo.toml` — that would re-include + # the excluded dash crates): `[workspace.dependencies]` edits here + # can change a serving crate's deps without touching Cargo.lock. + - 'Cargo.toml' + - 'Cargo.lock' + - 'rust-toolchain*' + - '.github/workflows/**' # THIRD_PARTY_NOTICES.txt staleness gate: anything that changes the # dependency tree, the cargo-about config/template, the generator, or # the generated file itself. @@ -741,7 +771,7 @@ jobs: && ( (github.event_name != 'workflow_dispatch' && needs.build-and-test.result == 'success' - && needs.changes.outputs.heavy == 'true') + && needs.changes.outputs.serve == 'true') || (github.event_name == 'workflow_dispatch' && (inputs.platform == 'all' || inputs.platform == 'app-dev-gpu')) ) @@ -757,6 +787,12 @@ jobs: # workflow sets this unconditionally; here it lets a scoped dispatch confirm # a single nightly scenario (e.g. the 27B serve) without the full nightly run. E2E_INCLUDE_NIGHTLY: "${{ inputs.include_nightly && '1' || '' }}" + # PR canary mode: on a pull_request this MI300X lane serves ONLY the single + # @canary scenario (a minimal real vLLM serve) as a fast pre-merge smoke; + # the full serve matrix runs in the merge queue (merge_group forces this + # off). Every other @requires-gpu scenario resolves to Skip in the harness, + # so platform.json stays valid and the consolidated report reconciles. + E2E_PR_CANARY: "${{ github.event_name == 'pull_request' && '1' || '' }}" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -925,14 +961,19 @@ jobs: # carries `strix-halo`, but the paths below exist only on the native one. runs-on: [self-hosted, linux, strix-halo, native] needs: [changes, build-and-test] - # See `e2e`: dispatch tolerates skipped build-and-test; strix-ubuntu. + # Merge-queue-only heavy lane: this slow serve matrix runs in the merge queue + # (merge_group) and on push to main, NOT on every pull_request — the PR + # pre-merge serve smoke is the MI300X @canary lane above. A manual dispatch + # can still target it. The job is required-but-continue-on-error, so skipping + # it on a PR reports the required check as skipped (branch protection + # satisfied) without running the hardware. if: >- always() && needs.changes.result == 'success' && ( - (github.event_name != 'workflow_dispatch' + ((github.event_name == 'merge_group' || github.event_name == 'push') && needs.build-and-test.result == 'success' - && needs.changes.outputs.heavy == 'true') + && needs.changes.outputs.serve == 'true') || (github.event_name == 'workflow_dispatch' && (inputs.platform == 'all' || inputs.platform == 'strix-ubuntu')) ) @@ -1082,14 +1123,17 @@ jobs: timeout-minutes: 35 runs-on: [self-hosted, windows, strix-halo, native] needs: [changes, build-and-test] - # See `e2e`: dispatch tolerates skipped build-and-test; strix-windows. + # Merge-queue-only heavy lane (see e2e-gpu-strix-ubuntu): runs on merge_group + # and push to main, NOT on every pull_request; a manual dispatch can target + # it. Required-but-continue-on-error, so a PR skip satisfies branch + # protection without running the hardware. if: >- always() && needs.changes.result == 'success' && ( - (github.event_name != 'workflow_dispatch' + ((github.event_name == 'merge_group' || github.event_name == 'push') && needs.build-and-test.result == 'success' - && needs.changes.outputs.heavy == 'true') + && needs.changes.outputs.serve == 'true') || (github.event_name == 'workflow_dispatch' && (inputs.platform == 'all' || inputs.platform == 'strix-windows')) ) @@ -1246,11 +1290,15 @@ jobs: - e2e-gpu - e2e-gpu-strix-ubuntu - e2e-gpu-strix-windows - # Consolidate whatever ran. On dispatch `heavy` is unset, so also run when the - # trigger was manual; `always()` still lets it collect partial/failed tiers. + # Consolidate whatever ran. Gate on heavy OR serve: the mock e2e lane runs on + # `heavy`, the GPU lanes on `serve`, and `serve` matches paths `heavy` does + # not — so a serve-only change runs GPU jobs whose report must still be + # produced. On dispatch both are unset, so also run when the trigger was + # manual; `always()` still lets it collect partial/failed tiers. if: >- always() && (needs.changes.outputs.heavy == 'true' + || needs.changes.outputs.serve == 'true' || github.event_name == 'workflow_dispatch') steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 diff --git a/tests/e2e-cucumber/features/chat.feature b/tests/e2e-cucumber/features/chat.feature index d0cc2609..6827541d 100644 --- a/tests/e2e-cucumber/features/chat.feature +++ b/tests/e2e-cucumber/features/chat.feature @@ -45,7 +45,9 @@ Feature: Chat and endpoint detection # real `rocm serve`, on the no-GPU mock lane it's backed by MockServer. The # assertion (a tools-bearing request is accepted) is engine-agnostic, so no GPU # is required — dropping @requires-gpu gives this per-PR mock-lane coverage. - @id:chat-tool-definitions-accepted + # @serves-on-gpu: it DOES real-serve on a GPU host, so PR canary mode skips it + # (the mock lane still covers it every PR). + @id:chat-tool-definitions-accepted @serves-on-gpu Scenario: 5 - Chat requests that include tool definitions are accepted Given a managed runtime is active And a model is served in the background @@ -55,8 +57,9 @@ Feature: Chat and endpoint detection # Runs on every lane (see scenario 5): real serve on a GPU host, MockServer on # the no-GPU mock lane. Asserts only that a served model returns a non-empty # reply, which is engine-agnostic — real generation is covered by the - # @requires-gpu serve-*-inference scenarios. - @id:chat-end-to-end-local-model + # @requires-gpu serve-*-inference scenarios. @serves-on-gpu: real-serves on a + # GPU host, so PR canary mode skips it (mock lane still covers it every PR). + @id:chat-end-to-end-local-model @serves-on-gpu Scenario: 6 - End-to-end chat through a locally served model Given a managed runtime is active And a model is served in the background diff --git a/tests/e2e-cucumber/features/model_serving.feature b/tests/e2e-cucumber/features/model_serving.feature index 27fe87f3..3b822ac1 100644 --- a/tests/e2e-cucumber/features/model_serving.feature +++ b/tests/e2e-cucumber/features/model_serving.feature @@ -5,12 +5,15 @@ Feature: Model serving # @expected-failure tags. Engine requirements are declared via @requires-engine # so the harness can skip a scenario whose engine can't start on this host. - @id:serve-short-name-expansion + # @serves-on-gpu: the step does a real `rocm serve` on a GPU host, so PR canary + # mode skips it (mock lane still covers name expansion every PR). + @id:serve-short-name-expansion @serves-on-gpu Scenario: 1 - Short model names are expanded to their full name When the user serves a model using its short name Then the output shows the full model name - @id:serve-short-name-consistent-across-engines + # @serves-on-gpu: real `rocm serve` on a GPU host (see scenario 1). + @id:serve-short-name-consistent-across-engines @serves-on-gpu Scenario: 2 - Short name expansion is consistent across engines When the user serves the same short name with different engines Then all engines expand to the same full model name @@ -73,8 +76,12 @@ Feature: Model serving Then an engine is selected automatically And the model is reachable - # The inference half of scenario 6. - @id:serve-default-engine-inference @requires-gpu + # The inference half of scenario 6. Also the PR pre-merge serve canary + # (@canary): a real default-engine serve + inference that is ExpectPass on + # MI300X (only xfail on lemonade+linux, EAI-7423). In canary mode (the MI300X + # pull_request lane, E2E_PR_CANARY) this is the ONLY GPU scenario that serves; + # the full matrix runs in the merge queue. See resolve()/@canary in expectation.rs. + @id:serve-default-engine-inference @requires-gpu @canary Scenario: 6b - A default-engine served model responds to inference requests Given a managed runtime is active When the user serves a model without specifying an engine diff --git a/tests/e2e-cucumber/src/expectation.rs b/tests/e2e-cucumber/src/expectation.rs index c8b12e4a..1a9d3772 100644 --- a/tests/e2e-cucumber/src/expectation.rs +++ b/tests/e2e-cucumber/src/expectation.rs @@ -29,6 +29,8 @@ const REQUIRES_NO_GPU_TAG: &str = "requires-no-gpu"; const SERVE_TIMEOUT_PREFIX: &str = "serve-timeout:"; const NIGHTLY_TAG: &str = "nightly"; const LIFECYCLE_TAG: &str = "lifecycle"; +const CANARY_TAG: &str = "canary"; +const SERVES_ON_GPU_TAG: &str = "serves-on-gpu"; /// The resolved expectation for one scenario on one host. #[derive(Debug, Clone, PartialEq, Eq)] @@ -78,6 +80,19 @@ pub struct ScenarioDecl { /// so the fast E2E suite stays fast, and only runs when the caller opts in via /// `E2E_INCLUDE_LIFECYCLE`. Runs explicitly in heavy CI and on demand. pub lifecycle: bool, + /// `@canary`: the single minimal real-serve scenario that runs as the PR + /// pre-merge smoke. In canary mode (CI sets `E2E_PR_CANARY` on the MI300X + /// lane for a pull_request), every OTHER GPU-serving scenario is skipped so + /// only this one serves; the full matrix runs in the merge queue. Has no + /// effect outside canary mode. + pub canary: bool, + /// `@serves-on-gpu`: this scenario does a REAL `rocm serve` when an AMD GPU is + /// present, even though it is not tagged `@requires-gpu` (its assertion is + /// engine-agnostic, so it also runs mock-backed on the no-GPU lane — e.g. the + /// chat behavioural scenarios). Canary mode must skip these too, or they'd + /// launch extra real serves on the PR MI300X lane. No effect outside canary + /// mode: on a GPU host they still real-serve; on mock they stay mock-backed. + pub serves_on_gpu: bool, } impl ScenarioDecl { @@ -92,6 +107,8 @@ impl ScenarioDecl { let mut serve_timeout_secs = None; let mut nightly = false; let mut lifecycle = false; + let mut canary = false; + let mut serves_on_gpu = false; for tag in tags { let tag = tag .as_ref() @@ -113,6 +130,10 @@ impl ScenarioDecl { nightly = true; } else if tag == LIFECYCLE_TAG { lifecycle = true; + } else if tag == CANARY_TAG { + canary = true; + } else if tag == SERVES_ON_GPU_TAG { + serves_on_gpu = true; } } Self { @@ -124,6 +145,8 @@ impl ScenarioDecl { serve_timeout_secs, nightly, lifecycle, + canary, + serves_on_gpu, } } @@ -321,12 +344,19 @@ pub struct PlatformManifest<'a> { /// scenarios stay out of the fast path. `include_lifecycle` is set (via /// `E2E_INCLUDE_LIFECYCLE`) only when the caller opts into the expensive, /// OS-mutating release-lifecycle scenarios; the default fast suite keeps them out. +/// +/// `canary_mode` is set by CI on the MI300X lane of a pull_request (via +/// `E2E_PR_CANARY`): only the single `@canary` scenario serves as the pre-merge +/// smoke; every other GPU-serving scenario resolves to `Skip` so its resolution +/// is still recorded (valid platform.json / N/A column) without serving. The +/// full serve matrix runs in the merge queue with canary mode off. pub fn resolve( decl: &ScenarioDecl, cap: &HostCapability, matrix: &Expectations, include_nightly: bool, include_lifecycle: bool, + canary_mode: bool, ) -> Expectation { // (1) Applicability / skip. if decl.nightly && !include_nightly { @@ -339,6 +369,16 @@ pub fn resolve( reason: "lifecycle-only scenario; set E2E_INCLUDE_LIFECYCLE=1 to run".to_owned(), }; } + // In PR canary mode, only the @canary scenario serves; skip every other + // scenario that would real-serve on this GPU host — both `@requires-gpu` + // scenarios and `@serves-on-gpu` ones (untagged-for-GPU scenarios that still + // launch a real serve when a GPU is present, e.g. the chat behavioural + // scenarios). Purely non-GPU scenarios are unaffected — they still run. + if canary_mode && (decl.requires_gpu || decl.serves_on_gpu) && !decl.canary { + return Expectation::Skip { + reason: "non-canary GPU-serving scenario; skipped in PR canary mode".to_owned(), + }; + } if decl.requires_gpu && !cap.has_amd_gpu { return Expectation::Skip { reason: "requires an AMD GPU; none detected on this host".to_owned(), @@ -524,17 +564,17 @@ serve_timeout_secs = 90 let d = decl(&["id:big", "requires-gpu", "nightly"]); assert!(d.nightly); assert!(matches!( - resolve(&d, &cap("mi300x"), &m, false, false), + resolve(&d, &cap("mi300x"), &m, false, false, false), Expectation::Skip { .. } )); assert_eq!( - resolve(&d, &cap("mi300x"), &m, true, false), + resolve(&d, &cap("mi300x"), &m, true, false, false), Expectation::ExpectPass ); // The nightly gate is cheapest-first: a @nightly scenario that ALSO can't // run here (no GPU) still skips regardless of the include flag. assert!(matches!( - resolve(&d, &cap("mock"), &m, true, false), + resolve(&d, &cap("mock"), &m, true, false, false), Expectation::Skip { .. } )); } @@ -551,18 +591,77 @@ serve_timeout_secs = 90 ]); assert!(d.lifecycle); assert!(matches!( - resolve(&d, &cap("strix-ubuntu"), &m, false, false), + resolve(&d, &cap("strix-ubuntu"), &m, false, false, false), Expectation::Skip { .. } )); assert_eq!( - resolve(&d, &cap("strix-ubuntu"), &m, false, true), + resolve(&d, &cap("strix-ubuntu"), &m, false, true, false), Expectation::ExpectPass ); // Even when included, an inapplicable OS still skips (os gate is checked). assert!(matches!( - resolve(&d, &cap("strix-windows"), &m, false, true), + resolve(&d, &cap("strix-windows"), &m, false, true, false), + Expectation::Skip { .. } + )); + } + + #[test] + fn canary_tag_parses() { + let d = decl(&[ + "id:serve-default-engine-inference", + "requires-gpu", + "canary", + ]); + assert!(d.canary); + let no = decl(&["id:x", "requires-gpu"]); + assert!(!no.canary); + } + + #[test] + fn canary_mode_runs_only_the_canary_gpu_scenario() { + let m = Expectations::default(); + // The @canary scenario serves in canary mode (ExpectPass on MI300X vLLM; + // no matrix entry here so it resolves to a plain pass). + let canary = decl(&[ + "id:serve-default-engine-inference", + "requires-gpu", + "canary", + ]); + assert_eq!( + resolve(&canary, &cap("mi300x"), &m, false, false, true), + Expectation::ExpectPass + ); + // Every OTHER @requires-gpu scenario is skipped in canary mode — its + // resolution is still recorded (valid platform.json) but it does not run. + let other = decl(&["id:serve-vllm-inference", "requires-gpu"]); + assert!(matches!( + resolve(&other, &cap("mi300x"), &m, false, false, true), Expectation::Skip { .. } )); + // A @serves-on-gpu scenario (real-serves on a GPU host but not tagged + // @requires-gpu, e.g. the chat behavioural scenarios) is ALSO skipped in + // canary mode — otherwise it would launch an extra real serve on the PR. + let serves = decl(&["id:chat-end-to-end-local-model", "serves-on-gpu"]); + assert!(matches!( + resolve(&serves, &cap("mi300x"), &m, false, false, true), + Expectation::Skip { .. } + )); + // But with canary mode OFF it runs as usual on the GPU host. + assert_eq!( + resolve(&serves, &cap("mi300x"), &m, false, false, false), + Expectation::ExpectPass + ); + // Canary mode does not touch non-GPU scenarios — they still run. + let no_gpu = decl(&["id:serve-no-gpu-fails-fast", "requires-no-gpu"]); + assert_eq!( + resolve(&no_gpu, &cap("mock"), &m, false, false, true), + Expectation::ExpectPass + ); + // With canary mode OFF, the non-canary GPU scenario runs as usual. + assert_eq!( + resolve(&other, &cap("mi300x"), &m, false, false, false), + Expectation::ExpectPass + ); } #[test] @@ -585,17 +684,17 @@ serve_timeout_secs = 90 // MI300X: default engine vLLM → xfail. assert!(matches!( - resolve(&d, &cap("mi300x"), &m, false, false), + resolve(&d, &cap("mi300x"), &m, false, false, false), Expectation::ExpectXfail { .. } )); // Strix Ubuntu: gfx1151 → lemonade default → NOT vLLM → expect-pass. assert_eq!( - resolve(&d, &cap("strix-ubuntu"), &m, false, false), + resolve(&d, &cap("strix-ubuntu"), &m, false, false, false), Expectation::ExpectPass ); // Strix Windows: lemonade default → expect-pass (this is the XPASS fix). assert_eq!( - resolve(&d, &cap("strix-windows"), &m, false, false), + resolve(&d, &cap("strix-windows"), &m, false, false, false), Expectation::ExpectPass ); } @@ -605,7 +704,7 @@ serve_timeout_secs = 90 let m = eai7333_matrix(); let d = decl(&["id:serve-default-engine-inference", "requires-gpu"]); assert!(matches!( - resolve(&d, &cap("mock"), &m, false, false), + resolve(&d, &cap("mock"), &m, false, false, false), Expectation::Skip { .. } )); } @@ -616,16 +715,16 @@ serve_timeout_secs = 90 let d = decl(&["id:serve-no-gpu-fails-fast", "requires-no-gpu"]); // The mock host has no AMD GPU → the no-GPU premise applies → runs. assert_eq!( - resolve(&d, &cap("mock"), &m, false, false), + resolve(&d, &cap("mock"), &m, false, false, false), Expectation::ExpectPass ); // Every GPU host skips it — the premise can't hold there. assert!(matches!( - resolve(&d, &cap("mi300x"), &m, false, false), + resolve(&d, &cap("mi300x"), &m, false, false, false), Expectation::Skip { .. } )); assert!(matches!( - resolve(&d, &cap("strix-ubuntu"), &m, false, false), + resolve(&d, &cap("strix-ubuntu"), &m, false, false, false), Expectation::Skip { .. } )); } @@ -641,12 +740,12 @@ serve_timeout_secs = 90 ]); // MI300X: vLLM available → not skipped (expect-pass here, no matrix entry). assert_eq!( - resolve(&d, &cap("mi300x"), &m, false, false), + resolve(&d, &cap("mi300x"), &m, false, false, false), Expectation::ExpectPass ); // Strix Windows: vLLM can't start → skip (N/A). assert!(matches!( - resolve(&d, &cap("strix-windows"), &m, false, false), + resolve(&d, &cap("strix-windows"), &m, false, false, false), Expectation::Skip { .. } )); } @@ -660,15 +759,15 @@ serve_timeout_secs = 90 // Runs on a Linux GPU host; skips where os_family != linux (windows, and // the "other" fixture host). assert_eq!( - resolve(&d, &cap("strix-ubuntu"), &m, false, false), + resolve(&d, &cap("strix-ubuntu"), &m, false, false, false), Expectation::ExpectPass ); assert!(matches!( - resolve(&d, &cap("strix-windows"), &m, false, false), + resolve(&d, &cap("strix-windows"), &m, false, false, false), Expectation::Skip { .. } )); assert!(matches!( - resolve(&d, &cap("mock"), &m, false, false), + resolve(&d, &cap("mock"), &m, false, false, false), Expectation::Skip { .. } )); } @@ -678,11 +777,11 @@ serve_timeout_secs = 90 let m = Expectations::default(); let d = decl(&["id:examine-version"]); assert_eq!( - resolve(&d, &cap("mock"), &m, false, false), + resolve(&d, &cap("mock"), &m, false, false, false), Expectation::ExpectPass ); assert_eq!( - resolve(&d, &cap("mi300x"), &m, false, false), + resolve(&d, &cap("mi300x"), &m, false, false, false), Expectation::ExpectPass ); } @@ -701,11 +800,11 @@ reason = "short-name not surfaced" let d = decl(&["id:serve-short-name-expansion"]); // No requires-gpu → runs everywhere, always xfail. assert!(matches!( - resolve(&d, &cap("mock"), &m, false, false), + resolve(&d, &cap("mock"), &m, false, false, false), Expectation::ExpectXfail { .. } )); assert!(matches!( - resolve(&d, &cap("mi300x"), &m, false, false), + resolve(&d, &cap("mi300x"), &m, false, false, false), Expectation::ExpectXfail { .. } )); } @@ -728,12 +827,12 @@ reason = "lemonade vulkan fallback" ]); // Strix Ubuntu (linux, lemonade) → xfail. assert!(matches!( - resolve(&d, &cap("strix-ubuntu"), &m, false, false), + resolve(&d, &cap("strix-ubuntu"), &m, false, false, false), Expectation::ExpectXfail { .. } )); // Strix Windows (windows, lemonade) → os mismatch → expect-pass. assert_eq!( - resolve(&d, &cap("strix-windows"), &m, false, false), + resolve(&d, &cap("strix-windows"), &m, false, false, false), Expectation::ExpectPass ); } diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 43be1645..65ce1d72 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -753,6 +753,11 @@ async fn main() { // either its CLI filter OR this closure, so CLI selection would bypass OS, // nightly/lifecycle, ID, and expectation resolution entirely. let only_lifecycle = std::env::var_os("E2E_ONLY_LIFECYCLE").is_some_and(|v| v == "1"); + // PR canary mode: CI sets `E2E_PR_CANARY` on the MI300X lane of a + // pull_request so only the single `@canary` scenario serves as a fast + // pre-merge smoke; every other GPU scenario is skipped (full matrix runs in + // the merge queue). Off everywhere else. + let canary_mode = std::env::var_os("E2E_PR_CANARY").is_some_and(|v| v == "1"); eprintln!( "Host capability: platform={} os={} gpu={} effective_engine={}", cap.platform_slug, cap.os_family, cap.has_amd_gpu, cap.effective_serve_engine, @@ -821,7 +826,14 @@ async fn main() { .filter_run(concat!(env!("CARGO_MANIFEST_DIR"), "/features/"), { move |_feature, _rule, scenario| { let decl = ScenarioDecl::from_tags(&scenario.tags); - let expectation = resolve(&decl, cap, matrix, include_nightly, include_lifecycle); + let expectation = resolve( + &decl, + cap, + matrix, + include_nightly, + include_lifecycle, + canary_mode, + ); let run = (!only_lifecycle || decl.lifecycle) && !matches!(expectation, Expectation::Skip { .. }); if let Some(id) = &decl.id { From ffbbc03ef04388d29c4e03237dfb5d10cd5a0360 Mon Sep 17 00:00:00 2001 From: fredespi Date: Tue, 4 Aug 2026 14:41:14 +0200 Subject: [PATCH 2/2] =?UTF-8?q?ci:=20address=20serve-filter=20review=20?= =?UTF-8?q?=E2=80=94=20canary=20docs,=20cardinality=20test,=20gate=20order?= =?UTF-8?q?=20(EAI-7746)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the serve paths-filter + PR canary change, addressing review findings on the same PR: - Document @canary and @serves-on-gpu in the e2e-cucumber README tag table, and correct the now-stale "each job runs the whole suite" text and job table to reflect PR canary mode (MI300X runs only @canary) and the Strix lanes skipping on pull_request. - Add a unit test that parses the real .feature files and asserts exactly one @canary scenario, so a later refactor dropping/renaming the tag fails cheaply instead of silently emptying the pre-merge serve smoke. - Move the canary-mode skip after the host-applicability checks in resolve() so a scenario inapplicable for a hard reason (no GPU / wrong OS) reports that reason rather than the canary skip. - Make the merge_group gating comment honest: the Strix lanes are continue-on-error on merge_group too, so they are not a gating backstop — a regression surfaces at nightly, a deliberate trade for serial hardware. Signed-off-by: fredespi --- .github/workflows/ci.yml | 9 ++++ tests/e2e-cucumber/README.md | 48 ++++++++++++------- tests/e2e-cucumber/src/expectation.rs | 67 +++++++++++++++++++++++---- 3 files changed, 98 insertions(+), 26 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f685a5bc..8d22d847 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -967,6 +967,15 @@ jobs: # can still target it. The job is required-but-continue-on-error, so skipping # it on a PR reports the required check as skipped (branch protection # satisfied) without running the hardware. + # + # NOT a gating backstop: this lane is `continue-on-error` on merge_group too, + # so a Strix (lemonade / Windows) serve regression skips on the PR, fails + # non-blocking in the queue, and still lands on main — first hard signal is + # the nightly run, a day later, decoupled from the causing PR. Deliberate + # trade for scarce serial Strix hardware (a per-PR real Strix serve is the + # cost this change removes); the Strix lanes are proving-out and non-blocking + # by design. Drop `continue-on-error` on the merge_group path if/when they + # graduate to a true pre-merge gate. if: >- always() && needs.changes.result == 'success' diff --git a/tests/e2e-cucumber/README.md b/tests/e2e-cucumber/README.md index 77700135..62a58238 100644 --- a/tests/e2e-cucumber/README.md +++ b/tests/e2e-cucumber/README.md @@ -88,10 +88,14 @@ E2E_INCLUDE_LIFECYCLE=1 E2E_ONLY_LIFECYCLE=1 cargo xtask e2e ## Tags and per-scenario expectations -There is no tag-filter tiering. Each CI job runs the **whole** suite -(`cargo xtask e2e`, no `-t` filter); the harness resolves every scenario to -**pass / xfail / skip** at runtime from its capability tags plus the known-bug -matrix, then reconciles the actual result against that expectation. +There is no tag-filter tiering: a job runs `cargo xtask e2e` with no `-t` filter, +and the harness resolves every scenario to **pass / xfail / skip** at runtime from +its capability tags plus the known-bug matrix, then reconciles the actual result +against that expectation. Two env-gated modes narrow *which* scenarios actually +run (still via resolution, not a CLI filter): `E2E_PR_CANARY=1` on the per-PR +MI300X lane runs only the `@canary` serve (see the job table below), and +`E2E_INCLUDE_NIGHTLY` / `E2E_INCLUDE_LIFECYCLE` opt expensive scenarios in. Off +those modes (merge queue, push, nightly, dispatch) a job runs the whole suite. Scenarios carry stable-id and capability tags: @@ -104,6 +108,8 @@ Scenarios carry stable-id and capability tags: | `@serve-timeout:` | Lengthen the serve-readiness wait for a genuinely slow serve (e.g. a large model). | | `@nightly` | Expensive scenario skipped by default; included when `E2E_INCLUDE_NIGHTLY=1`. | | `@lifecycle` | Expensive, OS-mutating release-lifecycle scenario (packaging + real installer + install/uninstall). Skipped by default; included when `E2E_INCLUDE_LIFECYCLE=1`. `E2E_ONLY_LIFECYCLE=1` selects only this set without bypassing expectation resolution. | +| `@canary` | The single minimal real-serve scenario used as the per-PR pre-merge smoke on the MI300X lane. In canary mode (`E2E_PR_CANARY=1`) it is the **only** GPU-serving scenario that runs; every other one resolves to skip. Exactly one scenario must carry this tag (enforced by a unit test). | +| `@serves-on-gpu` | Marks a scenario that does a **real** `rocm serve` on a GPU host even though it is not `@requires-gpu` (its assertion is engine-agnostic, so it also runs mock-backed on the no-GPU lane). Needed so canary mode skips it too; without the tag it would launch an extra real serve on every PR canary run. | Known bugs are **not** tagged in the `.feature` files — they live in `expectations.toml`, keyed by `@id`, each with a `when = { ... }` condition (e.g. @@ -122,18 +128,28 @@ serve does not compete with the first for device memory, and the failure quotes the service log tail plus the device's free-VRAM state, which is where the engine's own reason for the stall is recorded. -CI runs one job per platform, each executing the full suite: - -| Job | Platform | Blocking | -|---|---|---| -| `e2e` | Mock (no GPU, GitHub-hosted) | yes | -| `e2e-gpu` | MI300X (self-hosted) | no | -| `e2e-gpu-strix-ubuntu` | Strix Halo / Ubuntu (self-hosted) | no | -| `e2e-gpu-strix-windows` | Strix Halo / Windows (self-hosted) | no | - -The blocking mock job passes when every applicable scenario is pass-or-xfail with -no XPASS or unexpected failure; the GPU jobs are non-blocking. The `e2e-report` -job consolidates all platforms' results into one cross-platform report. +CI runs one job per platform. Because real GPU serves are the wall-clock long +pole on scarce serial hardware, the heavy GPU lanes are narrowed on a +`pull_request` (see the `changes` job's `serve` paths-filter and the per-job +`if:`/`E2E_PR_CANARY` gating in `ci.yml`): + +| Job | Platform | On `pull_request` | On merge_group / push / dispatch | Blocking | +|---|---|---|---|---| +| `e2e` | Mock (no GPU, GitHub-hosted) | full suite | full suite | yes | +| `e2e-gpu` | MI300X (self-hosted) | `@canary` scenario only (canary mode) | full suite | no | +| `e2e-gpu-strix-ubuntu` | Strix Halo / Ubuntu (self-hosted) | skipped | full suite | no | +| `e2e-gpu-strix-windows` | Strix Halo / Windows (self-hosted) | skipped | full suite | no | + +Only serve-affecting PRs run the GPU lanes at all: they are gated on the `serve` +paths-filter, so a PR that can't touch serving skips the whole matrix. When a GPU +lane does run on a PR, only the MI300X `@canary` serve executes as a pre-merge +smoke; the two Strix lanes skip on PRs (their required checks report as skipped, +satisfying branch protection) and run the full suite in the merge queue and on +push. The blocking mock job passes when every applicable scenario is pass-or-xfail +with no XPASS or unexpected failure; the GPU jobs are non-blocking +(`continue-on-error`) on every trigger — including merge_group — so they surface +regressions without gating the queue. The `e2e-report` job consolidates all +platforms' results into one cross-platform report. The nightly workflow runs three non-blocking jobs — the existing MI300X job and new Strix Halo jobs on Ubuntu and Windows — with `E2E_INCLUDE_NIGHTLY=1`. The diff --git a/tests/e2e-cucumber/src/expectation.rs b/tests/e2e-cucumber/src/expectation.rs index 1a9d3772..4bcc3787 100644 --- a/tests/e2e-cucumber/src/expectation.rs +++ b/tests/e2e-cucumber/src/expectation.rs @@ -369,16 +369,6 @@ pub fn resolve( reason: "lifecycle-only scenario; set E2E_INCLUDE_LIFECYCLE=1 to run".to_owned(), }; } - // In PR canary mode, only the @canary scenario serves; skip every other - // scenario that would real-serve on this GPU host — both `@requires-gpu` - // scenarios and `@serves-on-gpu` ones (untagged-for-GPU scenarios that still - // launch a real serve when a GPU is present, e.g. the chat behavioural - // scenarios). Purely non-GPU scenarios are unaffected — they still run. - if canary_mode && (decl.requires_gpu || decl.serves_on_gpu) && !decl.canary { - return Expectation::Skip { - reason: "non-canary GPU-serving scenario; skipped in PR canary mode".to_owned(), - }; - } if decl.requires_gpu && !cap.has_amd_gpu { return Expectation::Skip { reason: "requires an AMD GPU; none detected on this host".to_owned(), @@ -396,6 +386,18 @@ pub fn resolve( reason: format!("requires os '{os}'; this host is '{}'", cap.os_family), }; } + // In PR canary mode, only the @canary scenario serves; skip every other + // scenario that would real-serve on this GPU host — both `@requires-gpu` + // scenarios and `@serves-on-gpu` ones (untagged-for-GPU scenarios that still + // launch a real serve when a GPU is present, e.g. the chat behavioural + // scenarios). Purely non-GPU scenarios are unaffected — they still run. This + // sits AFTER the host-applicability checks (no-GPU / OS) so a scenario that + // is inapplicable for a hard reason reports THAT reason, not the canary skip. + if canary_mode && (decl.requires_gpu || decl.serves_on_gpu) && !decl.canary { + return Expectation::Skip { + reason: "non-canary GPU-serving scenario; skipped in PR canary mode".to_owned(), + }; + } let engine = decl.effective_engine(cap); // A scenario that pins or defaults to a real engine and would actually serve // needs that engine to be startable here. We treat any GPU scenario with a @@ -664,6 +666,51 @@ serve_timeout_secs = 90 ); } + // Guards the canary mechanism against silent decay. `E2E_PR_CANARY` skips + // every GPU-serving scenario except the one `@canary`; if a later refactor + // drops or renames that tag, the per-PR MI300X lane would skip everything, + // still write a valid platform.json, reconcile clean, and go green in ~2min — + // silently testing nothing. Parse the real .feature files and assert exactly + // one scenario carries `@canary` so that regression fails a cheap unit test + // instead of quietly gutting the pre-merge serve smoke. + #[test] + fn exactly_one_canary_scenario_across_feature_files() { + let features_dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("features"); + let mut canary_ids: Vec = Vec::new(); + // A scenario's tags are the whitespace-separated `@tag` tokens on the + // line(s) immediately preceding its `Scenario:`. We only inspect lines + // whose first non-space char is `@` (real tag lines) — `#` comment lines + // that merely mention `@canary` in prose are ignored, and the tag line's + // own `@id:` gives us the scenario id for a useful failure message. + for entry in std::fs::read_dir(&features_dir) + .expect("read features dir") + .flatten() + { + let path = entry.path(); + if path.extension().and_then(|e| e.to_str()) != Some("feature") { + continue; + } + let text = std::fs::read_to_string(&path).expect("read feature file"); + for line in text.lines() { + let trimmed = line.trim_start(); + if !trimmed.starts_with('@') { + continue; + } + let tags: Vec<&str> = trimmed.split_whitespace().collect(); + let decl = ScenarioDecl::from_tags(&tags); + if decl.canary { + canary_ids.push(decl.id.unwrap_or_else(|| format!(" in {path:?}"))); + } + } + } + assert_eq!( + canary_ids.len(), + 1, + "expected exactly one @canary scenario, found {}: {canary_ids:?}", + canary_ids.len(), + ); + } + #[test] fn effective_engine_prefers_explicit_pin() { let d = decl(&["id:x", "requires-gpu", "requires-engine:vllm"]);