Skip to content

ci(e2e): move the AIM serve scenarios off the per-PR Strix Ubuntu lane - #852

Merged
volen-silo merged 1 commit into
mainfrom
ci/strix-ubuntu-aim-scenarios
Nov 4, 2026
Merged

volen-silo merged 1 commit into
mainfrom
ci/strix-ubuntu-aim-scenarios

Conversation

@volen-silo

@volen-silo volen-silo commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The self-hosted E2E tests (Strix Halo, Ubuntu, release, current) and ... nightly, current jobs 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-26 and serve-27 are 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 plus serve-26's readiness wait reaches the cap with serve-27 just starting: in the artifact of a cancelled run (started 07:46, cancelled 08:32), the command log holds 183 scenarios and ends inside serve-27 after both launched rocm serve ... --engine aim with 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-26 itself 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

  • Tag serve-26 and serve-27 @nightly, so they run only when E2E_INCLUDE_NIGHTLY is set, on a host with Docker and a supported GPU. Ids and numbering are unchanged (feature_naming still passes), and the feature comment says why.
  • A unit test in 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 own resolve, with an injected host and opt-in: without E2E_INCLUDE_NIGHTLY both 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 @nightly tags 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 real docker info, so the run-path branch executes only on a host with Docker; I ran the skip branch locally.
  • Lanes that do opt in get a cap that fits: the nightly Strix Halo job goes from 90 to 120 minutes, and the per-PR Strix Ubuntu job gets ${{ inputs.include_nightly && 105 || 45 }}, so per-PR runs keep 45 and only an include_nightly dispatch 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.md says where the AIM scenarios now run, with the caveat below.
  • The nightly Strix Halo job gets one step before the E2E run that runs docker info, the same probe the harness uses, and prints a ::warning:: if Docker is unreachable. The job is continue-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 @nightly mechanism avoids.

Scenario coverage

