Skip to content

test(e2e): gate heavy GPU serves to the merge queue - #157

Open
fredespi wants to merge 1 commit into
mainfrom
e2e-task-8-gate-gpu-serve-matrix-to-merge-group
Open

test(e2e): gate heavy GPU serves to the merge queue#157
fredespi wants to merge 1 commit into
mainfrom
e2e-task-8-gate-gpu-serve-matrix-to-merge-group

Conversation

@fredespi

@fredespi fredespi commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

The full real-GPU serve matrix ran on every PR, making the per-PR GPU lanes slow. This gates the redundant heavy serves to the merge queue while keeping one real serve per engine on the PR path as a pre-merge canary, so a broken serve is still caught before a change enters the queue.

Changes

  • Add a @merge-queue scenario axis, mirroring the existing @nightly one: a new merge_queue field on ScenarioDecl, an include_merge_queue arg to resolve() with a skip branch, and an E2E_MERGE_QUEUE env read in the harness.
  • Tag the redundant default-engine and readiness serves (serve-default-engine-working-endpoint, serve-default-engine-inference, serve-readiness-contract) @merge-queue so they run only in the merge queue.
  • Keep serve-vllm-inference (vLLM) and serve-lemonade-inference (lemonade) untagged as per-engine PR canaries.
  • ci.yml sets E2E_MERGE_QUEUE on the merge_group event for all three GPU jobs. The jobs still run on pull_request and produce their required checks; only the work is trimmed.

Test plan

  • cargo fmt --check, cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings clean (Linux container).
  • cargo test --workspace + cargo test -p e2e-cucumber --lib green (64/64, incl. a new merge_queue_scenario_skips_unless_included unit test).
  • E2E mock lane reconciliation: 3 xfail / 0 XPASS / 0 unexpected.
  • Scoped MI300X dispatch verified the grid on hardware: scenarios 6/6b/8 resolve to skip on the PR path, canaries 5/7 run and serve; 0 unexpected failures.

The full real-GPU serve matrix ran on every PR, making the per-PR GPU
lanes slow. Add a @merge-queue scenario axis (mirroring @nightly) so the
redundant default-engine and readiness serves run only in the merge
queue, while one real serve per engine (vLLM, lemonade) stays on the PR
path as a pre-merge canary.

The GPU E2E jobs still run on pull_request and produce their required
checks; only the work is trimmed. ci.yml sets E2E_MERGE_QUEUE on the
merge_group event to opt those scenarios back in before a change lands.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
@fredespi
fredespi requested a review from a team as a code owner August 1, 2026 12:49
@rominf

rominf commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Automated review pass over the diff against main. No code changed. Verdict: approve with minor changes — one doc fix is worth landing before merge; the rest are judgement calls for you.

The mechanism itself checks out. Verified directly: the @merge-queue axis mirrors @nightly/@lifecycle exactly, the gate is evaluated cheapest-first (before the xfail matrix lookup), the empty-string env value is handled correctly by is_some_and(|v| v == "1"), merge_group: is a real trigger and the merge queue is active on main, the changes job forces heavy=true off-PR so the GPU jobs really do run in the queue, and cargo test -p e2e-cucumber --lib is green locally (56/56). tests/e2e-cucumber/tests/e2e.rs:828 is the only non-test call site of resolve().

Findings, most severe first

1. tests/e2e-cucumber/README.md:97-106 — stale tag table (worth fixing before merge).
The component's own tag-axis table lists @id, @requires-gpu, @requires-engine, @requires-os, @serve-timeout, @nightly, @lifecycle. This PR adds a peer axis without adding the row. Failure scenario: a contributor sees scenarios 6/6b/8 skipped on their GPU dispatch, finds no @merge-queue in the documented tag list, and either re-tags them or "fixes" the skip. (@requires-no-gpu is also missing — pre-existing, cheap to add while you're there.)

