Skip to content

ci(e2e): stop pinning TokenSpeed's sequence window in the Qwen3.5-9B spec - #2467

Merged
slin1237 merged 1 commit into
mainfrom
ci/tokenspeed-spec-no-seq-window
Sep 8, 2026
Merged

slin1237 merged 1 commit into
mainfrom
ci/tokenspeed-spec-no-seq-window

Conversation

@hello-alexmcc

Copy link
Copy Markdown
Collaborator

Description

Problem

e2e_test/infra/model_specs.py pins --max-num-seqs for Qwen3.5-9B on TokenSpeed. The pin arrived with the EPD multimodal smoke lane (#1924) at 4, without a stated reason, and #2463 raised it to 32 after the first TokenSpeed PD run deadlocked: with a 4-wide decode window and 32 concurrent requests, the prefill's bootstrap deadline expired on requests that were merely queued behind the decode's admission. A small pinned window is not a property of the model, and it is exactly the configuration that turns a burst into bootstrap timeouts in PD. Nobody runs the engine that way outside this spec.

Solution

Remove the pin and let the engine's default window apply, as it does for every other model in the suite. The e2e-2gpu-pd (tokenspeed), e2e-4gpu-epd (tokenspeed) and, once #2464 lands, e2e-4gpu-pd (tokenspeed) lanes on this PR are the check that nothing relied on it. If the EPD lane turns out to need a bound for the encode worker, it should be scoped to that role in worker.py, not applied to every TokenSpeed worker of the model.

The burst-versus-window behaviour itself gets its own coverage: a topology-suite test that sets a deliberately small window and asserts the gateway's admission sheds instead of the engine timing out (follow-up on #2464), alongside the gateway admission fix in #2466 and the engine fix in TokenSpeed.

Changes

  • e2e_test/infra/model_specs.py: drop --max-num-seqs and its comment from the Qwen3.5-9B TokenSpeed args.

Test Plan

  • ruff check / ruff format --check clean.
  • Lanes exercising the change on this PR: e2e-2gpu-pd (tokenspeed) (11 cases, MMLU floor marked expected-fail), e2e-4gpu-epd (tokenspeed), e2e-1gpu-chat (tokenspeed).
Checklist
  • cargo +nightly fmt passes (no Rust changes)
  • cargo clippy --all-targets --all-features -- -D warnings passes (no Rust changes)
  • (Optional) Documentation updated
  • (Optional) Please join us on Slack #sig-smg to discuss, review, and merge PRs

…spec

The spec carried --max-num-seqs since the EPD smoke lane was added, first
at 4 and then at 32 after a PD burst deadlocked against the smaller
window. Neither value was a property of the model or of PD: a decode
worker's window bounds how many requests it can admit, and a pinned small
window is exactly what turns a burst into bootstrap timeouts. Let the
engine default apply; the PD lanes and the EPD lane on this change show
whether anything relied on the pin.

Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 9ab9b171-19d8-4934-ba00-7688ee622112

📥 Commits

Reviewing files that changed from the base of the PR and between 5630e82 and 61fdba2.

📒 Files selected for processing (1)
  • e2e_test/infra/model_specs.py
💤 Files with no reviewable changes (1)
  • e2e_test/infra/model_specs.py

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Configuration
    • Updated the Qwen3.5-9B model configuration to use the engine’s default maximum sequence count instead of an explicit limit of 32.

Walkthrough

The Qwen/Qwen3.5-9B TokenSpeed configuration removes the explicit --max-num-seqs 32 setting and its explanatory comment. The engine now uses its default maximum sequence count.

Changes

TokenSpeed configuration

Layer / File(s) Summary
Remove explicit sequence cap
e2e_test/infra/model_specs.py
The Qwen/Qwen3.5-9B tokenspeed_args no longer sets --max-num-seqs 32, allowing the engine to use its default.

Priority: ⬇️ Low — Impact reflects low issue severity.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 61fdb

The TokenSpeed specification now uses the engine default sequence window, with no concrete merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: removing the TokenSpeed sequence-window pin from the Qwen3.5-9B specification.
Description check ✅ Passed The description directly explains the problem, the solution, the affected file, and the planned validation. It is consistent with the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci/tokenspeed-spec-no-seq-window

Comment @coderabbitai help to get the list of available commands.

"--max-num-seqs",
"32",
"--gpu-memory-utilization",
"0.8",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Nit: Removing the pin leaves a stale cross-reference in the lane that motivated it. .github/workflows/pr-test-rust.yml:924-926 (the engine: tokenspeed entry in e2e-2gpu-pd) still reads:

# 4 (prefill waits for a decode that is never admitted). The window
# now covers the suite's bursts; the over-window deadlock itself is
# pinned by the topology suite.

After this change there is no window in the spec at all, so "the window now covers the suite's bursts" describes state that no longer exists — and it's exactly the comment the next person debugging a deadlock in this lane will read. Worth updating it in the same PR to say the spec now takes the engine default (and, per the description, that the burst/window case is covered elsewhere rather than by this pin).

@@ -187,13 +187,6 @@ def _resolve_model_path(hf_path: str) -> str:
"fa3",
"--max-model-len",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Nit: The replacement coverage this removal leans on isn't in the tree yet, so between this PR and the follow-up nothing guards the case the pin was covering. e2e_test/router/test_pd_mmlu.py:88-90 still drives num_threads=32 (and test_loads.py:120 16 concurrent long requests) against this model on the e2e-2gpu-pd (tokenspeed) lane, and there is no topology test asserting the small-window/burst behaviour today — grep -rn "topology" e2e_test/ only hits test_epd_multimodal.py, whose topology cases are sequential.

That's fine if TokenSpeed's default sequence window is comfortably above 32, which the lanes on this PR will show. The part that won't be caught later is a silent change in that default: if it ever computes below the suite's burst (this spec is a hybrid GDN/MoE pool at --gpu-memory-utilization 0.8, where the derived running-request cap is memory-dependent), the failure mode is the 75-minute lane timing out on a bootstrap deadlock rather than a fast, legible failure. Landing #2464's topology test before or with this removal — or noting the observed default in the spec comment — would keep that diagnosable.

@hello-alexmcc

Copy link
Copy Markdown
Collaborator Author

CI: the engine default holds — no scoped bound needed

Run 34195823976 is green end to end (finish included).

The concern that the EPD lane needed a small window just to get the engine up does not hold. Every TokenSpeed lane came up and passed with --max-num-seqs gone:

lane this PR main @ 5630e82c
e2e-4gpu-epd (tokenspeed) 37m57s 35m28s
e2e-2gpu-pd (tokenspeed) 32m34s 33m29s
e2e-1gpu-chat (tokenspeed) 25m05s 30m36s
e2e-1gpu-chat-zmq (tokenspeed) 45m14s 41m56s
e2e-2gpu-chat-zmq-dp (tokenspeed) 37m47s 37m39s

EPD selected 8 of 8 and reported 8 passed in 1539.38s (0:25:39), covering all four topologies (1e1p1d, 1e2p1d, 2e1p1d, 1e1p2d) — so encode, prefill and decode each booted under the engine default in both the single- and multi-worker layouts. PD reported 11 passed, 49 deselected, 1 xfailed in 1255.83s (0:20:55), the same shape as main.

Job totals move with image-pull time, so the test steps are the cleaner comparison: EPD's Run E2E tests was 25m50s here vs 24m13s on main (+1m37s), PD's 21m05s vs 22m37s (−1m32s). Opposite signs — runner noise, not a regression.

Why nothing needed a bound

At the pinned ref (.github/versions/tokenspeed.ref → 7cd7ca0b), ServerArgs.max_num_seqs resolves to 160 with no speculative algorithm. Walking the startup path per role:

  • encode — builds no ModelExecutor, holds no KV pool and captures no graphs; max_num_seqs only sizes its EncodeScheduler item batch. Nothing about it is a startup cost, which is why the pin at 4 in feat(e2e): EPD multimodal smoke CI on 4-gpu-h100 #1924 was never load-bearing for bring-up.
  • prefill — launched --enforce-eager by worker.py, so no capture either.
  • decode — the one place the window shows up at startup. get_batch_sizes_to_capture caps the ladder at min(max_cudagraph_capture_size, max_num_seqs // dp_size), so the pin was implicitly truncating capture to 8 sizes and the default admits the full 23-size ladder to 160. That is the plausible source of EPD's +1m37s if it is signal at all.

If decode-side capture ever does need bounding, --max-cudagraph-capture-size is the knob — it caps graph capture without touching admission, which is exactly the property the pin lacked. Not warranted on this evidence, so no code change: the PR stands as-is.

@slin1237
slin1237 merged commit c36298e into main Sep 8, 2026
54 checks passed
@slin1237
slin1237 deleted the ci/tokenspeed-spec-no-seq-window branch September 8, 2026 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants