Repository navigation
ci(e2e): move the AIM serve scenarios off the per-PR Strix Ubuntu lane - #852
Conversation
juhovainio
left a comment
There was a problem hiding this comment.
I read through the change and found nothing that blocks the move itself: tagging serve-26/27 @nightly, the raised nightly cap and the docs update all match the PR description. I'm requesting changes for three smaller points that I think should be settled before merge, left as inline comments:
- The opt-in half of the new unit test only asserts when
resolvereturns a Skip, so it doesn't actually pin that the scenarios are still reachable when nightly is opted in. - Per-PR AIM serve coverage goes away, and the PR itself says it's unconfirmed whether the nightly Strix host has Docker. If it doesn't, serve-26/27 skip every night. Worth confirming, or filing a follow-up, and the docs could carry the same caveat the workflow comment does.
- The
inputsexpression in job-leveltimeout-minutesshould resolve to 45 on per-PR runs; the first CI run here is the check.
I also haven't seen the new test shown to fail with the tags removed (I only read it, didn't run it), so a quick mutation check before merge would be good. CI was still pending on the E2E lanes when I looked; the rest was green.
| nightly: true, | ||
| ..Included::default() | ||
| }; | ||
| if let Expectation::Skip { reason } = resolve(&d, &cap("strix-ubuntu"), &m, opted_in) { |
There was a problem hiding this comment.
The opt-in half only asserts when resolve returns a Skip, so a Skip for another reason or any non-Skip outcome passes. Consider also asserting Run for a capability set that has Docker and a supported GPU, so "still reachable when opted in" is actually pinned.
There was a problem hiding this comment.
Fixed in a79fbff. The test now asserts the positive with an injected opt-in and capability set, through the harness's own resolve: with the opt-in set, serve-26/27 never skip for the nightly reason; on a host that lists aim they must be ExpectPass when Docker answers, and otherwise skip on "engine 'aim' cannot start"; any other outcome panics. A host that does not list aim must skip on the engine reason. It fails with the @nightly tags removed and with the opt-in forced off (both mutations shown in the description). One limit: engine_available("aim") calls the real docker info, so the pass branch only executes on a host with Docker; I ran the skip branch locally.
| e2e-gpu-nightly-strix: | ||
| name: E2E tests (Strix Halo, incl. nightly-only) | ||
| timeout-minutes: 90 | ||
| # 120: serve-26/27 (AIM, @nightly) add up to 2 x 1200s readiness waits when |
There was a problem hiding this comment.
The comment hedges on whether this host has Docker, and the PR body says it's unconfirmed. If it doesn't, serve-26/27 skip every night here and nothing runs AIM serve except a manual include_nightly dispatch. Worth confirming before merge, or filing a follow-up for a pool-based AIM nightly. docs/testing.md and the feature comment could carry the same caveat.
There was a problem hiding this comment.
Right. Nothing in nightly.yml or its scripts installs or checks Docker, so it cannot be settled from the repo. I added a step before the E2E run that runs docker info (the harness's own probe) and prints a ::warning:: when Docker is unreachable, since the job is continue-on-error and a silent skip would otherwise never show. The caveat is also in docs/testing.md and the feature comment, and the description lists the follow-up if the warning shows: a pool-based AIM nightly (or moving this lane onto the pool).
| # An include_nightly dispatch also runs the AIM serve scenarios (serve-26/27, | ||
| # @nightly, up to 1200s readiness each on a cold pull) and the large-model | ||
| # serve, which cannot fit the per-PR 45min budget. | ||
| timeout-minutes: ${{ inputs.include_nightly && 105 || 45 }} |
There was a problem hiding this comment.
Please confirm the first run on this PR accepts the inputs expression in job-level timeout-minutes (per-PR should resolve to 45).
There was a problem hiding this comment.
Checked the documentation rather than guessing: GitHub's context-availability table lists inputs for jobs.<job_id>.timeout-minutes, and on pull_request, merge_group and push there are no dispatch inputs, so inputs.include_nightly is empty and the expression resolves to 45 (the same job already uses inputs.include_nightly && '1' || '' at env level on those events). The first run of this PR on the workflow is the final check, and the per-PR Strix Ubuntu job's timeout-minutes should show 45 there.
nowycondro
left a comment
There was a problem hiding this comment.
🤖 Automated review (Claude Code) · ef9d8b7
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Review — needs work
Full review of the whole change.
No ticket named in the PR title or body. The diff matches what the description claims it does (tag serve-26/serve-27 @nightly, raise two job timeouts, document it, add a pinning unit test) — except for one specific claim about the new test's behavior, see Blocking below.
Blocking
-
The new test's "reachable once opted in" half never actually passes or fails on that condition — it's vacuous —
tests/e2e-cucumber/src/expectation.rs:1077-1084, fixture attests/e2e-cucumber/src/expectation.rs:711-717,tests/e2e-cucumber/src/capability.rs:171-179
cap("strix-ubuntu")'savailable_enginesis["lemonade", "vllm"]— it never contains"aim".HostCapability::engine_available("aim")requiresself.available_engines.iter().any(|e| e == "aim")as its first, short-circuiting condition, so it is unconditionallyfalsefor this fixture regardless ofdocker_available()or GPU family. Tracingresolve(): the nightly check (decl.nightly && !include_nightly) passes onceopted_in.nightly = true, but execution then reaches therequires_gpu && !cap.engine_available(engine)check lower inresolve(expectation.rs:604-611), which returnsSkip{reason: "engine 'aim' cannot start on this host (strix-halo)"}— every time, opted in or not. The test'sif let Expectation::Skip { reason } = resolve(...) { assert!(!reason.contains("E2E_INCLUDE_NIGHTLY"), ...) }is trivially satisfied by this unrelated reason string, so it never actually confirmsserve-26/serve-27resolve toExpectPass(or anything reachable) once the opt-in is set. This directly contradicts the PR description's own claim that the test "pins that both skip by default and are not skipped once the opt-in is set" — it only pins the first half. The test would also not go red under the "replace one branch of the production logic" mutation AGENTS.md §3 asks every new test to be checked against (e.g. ifresolve's nightly-disable branch were broken to re-skip the opted-in case for an unrelated reason, this assertion still passes).
Confidence 90 · logic · Fix: add"aim"tocap("strix-ubuntu")'savailable_enginesand replace theif let Skipwith a directassert_eq!(resolve(&d, &cap("strix-ubuntu"), &m, opted_in), Expectation::ExpectPass, "{prefix}"), mirroring the strong-assertion style already used elsewhere in this file (e.g. the merge-queue opt-in test). Note the fixture fix alone isn't quite sufficient:docker_available()is a live, unmockabledocker infosubprocess call, so even with"aim"added to the fixture, this half of the test would only pass on a CI runner that actually has a reachable Docker daemon — worth a comment or an injectable seam so the test is deterministic rather than silently environment-dependent. -
CI is red on the reviewed commit: "E2E tests (Strix Halo, Ubuntu, nightly, current)" failed — check run on
ef9d8b7b, started 2026-11-03T11:00:01Z
Failed in 3 seconds duringactions/checkoutwith##[error]No space left on deviceon the ephemeral DevLab runner. This reads as an infrastructure flake (disk space on the pool host) unrelated to this diff's content, not something this change needs to fix in-tree, but it is a currently-red required check.
Confidence 100 · ci · Fix: rerun; escalate to whoever maintains the ephemeral pool runners if it recurs (per this repo's own CI-ownership guidance to drive checks to green and handle infra flakes by rerun/escalation). -
CI is red on the reviewed commit: "E2E tests (Strix Halo, Windows, current)" failed — check run on
ef9d8b7b, started 2026-11-03T10:34:52Z, completed 10:49:50Z
FAIL: 'runtime-resolve-canonical-release' was expected to pass on this host but FAILED — a regression.That scenario is unrelated tomodel_serving.feature/AIM serving and this diff touches neither the runtime-resolve code nor its scenario, so this looks pre-existing/unrelated to this change — but it is still a currently-red required check blocking merge until resolved, rerun, or explicitly waived.
Confidence 100 · ci · Fix: confirm whether this is a known flake or an active regression onmainbefore merging; if pre-existing, it should be tracked separately rather than attributed to this PR.
Non-blocking
None
Decisions for the author
- Moving AIM serve/restart off every per-PR and merge-queue run, onto nightly-only (and only where Docker is actually installed) or a manual dispatch — tradeoff
This trades immediate, every-PR detection of an AIM-path regression for a reliable per-PR CI budget. Detection now depends on the nightly Strix Halo lane actually having Docker (the PR's own text says this is unconfirmed) or someone remembering to run aninclude_nightlydispatch. The PR already names two ways to close this (move the nightly Strix Ubuntu lane onto the pool, or add a small AIM-only nightly job there) — worth deciding now rather than finding out from a missed regression later.
Positive signals
model_serving_tags()(expectation.rs:1046) parses the tag line straight out of the realmodel_serving.featureviainclude_str!rather than hardcoding a tag list, so the test and the feature file's actual tag set structurally cannot drift on which tagsserve-26/serve-27carry.- The new 105/120-minute budgets are built from specific, cross-checkable figures (the ~25-minute measured base, the existing
@serve-timeout:1200tag, and the large-model-serve time independently stated elsewhere in the same workflow file) rather than round-number guesses. - The
inputs.include_nightlyreference the newtimeout-minutesexpression relies on is not a new pattern: the same input already drivesenv:values andif:conditions elsewhere in this exact workflow, across every trigger type it has (pull_request,merge_group,workflow_dispatch). That in-repo precedent, plus CI actually dispatching and running the job on this PR without a workflow-parse failure, is strong evidence the expression is valid and evaluates as intended — the one thing the PR's own "Not verified" section flagged as open.
Deployment notes
- If the nightly Strix Halo static host turns out to lack a working Docker daemon,
serve-26/serve-27will silently skip every night, and the only path that reliably exercises AIM serving becomes a manualinclude_nightlydispatch. Confirm Docker is actually installed there before relying on "nightly" as the safety net the PR's description assumes. - AIM serve/restart behavior is no longer exercised by any per-PR or merge-queue run as of this change; a regression in that path surfaces only via the nightly or manual lane.
What this covered
Full diff origin/main...pr/852 (merge-base c6ab82b5, 1 commit, 5 files, +70/-4) at head ef9d8b7b8191f33902b70500f26683f1b76eca9c, fetched read-only (pull/852/head) and never checked out. Every changed file read completely at the PR head, plus supporting non-diff code needed to judge them: tests/e2e-cucumber/src/capability.rs (HostCapability::engine_available, docker_available, family_prefers_vllm), the full resolve() function and Included/Expectation/cap() definitions in expectation.rs, and the other self-hosted jobs in e2e-selfhosted.yml/nightly.yml (W7900, MI300X, Windows, WSL2) to check whether the timeout/opt-in wiring was consistent across lanes. Two independent fan-out workers covered the two coupled file-groups (tests+feature file; workflows+docs); their candidate findings were individually re-verified against the source directly by re-reading the same functions before being scored here, and one candidate (a CI-style timeout-minutes validity concern) and one (a nightly.yml 90→120 timeout-arithmetic gap, explained once the job's "pre-warmed static host" context was read) were investigated and dropped as not reaching the confidence bar.
Did not run: cargo test/cargo nextest — no Rust toolchain available in this environment; the primary test-coverage finding was established by direct line-by-line tracing of resolve, engine_available, and the cap()/docker_available() fixtures against the new test's code, not by executing it. Prior-changes pass (review comments on earlier changes to these exact files/lines) was not separately searched beyond this file's own git log/git blame, which surfaced only the original AIM-scenario authorship commit (#708) already referenced in this PR's own comments. No ticket identifier was named, so no tracker lookup was attempted. CI was read directly from GitHub for commit ef9d8b7b (not inferred from the workflow files): one additional job, "E2E tests (Strix Halo, Ubuntu, release, current)" — the exact job whose timeout-minutes this PR edits — was still in_progress as of this review and had not yet confirmed the per-PR value resolves to 45; everything else not already named above was green.
One methodological note: the claim-check this task required before the review began (confirming no prior automated review already covers this commit) necessarily fetched the PR's existing reviews and full description first, which surfaced the text of an existing human review (CHANGES_REQUESTED) and the PR body before this review's blind code-reading phase started — this sits in tension with this method's normal blind-findings ordering. Every finding above was nonetheless independently re-derived and verified directly against the source (traced through resolve/engine_available/the fixtures, and through live CI data), not taken on that review's wording, and in one respect (the timeout-minutes expression validity) this review reaches a different, more confident conclusion than that earlier one. Not otherwise reconciled against that review's specific wording or any other discussion on this PR.
🤖 Generated with Claude Code
The self-hosted `E2E tests (Strix Halo, Ubuntu, ...)` jobs have been cancelled at their 45 minute cap on every run that contains the AIM engine (84f951e). Before it the same lane passed in 25-26 minutes, and runs whose merge ref lacked it still did. Cause: serve-26 and serve-27 are the last two of 183 scenarios in the suite and are tagged `@requires-engine:aim @serve-timeout:1200`, so each may wait up to 20 minutes for readiness. The pool runner is cold on every job, so the AIM image and a 9B checkpoint are pulled each time. About 25 minutes for the other scenarios plus serve-26's wait lands on the 45 minute cap with serve-27 just starting; the cancelled run's command log ends inside serve-27. The W7900 and MI300X lanes never run them (the harness needs Docker and a Radeon/Strix Halo gfx family), so only this lane was affected. A lane failure drops a merge-queue entry. Fix: tag both scenarios `@nightly` (the existing opt-in, so ids and numbering are unchanged), pin that with a unit test, and give the lanes that do opt in room for them: the nightly Strix lane goes from 90 to 120 minutes and the per-PR Strix Ubuntu job gets 105 minutes when an `include_nightly` dispatch is run. Per-PR runs keep the 45 minute cap. The unit test also pins the opt-in side: with the nightly opt-in set, the scenarios never skip for the nightly reason; on a host that lists aim they run exactly when Docker answers, and otherwise skip on the engine. AIM coverage still needs a nightly host with Docker, which is unconfirmed: the nightly Strix job now logs whether Docker is reachable, and the caveat is in docs/testing.md and the feature comment. Signed-off-by: Eugene Volen <eugene.volen@amd.com>
ef9d8b7 to
a79fbff
Compare
|
Pushed a79fbff (rebased onto main). All three points are addressed (threads), including the mutation check you asked for: the test fails with the |
The three points are addressed in a79fbff (test now pins the opt-in path with a mutation check; the Docker caveat is documented and now logged by the nightly job; the expression is confirmed against the documentation). See the replies. A re-review is welcome.
nowycondro
left a comment
There was a problem hiding this comment.
🤖 Automated review (Claude Code) · a79fbff
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Review — needs work
Full review of the whole change.
No ticket named in the PR title or body (it cites prior issue #708 as context for the root cause, not as something this PR closes).
Blocking
-
e2e-gpu-strix-windowswas not given the same nightly-dispatch timeout fix as its Ubuntu sibling, and its own comment now reads as false —.github/workflows/e2e-selfhosted.yml:576(comment "Matches e2e-gpu-strix-ubuntu's raised cap"),:580(timeout-minutes: 45, unchanged),:610(E2E_INCLUDE_NIGHTLYwired identically to the Ubuntu job)
This job runs on the samestrix-halo(gfx1151) hardware and wiresinclude_nightlythe same way ase2e-gpu-strix-ubuntu. TracedHostCapability::engine_available(tests/e2e-cucumber/src/capability.rs:164-180): the"aim"arm checks engine-list membership,docker_available(), and GPU family — there is no Windows exclusion (unlike"vllm", which explicitly bails on Windows). So aninclude_nightlydispatch withplatform: strix-windows(or the defaultplatform: all) opts serve-26/27 in on this lane too, and if Docker is reachable there, this job can hit the exact 45-minute cliff this PR exists to fix — except its cap was left at a flat 45 while Ubuntu's became conditional. The comment claiming parity is now objectively wrong post-diff.
Confidence 82 · logic · Fix: apply the same${{ inputs.include_nightly && 105 || 45 }}pattern here (or confirm Docker/aim genuinely cannot run on this host and say so in the comment instead of claiming parity that no longer exists). -
The 105-minute Strix Ubuntu budget accounts for only 3 of the scenarios that actually qualify to run under its own fix —
.github/workflows/e2e-selfhosted.yml:423-426(comment basis: "~25 base + 2×20 AIM + large-model-serve = 105"), scenario tags attests/e2e-cucumber/features/runtime_setup.feature:13,227,353(runtime-01,runtime-12,runtime-lemonade-backend-alignment-opt-out— all@requires-gpu @nightly, none@requires-engine:vllm),tests/e2e-cucumber/features/therock_next_generation.feature:81,100,tests/e2e-cucumber/features/comfyui.feature:322
Enumerating every@nightlytag in the suite and checking each againstengine_available/requires_os/requires_bare_metalfor thestrix-ubuntufixture (available_engines:["lemonade","vllm"], linux, bare-metal, gfx1151 → lemonade always available, vllm unavailable on this family): at leastruntime-01(SDK install),runtime-12(SDK reinstall),runtime-lemonade-backend-alignment-opt-out,therock-next-07(live install),therock-next-06, andcomfyui-install-preserves-the-rocm-runtime(bare-metal Linux install) also resolve toExpectPasson this exact host underinclude_nightly— none is@requires-engine:vllmor@requires-os:windows, which is what would exclude them. These are real install/download scenarios, not the ~8-second one the file itself singles out as cheap (runtime_setup.feature:120). The 105-minute figure, and the matchingdocs/testing.md:742-751paragraph, name only serve-26/27 and "the large-model serve" — the manualinclude_nightlydispatch this PR calls out as the fallback safety net for AIM coverage risks the same timeout cliff it was built to avoid.
Confidence 73 · logic · Fix: widen the budget comment's accounting to the full@nightly-eligible set for this host (or verify empirically via one real dispatch) before relying on 105 minutes as sufficient headroom.
Non-blocking
-
model_serving_tagsduplicates the file's existing, more robustfeature_tagshelper —tests/e2e-cucumber/src/expectation.rs:1046-1057vs.feature_tagsat:1358-1373(used by an existing test in this same file against this same feature file)
feature_tags(feature, id)locates a scenario's tag line by its stable@id:slug, independent of physical position. The new helper instead takeslines[at - 1]relative to theScenario:line, assuming the tag line is always the single line directly above with no separator — true today (verified againstmodel_serving.feature) but more fragile and a needless duplication of a pattern the file already has.
Confidence 85 · architectural · Fix: callfeature_tags(feature, "serve-aim-inference")/feature_tags(feature, "serve-aim-lifecycle-parity")instead of adding a new helper. -
Stray run of whitespace in a new assertion's panic message —
tests/e2e-cucumber/src/expectation.rs:1097:"...aim listed and Docker reachable: {reason}"
Confidence 100 · mechanical · Fix: collapse to a single space. -
The new test spawns a live, unmockable
docker infosubprocess twice per iteration (once insideresolve()→engine_available, once directly viadocker_available()atexpectation.rs:1090-1094) to decide which branch to assert
This is the first test in the file to reach a realdocker_available()call (every other docker-gated test passesIncluded{docker:false,..}and short-circuits first). Matching on the live result rather than a fixed expected value means theExpectPassbranch is only ever positively exercised on a host with a reachable Docker daemon — in a plaincargo testrun without one, only theSkipbranch runs. Calling it twice also leaves a narrow window where a flapping daemon could answer differently between the two calls. The author's own PR description already discloses the single-call version of this limitation ("I ran the skip branch locally"), so this is refinement rather than a new gap.
Confidence 75 · mechanical · Fix: calldocker_available()once, store it, and use the same value both to predict the branch and to assert against.
Decisions for the author
- Moving AIM serve/restart off every per-PR and merge-queue run, onto nightly-only or a manual
include_nightlydispatch — tradeoff
Both of the PR's own named fallbacks carry open risk: the nightly Strix Halo host's Docker presence is unconfirmed, and its new "Report whether Docker is reachable" step (nightly.yml:778-786) sits inside a job withcontinue-on-error: true(nightly.yml:723) — a::warning::there doesn't fail anything, and the consolidated report renders an expected skip as a bare glyph with no reason, so confirming AIM coverage ran requires someone to manually open that job's log. Combined with the budget gap above on the other fallback, detecting an AIM regression currently depends on more manual follow-through than the description implies. Worth deciding now whether that's acceptable or whether one of the two already-named follow-ups (pool-based nightly lane, dedicated AIM nightly job) should land first.
Positive signals
- The previously-flagged vacuous assertion is genuinely fixed: pushing
"aim"onto a cloned capability (expectation.rs:1088) rather than mutating the sharedcap()fixture is surgical and makes the opted-in branch actually depend on Docker reachability and GPU family, without touching any other test's fixture. aim_serve_scenarios_stay_off_the_per_pr_lanesderives its tag check from the realmodel_serving.featureviainclude_str!rather than a hand-copied tag list, so it structurally cannot drift from the file's actual tags.- Restraint shown on the lanes that didn't need a change: MI300X (AIM excluded by GPU family) and W7900 (already ran serve-26/27 unconditionally, pre-existing budget) were correctly left untouched rather than blanket-bumped.
docs/testing.md's new paragraph matches the code precisely and states the Docker-presence caveat plainly rather than overclaiming coverage.inputs.include_nightlyin the new job-leveltimeout-minutes:expression is declaredtype: booleanand the sameinputs.include_nightly && X || Ypattern already runs today in this file'senv:blocks across every trigger type this job has — strong static evidence the expression resolves as intended, closing out what the PR's own "Not verified" section flagged as open.
Deployment notes
- Nightly Strix Halo Docker presence remains unconfirmed; if absent, serve-26/27 skip silently every night and the
include_nightlyworkflow_dispatch becomes the only path — which itself may not fit in 105 minutes (see Blocking). - AIM serve/restart behavior is no longer exercised by any per-PR or merge-queue run as of this change.
- At review time the "E2E self-hosted" workflow run for this exact commit (
a79fbff7) was still queued/pending (no jobs started) — not failed, but unresolved; every other check (25 check-runs plus the Read the Docs status) was green.
What this covered
Full diff 6b175ee...a79fbff7 (merge-base 6b175eeeb1e881d4b293b9670894fb1811f61de1, 1 commit, 5 files, +123/-4), fetched read-only (pull/852/head) and never checked out. Every changed file read completely at the PR head — tests/e2e-cucumber/src/expectation.rs, tests/e2e-cucumber/features/model_serving.feature, .github/workflows/e2e-selfhosted.yml, .github/workflows/nightly.yml, docs/testing.md — plus supporting non-diff code: the full tests/e2e-cucumber/src/capability.rs (engine_available, docker_available, family_prefers_vllm), the full resolve()/Included/Expectation/cap() definitions, every other self-hosted job in both workflow files (W7900, MI300X, Windows, WSL2, nightly Strix), and every @nightly-tagged scenario repo-wide (enumerated across all .feature files) to check what else qualifies to run under include_nightly on Strix Ubuntu. Two independent fan-out workers covered the two coupled file-groups (tests+feature file; workflows+docs); every candidate they returned was independently re-verified against the source directly before being scored here, and one candidate (a documentation-accuracy point about the nightly.yml 90→120 comment's causal framing) was investigated and dropped below the confidence bar.
Did not run: cargo test/cargo nextest/clippy — no Rust toolchain available in this environment; findings were established by direct tracing of resolve, engine_available, and the feature-file tag set against the new test's code, not by executing it. History/prior-changes pass was limited to git log on the changed files, which surfaced only the original AIM-scenario authorship commit (#708) already referenced in this PR. No ticket identifier was named, so no tracker lookup was attempted. CI status was read directly from the GitHub API for commit a79fbff7 (check-runs and combined status), not inferred from workflow files.
This PR previously carried an automated review at an older commit (ef9d8b7b) that has since been superseded by a rebase; its findings are not assumed fixed here — this round re-derived everything independently from the current head and reached the two blocking findings above on its own. Not reconciled against that review's or the existing human review's specific wording, or against any other discussion on this PR.
🤖 Generated with Claude Code
juhovainio
left a comment
There was a problem hiding this comment.
Looks good. Moving the AIM serve scenarios off the per-PR lane and onto the nightly/opt-in path is cleanly done: the test resolves through the harness's own resolve and reads the feature file, the Docker caveat is stated openly, scenario IDs are unchanged, and docs are in sync. CI is green.
Nothing blocking. A few non-blocking notes:
- Inline: the assert message in
expectation.rshas a long run of literal spaces (see inline comment). - r9700 / w7900 nightly jobs: they set
E2E_INCLUDE_NIGHTLYbut keep the 90 min cap and get no Docker probe. If those hosts do have Docker, serve-26/27 now run there too. The PR says they don't; worth confirming. include_nightlyinput description ine2e-selfhosted.ymldoesn't mention the AIM scenarios now gated behind it.- Coverage gap: AIM now has no per-PR coverage. If the nightly Strix host has no Docker, the scenarios skip every night and the only signal is a
::warning::on acontinue-on-errorjob. Is that signal enough, or should a skip be surfaced more loudly? - The new test calls the real
docker info, so the ExpectPass branch only runs on a Docker host. The assertions are consistent on both branches, so this is fine; just noting it. - The 105/120 min caps are estimates; worth revisiting once there are real nightly timings.
| Expectation::Skip { reason } => { | ||
| assert!( | ||
| !docker_available(), | ||
| "{prefix} skipped with the opt-in set, aim listed and Docker reachable: {reason}" |
There was a problem hiding this comment.
Nit: this message literal contains a long run of spaces between "Docker" and "reachable" (looks like a collapsed \ line continuation). Rejoin with a single space.
Problem
The self-hosted
E2E tests (Strix Halo, Ubuntu, release, current)and... nightly, currentjobs hit their 45-minute cap on essentially every run since the AIM engine landed (#708). A merge-queue entry is dropped when a lane fails, so this blocks other PRs from merging too (see the heads-up on #708).Cause
Before the AIM change the lane passed in 25-26 minutes (runs between 2026-10-28 and 2026-10-29, for example on release/v0.2.1). Every run whose merge includes it is cancelled at 45 minutes.
serve-26andserve-27are the last two scenarios on the lane and both carry@serve-timeout:1200. The pool runner starts cold for every job, so the AIM image and a 9B checkpoint are pulled each time. About 25 minutes for the rest of the suite plusserve-26's readiness wait reaches the cap withserve-27just starting: in the artifact of a cancelled run (started 07:46, cancelled 08:32), the command log holds 183 scenarios and ends insideserve-27after both launchedrocm serve ... --engine aimwith rc 0. The W7900 and MI300X lanes never run them, because the harness requires Docker and a Radeon or Strix Halo GPU (HostCapability::engine_available's "aim" arm).What is proven: the pass-to-cancel flip tracks the AIM merge, and the cancelled run ends in the AIM scenarios. What is inferred: that
serve-26itself used most of its 20-minute readiness cap. The cancelled job has no logs and the artifact has no timestamps, so this is not directly measured.Change
serve-26andserve-27@nightly, so they run only whenE2E_INCLUDE_NIGHTLYis set, on a host with Docker and a supported GPU. Ids and numbering are unchanged (feature_namingstill passes), and the feature comment says why.tests/e2e-cucumber/src/expectation.rs,aim_serve_scenarios_stay_off_the_per_pr_lanes, reads the feature file and pins both halves through the harness's ownresolve, with an injected host and opt-in: withoutE2E_INCLUDE_NIGHTLYboth scenarios skip for the nightly reason; with it they never skip for that reason, and on a host that lists aim they run exactly when Docker answers, otherwise they skip on the engine. It fails with the@nightlytags removed ("serve-26 - must be @nightly") and with the opt-in forced off ("wrong skip reason with the opt-in set"). Limitation:engine_available("aim")calls the realdocker info, so the run-path branch executes only on a host with Docker; I ran the skip branch locally.${{ inputs.include_nightly && 105 || 45 }}, so per-PR runs keep 45 and only aninclude_nightlydispatch gets the longer cap. The 105 and 120 are estimates: about 25 minutes for the rest, up to two 20-minute AIM waits, plus the existing large-model serve.docs/testing.mdsays where the AIM scenarios now run, with the caveat below.docker info, the same probe the harness uses, and prints a::warning::if Docker is unreachable. The job iscontinue-on-error, so a silent skip would otherwise never show; the line answers the question below on the first nightly run.Why not raise the per-PR cap instead: every PR would wait 20+ extra minutes for scenarios that depend on a cold pull, and it only moves the cap. A dedicated AIM job would work too but needs build, prewarm and report plumbing that the existing
@nightlymechanism avoids.Scenario coverage
serve-26andserve-27already cover AIM serving and restart; this changes when they run, not what they assert. They now run on the nightly Strix Halo lane, or on aninclude_nightlydispatch ofe2e-selfhosted.ymlwith platformstrix-ubuntu.Coverage gap to decide: the nightly Strix Halo lane runs on the static host, and I could not confirm Docker is installed there. If it is not, the AIM scenarios skip silently every night, and the only guaranteed place is the manual
include_nightlydispatch on the pool. Moving the nightly Strix Ubuntu lane onto the pool, or adding a small AIM-only nightly job there, would close it.Not verified
inputsexpression in a job-leveltimeout-minutes: GitHub's context-availability table listsinputsforjobs.<job_id>.timeout-minutes, and onpull_request,merge_groupandpushthere are no dispatch inputs, soinputs.include_nightlyis empty and falsy and the job resolves to 45 (the job already usesinputs.include_nightly && '1' || ''at env level on those events). The first run of this PR on the workflow is the final check.cargo nextest run -p xtask(358 passed),cargo nextest run -p e2e-cucumber --test feature_naming, the new test, clippy forxtaskande2e-cucumber(including--test e2e) andcargo fmt --checkpass. In thee2e-cucumberlibrary tests, twoshared_runtimesymlink tests fail locally with os error 1314 (Windows symlink privilege), unrelated to this change.