Skip to content

[Fix] Reject Chat Completions streaming errors in bench_serving - #40881

Closed
huhumeng2 wants to merge 4 commits into
sgl-project:mainfrom
huhumeng2:fix/bench-chat-stream-errors
Closed

huhumeng2 wants to merge 4 commits into
sgl-project:mainfrom
huhumeng2:fix/bench-chat-stream-errors

Conversation

@huhumeng2

@huhumeng2 huhumeng2 commented Sep 23, 2026 •

Copy link
Copy Markdown

Motivation

An OpenAI-compatible Chat Completions endpoint can send an error event after starting an HTTP 200 SSE response. SGLang emits this format for streaming generation failures.

The benchmark client currently treats that event as an empty-choices chunk, skips it, and marks the request successful at EOF. An error-only stream followed by [DONE] is reported as success=True, output_len=64 for a 64-token request, despite producing no text. This inflates completed-request and output-token totals and can hide failed warmup requests.

Modifications

Reject a non-null SSE error before processing usage or choices, using the existing exception-to-failed-request path. Add loopback HTTP regressions for errors before content, errors after partial content (including exclusion from aggregate metrics), and a valid chunk with error: null.

Only the benchmark client and its existing registered CPU test file change.

Accuracy Tests

Based on d4dcce12d4fcdb3eb694d54d6afd31628c2eb7ce.

  • Before the fix: the two error regressions fail because the client reports success; the null-error control passes.
  • After the fix: 13 tests pass, including existing reasoning-content, usage-only chunk and non-streaming tests.
  • Actual-model smoke check on one RTX 5090: 16 streaming and 16 non-streaming requests complete successfully using Qwen3-0.6B BF16, FlashInfer, and decode CUDA graphs. This validates the normal client path; generation failures are reproduced by the CPU loopback HTTP fixtures.
PYTHONPATH=python CUDA_VISIBLE_DEVICES=99 python -m pytest -q \
  test/registered/bench_fn/test_bench_serving_reasoning_stream.py

The invalid GPU ordinal hides the accelerator on the test machine. The current base's test-utils import fails with an empty CUDA_VISIBLE_DEVICES value, which is outside this change.

Environment: Ubuntu 22.04, Python 3.12.14, PyTorch 2.13.0+cu130. No model-forward or sampling changes.

Speed Tests and Profiling

No performance claim. This fixes benchmark success/token accounting; inference kernels are unchanged.

Checklist

  • Applicable pre-commit hooks pass on both modified files.
  • Add concrete regression tests using the existing CustomTestCase and CPU CI registration.
  • Follow repository code style.
  • Documentation assessed: no new option or supported behavior to document.
  • Full-repository pre-commit passed in the upstream lint workflow, including all four Rust hooks.
  • Hardware CI complete (requires an authorized maintainer to enable run-ci).

pre-commit run --all-files was executed in an isolated validation worktree. All checks other than four Rust hooks passed. Those four could not run because this environment has no cargo/rustup: rustfmt-sgl-model-gateway, rustfmt-sgl-router, clippy-rust-workspace, and rustfmt-rust-workspace. No Rust files change. The upstream lint workflow has since passed for commit 9a2827bb08c9e1122332d82f1fdbe66ea43abf58: https://github.com/sgl-project/sglang/actions/runs/35837114793. Hardware CI still requires the repository's authorized run-ci process.


CI States

Latest PR Test (Base): ❌ Run #36746890992
Latest PR Test (Extra): ❌ Run #36746890533
Latest PR Test (AMD ROCm 10): ❌ Run #36746891116

@huhumeng2
huhumeng2 marked this pull request as ready for review September 23, 2026 08:31
@huhumeng2

Copy link
Copy Markdown
Author

I have updated this branch with main; the current head is 0281067. The new PR Test run stops at the authorization gate with “Missing required label run-ci”, before test execution. Could an authorized maintainer enable run-ci and review this benchmark streaming-error fix? Lint passed on the previous revision; the new lint run is still in progress. The focused regression and smoke-test results in the PR description are from the earlier revision. Thank you!

@huhumeng2

Copy link
Copy Markdown
Author

@nvcnvn Could you help review this serving-benchmark streaming-error fix or route it to the appropriate maintainer? The updated head is 36264a75b1, which contains the required MIN_BASE_SHA (8854857a99), and the PR still changes only the original two files.

When a Chat Completions SSE response carries an error (including after partial content), the benchmark currently can count the request as successful. The fix rejects these error responses rather than allowing them to inflate success metrics. The two error-handling regressions fail against the pre-fix implementation; all 13 focused streaming tests pass on the updated branch in a fresh environment on my local 5090 machine. The repository-version isort, selected Ruff rules, formatting, codespell, CI test-registration validation, and git diff --check also pass.

Could an authorized maintainer enable run-ci and review this change? The previously reported green lint applies to the earlier head; the workflows for this push are new. Thank you.

bowenwan6 added a commit to bowenwan6/sglang-vllm-profiler that referenced this pull request Oct 5, 2026
A1.7 is met in T4 once aborted requests are counted as failed; the
throughput ranking there is inside the noise. sgl-project/sglang#40881
already fixes the chat path of the in-stream-error accounting, so the fix
is not opened as a competing PR and --goodput stands on its own. Adds the
limits found on a second reading: 50 s runs, joint against per-SLO
attainment, post hoc SLO sets for T2.
@nvcnvn

nvcnvn commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@nvcnvn Could you help review this serving-benchmark streaming-error fix or route it to the appropriate maintainer? The updated head is 36264a75b1, which contains the required MIN_BASE_SHA (8854857a99), and the PR still changes only the original two files.

When a Chat Completions SSE response carries an error (including after partial content), the benchmark currently can count the request as successful. The fix rejects these error responses rather than allowing them to inflate success metrics. The two error-handling regressions fail against the pre-fix implementation; all 13 focused streaming tests pass on the updated branch in a fresh environment on my local 5090 machine. The repository-version isort, selected Ruff rules, formatting, codespell, CI test-registration validation, and git diff --check also pass.

Could an authorized maintainer enable run-ci and review this change? The previously reported green lint applies to the earlier head; the workflows for this push are new. Thank you.

Hi, I'm just a random guy with a single contrib, last time I ask @BBuf on slack about this and he help me to review the PR.

Copy link
Copy Markdown
Author

Thanks @nvcnvn for clarifying and pointing me toward the right reviewer.

@BBuf, could you review this serving-benchmark fix or route it to its owner, and help authorize run-ci for head 36264a7? It rejects Chat Completions SSE error events so failed requests are not counted as successful. The current-head lint check is green, but PR Test stops at the missing run-ci label before CPU/GPU tests execute. The focused local regression results are recorded in the existing PR description and September 30 update; I am not claiming an upstream test pass.

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