test(e2e): run the responses suite on vllm and tokenspeed - #2044
Conversation
The responses (OpenAI Responses API) lane only launched sglang, so the
cross-engine responses tests already marked
`engine("sglang", "vllm", "trtllm", "tokenspeed")` never actually ran on
vllm or tokenspeed, and the `engine("vllm")` image-generation test never ran
at all.
Turn the lane into an engine matrix (sglang, vllm, tokenspeed) over the same
`e2e_test/responses` suite. The engine marker filter selects only the tests
each engine supports: vllm and tokenspeed pick up the shared gpt-oss-20b
tests (tools, streaming, structured output, state, sampling), vllm
additionally runs the Llama-3.1-8B image-generation test, and the sglang-only
Qwen2.5-14B cases deselect automatically. tokenspeed keeps a generous job
timeout for its from-source build.
Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe one-GPU responses E2E workflow now runs as a fail-fast-disabled matrix for SGLang, vLLM, and TokenSpeed. Each engine supplies its own job and test timeout. ChangesResponses E2E matrix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| include: | ||
| - engine: sglang | ||
| timeout: 28 | ||
| test_timeout: 20 | ||
| - engine: vllm | ||
| timeout: 28 | ||
| test_timeout: 20 | ||
| # tokenspeed builds from source (~30m cold), so keep the job | ||
| # timeout generous even though the test step is short. Only the | ||
| # cross-engine gpt-oss-20b responses tests select onto this lane; | ||
| # the engine marker filter deselects the sglang-only cases. | ||
| - engine: tokenspeed | ||
| timeout: 50 |
There was a problem hiding this comment.
🟡 Nit: Five response tests are marked @pytest.mark.engine("sglang", "vllm", "trtllm", "tokenspeed") but the matrix only adds vllm and tokenspeed — trtllm is still missing. The chat lane already runs trtllm on the same 1-gpu-h100 runner, so adding it here would close the remaining coverage gap. Intentional omission or follow-up?
There was a problem hiding this comment.
Clean change that follows the established matrix pattern from the chat lane. YAML structure is correct, aggregate gate covers all legs, and timeouts are reasonable (tokenspeed's 50m matches the chat lane's from-source build budget).
One 🟡 nit posted: trtllm is marked in 5 response tests but wasn't added to the matrix — worth a follow-up if not intentionally deferred.
Summary: 0 🔴 Important · 1 🟡 Nit · 0 🟣 Pre-existing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/pr-test-rust.yml (1)
685-691: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove unused secret inheritance.
.github/workflows/e2e-gpu-job.ymldoes not declare or reference secrets. Removesecrets: inheritfrom this call. If cloud response tests requireOPENAI_API_KEY, map that secret explicitly to the test environment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-test-rust.yml around lines 685 - 691, Remove the unused secrets inheritance from the workflow call to e2e-gpu-job.yml. If the cloud response tests need OPENAI_API_KEY, pass that secret explicitly through the test environment instead of inheriting all secrets.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/pr-test-rust.yml:
- Around line 685-691: Remove the unused secrets inheritance from the workflow
call to e2e-gpu-job.yml. If the cloud response tests need OPENAI_API_KEY, pass
that secret explicitly through the test environment instead of inheriting all
secrets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c91fa73-0390-45bf-8c6e-02a551e0d401
📒 Files selected for processing (1)
.github/workflows/pr-test-rust.yml
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
1 participant
Description
Problem
The
e2e-1gpu-responseslane launched sglang only. But many of theresponses (OpenAI Responses API) tests are already marked
@pytest.mark.engine("sglang", "vllm", "trtllm", "tokenspeed")— they werewritten to run cross-engine, yet they never executed on vllm or tokenspeed
because no such lane existed. One test (
image_generation) is marked@pytest.mark.engine("vllm")and never ran anywhere.Solution
Turn the responses lane into an engine matrix —
sglang,vllm,tokenspeed— over the samee2e_test/responsessuite. No test files change;the existing engine marker filter selects the right subset per lane:
gpt-oss-20btests (tools call, streaming events,structured output, state management, sampling params) plus the
vllm-onlyLlama-3.1-8Bimage-generation test (previously dead coverage).gpt-oss-20btests.Qwen2.5-14Bcases still run hereand deselect on the other lanes automatically.
Changes
.github/workflows/pr-test-rust.yml—e2e-1gpu-responsesbecomes astrategy.matrixover{sglang, vllm, tokenspeed}. tokenspeed keeps agenerous job timeout (~50m) for its from-source build, matching the chat
lane. Both new engines run
gpt-oss-20b(tp=1), already proven on1-gpu-h100 by the existing sglang responses and chat lanes.
Test Plan
Workflow-only change; validated by the responses lanes running green in this
PR's own CI (sglang / vllm / tokenspeed).
python3 -c "import yaml; yaml.safe_load(open('.github/workflows/pr-test-rust.yml'))"passese2e-1gpu-responsesis unchanged, so the aggregate summary gate stillcovers all three matrix legs.
Checklist