2. .github/workflows/nightly.yml:346,430,525 — nightly GPU lanes never set E2E_MERGE_QUEUE (advisory).
All three nightly GPU jobs set E2E_INCLUDE_NIGHTLY: "1" unconditionally but did not get the new var, so the heavy serves now run only on the merge_group event. The merge queue is confirmed active, so this is not a coverage hole today — it is a redundancy question. Failure scenario: if the merge-queue path is ever bypassed or broken, scenarios 6/6b/8 run nowhere and the nightly grid shows three quiet skip cells instead of surfacing the gap. One line per job restores the belt-and-braces; if omitting it is deliberate, a sentence in the PR body would make that clear.

3. .github/workflows/ci.yml:759-764 — the comment says "Opt-in @merge-queue serves", but there is no way to opt in.
E2E_INCLUDE_NIGHTLY has a workflow_dispatch input (include_nightly, ci.yml:41-44) precisely so a scoped dispatch can probe one heavy scenario. @merge-queue has no equivalent. Failure scenario: scenario 8 regresses in the queue and there is no way to reproduce it on hardware via dispatch — you have to push a branch and get it queued, or hand-edit the workflow. Mirroring include_nightly with an include_merge_queue boolean input would keep the two axes symmetric and make the comment true.

4. The PR description's framing overstates the safety net (pre-existing mechanism, but this change leans on it).
All three GPU e2e jobs are continue-on-error: true, and xtask e2e-report returns Ok(()) regardless of unexpected failures (xtask/src/e2e_report.rs:47-59) — while E2E tests (GPU), E2E tests (Strix Halo, Ubuntu), E2E tests (Strix Halo, Windows) and E2E consolidated report are all required contexts on main (verified via the branch-protection API). So a failing serve reports green and blocks nothing, on either path. Failure scenario: "a broken serve is still caught before a change enters the queue" reads as gating, but both paths are advisory, and the merge-queue results land in a run nobody looks at once the PR has merged. Not introduced here — but the rationale would be more accurate as "advisory signal on the PR, full advisory matrix in the queue".

5. tests/e2e-cucumber/src/expectation.rs:339-346resolve() now takes three adjacent bare bools.
include_nightly, include_lifecycle, include_merge_queue are positionally interchangeable and a transposition compiles silently; roughly 35 unit-test call sites pass bare literals. Failure scenario: a future fourth axis is inserted at the wrong position, or a test literal is swapped, and merge_queue_scenario_skips_unless_included — which exists specifically to prove the nightly and merge-queue axes are independent — asserts the wrong thing while still passing. The codebase already prefers named-options structs for multi-flag inputs (RunnerOptions, FixOptions, ComfyUiInstallOptions, DemoOptions). Cross-PR note: #155, #156 and #157 are each independently growing this trailing-bool list — a shared ScenarioSelection/RunMode struct is better done once across the three than three times.

6. tests/e2e-cucumber/src/expectation.rs:359 — skip reason is imprecise.
"merge-queue-only scenario; set E2E_MERGE_QUEUE to run", but the harness requires the value to be exactly "1" (tests/e2e-cucumber/tests/e2e.rs:756); the sibling lifecycle message correctly says set E2E_INCLUDE_LIFECYCLE=1 to run. Failure scenario: someone exports E2E_MERGE_QUEUE=true, the scenario still skips, and the message they just followed told them they did it right.

Checked and clean

  • xfail reconciliation: scenarios 6/6b/8 do have live xfail rows in expectations.toml, but resolve() returns Skip before the matrix lookup, so CellOutcome::Skip and is_problem() == false (crates/e2e-report/src/lib.rs:659-684). No spurious stale-xfail or XPASS. The advisory xfail-hint script is text-only and unaffected.
  • merge_group path filtering: paths-filter is pull_request-only (ci.yml:92) with forced=true off-PR (ci.yml:150-153), so no base-ref ambiguity and no docs-only skip in the queue.

Worth reinforcing

  • Placing the merge-queue gate with the other applicability gates, ahead of the matrix, is the right ordering, and it is explicitly tested for cheapest-first behavior on a no-GPU host.
  • Keeping one untagged real serve per engine rather than dropping all GPU serves from the PR path is a good cost/coverage trade, and the feature-file comments explain the canary role right where a future editor will read it.
  • The new unit test asserts axis independence (nightly on, merge-queue still skipped), which is the non-obvious property worth pinning.

Not verified

  • The GPU suite was not run here; the hardware-dispatch evidence in the description is taken at face value.

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