serve-26 and serve-27 already 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 an include_nightly dispatch of e2e-selfhosted.yml with platform strix-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_nightly dispatch 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

  • The self-hosted lanes and a real AIM run cannot be exercised locally.
  • The inputs expression in a job-level timeout-minutes: 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 falsy and the job resolves to 45 (the 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.
  • cargo nextest run -p xtask (358 passed), cargo nextest run -p e2e-cucumber --test feature_naming, the new test, clippy for xtask and e2e-cucumber (including --test e2e) and cargo fmt --check pass. In the e2e-cucumber library tests, two shared_runtime symlink tests fail locally with os error 1314 (Windows symlink privilege), unrelated to this change.

@juhovainio juhovainio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The opt-in half of the new unit test only asserts when resolve returns a Skip, so it doesn't actually pin that the scenarios are still reachable when nightly is opted in.
  2. 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.
  3. The inputs expression in job-level timeout-minutes should 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.

Comment thread tests/e2e-cucumber/src/expectation.rs Outdated
nightly: true,
..Included::default()
};
if let Expectation::Skip { reason } = resolve(&d, &cap("strix-ubuntu"), &m, opted_in) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please confirm the first run on this PR accepts the inputs expression in job-level timeout-minutes (per-PR should resolve to 45).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 nowycondro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 at tests/e2e-cucumber/src/expectation.rs:711-717, tests/e2e-cucumber/src/capability.rs:171-179
    cap("strix-ubuntu")'s available_engines is ["lemonade", "vllm"] — it never contains "aim". HostCapability::engine_available("aim") requires self.available_engines.iter().any(|e| e == "aim") as its first, short-circuiting condition, so it is unconditionally false for this fixture regardless of docker_available() or GPU family. Tracing resolve(): the nightly check (decl.nightly && !include_nightly) passes once opted_in.nightly = true, but execution then reaches the requires_gpu && !cap.engine_available(engine) check lower in resolve (expectation.rs:604-611), which returns Skip{reason: "engine 'aim' cannot start on this host (strix-halo)"} — every time, opted in or not. The test's if let Expectation::Skip { reason } = resolve(...) { assert!(!reason.contains("E2E_INCLUDE_NIGHTLY"), ...) } is trivially satisfied by this unrelated reason string, so it never actually confirms serve-26/serve-27 resolve to ExpectPass (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. if resolve'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" to cap("strix-ubuntu")'s available_engines and replace the if let Skip with a direct assert_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, unmockable docker info subprocess 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 during actions/checkout with ##[error]No space left on device on 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 to model_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 on main before 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 an include_nightly dispatch. 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 real model_serving.feature via include_str! rather than hardcoding a tag list, so the test and the feature file's actual tag set structurally cannot drift on which tags serve-26/serve-27 carry.
  • The new 105/120-minute budgets are built from specific, cross-checkable figures (the ~25-minute measured base, the existing @serve-timeout:1200 tag, and the large-model-serve time independently stated elsewhere in the same workflow file) rather than round-number guesses.
  • The inputs.include_nightly reference the new timeout-minutes expression relies on is not a new pattern: the same input already drives env: values and if: 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-27 will silently skip every night, and the only path that reliably exercises AIM serving becomes a manual include_nightly dispatch. 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>
@volen-silo
volen-silo force-pushed the ci/strix-ubuntu-aim-scenarios branch from ef9d8b7 to a79fbff Compare November 3, 2026 13:20
@volen-silo

Copy link
Copy Markdown
Collaborator Author

Pushed a79fbff (rebased onto main). All three points are addressed (threads), including the mutation check you asked for: the test fails with the @nightly tags removed and with the opt-in forced off. The Docker question on the nightly Strix host stays open by nature; the nightly job now logs it so the first run answers it.

@volen-silo
volen-silo dismissed juhovainio’s stale review November 3, 2026 13:21

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 nowycondro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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-windows was 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_NIGHTLY wired identically to the Ubuntu job)
    This job runs on the same strix-halo (gfx1151) hardware and wires include_nightly the same way as e2e-gpu-strix-ubuntu. Traced HostCapability::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 an include_nightly dispatch with platform: strix-windows (or the default platform: 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 at tests/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 @nightly tag in the suite and checking each against engine_available/requires_os/requires_bare_metal for the strix-ubuntu fixture (available_engines:["lemonade","vllm"], linux, bare-metal, gfx1151 → lemonade always available, vllm unavailable on this family): at least runtime-01 (SDK install), runtime-12 (SDK reinstall), runtime-lemonade-backend-alignment-opt-out, therock-next-07 (live install), therock-next-06, and comfyui-install-preserves-the-rocm-runtime (bare-metal Linux install) also resolve to ExpectPass on this exact host under include_nightly — none is @requires-engine:vllm or @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 matching docs/testing.md:742-751 paragraph, name only serve-26/27 and "the large-model serve" — the manual include_nightly dispatch 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_tags duplicates the file's existing, more robust feature_tags helper — tests/e2e-cucumber/src/expectation.rs:1046-1057 vs. feature_tags at :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 takes lines[at - 1] relative to the Scenario: line, assuming the tag line is always the single line directly above with no separator — true today (verified against model_serving.feature) but more fragile and a needless duplication of a pattern the file already has.
    Confidence 85 · architectural · Fix: call feature_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 info subprocess twice per iteration (once inside resolve()→engine_available, once directly via docker_available() at expectation.rs:1090-1094) to decide which branch to assert
    This is the first test in the file to reach a real docker_available() call (every other docker-gated test passes Included{docker:false,..} and short-circuits first). Matching on the live result rather than a fixed expected value means the ExpectPass branch is only ever positively exercised on a host with a reachable Docker daemon — in a plain cargo test run without one, only the Skip branch 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: call docker_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_nightly dispatch — 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 with continue-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 shared cap() 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_lanes derives its tag check from the real model_serving.feature via include_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_nightly in the new job-level timeout-minutes: expression is declared type: boolean and the same inputs.include_nightly && X || Y pattern already runs today in this file's env: 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_nightly workflow_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 juhovainio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.rs has a long run of literal spaces (see inline comment).
  • r9700 / w7900 nightly jobs: they set E2E_INCLUDE_NIGHTLY but 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_nightly input description in e2e-selfhosted.yml doesn'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 a continue-on-error job. 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}"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@volen-silo
volen-silo added this pull request to the merge queue Nov 4, 2026
Merged via the queue into main with commit 02421f1 Nov 4, 2026
26 checks passed
@volen-silo
volen-silo deleted the ci/strix-ubuntu-aim-scenarios branch November 4, 2026 08:22
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.

3 participants