Skip to content

fix(bench): stream reasoning_content and accept clean zero-content finishes#18

Merged
dutifulbob merged 3 commits into
mainfrom
fix/stream-reasoning-content
Jul 6, 2026
Merged

fix(bench): stream reasoning_content and accept clean zero-content finishes#18
dutifulbob merged 3 commits into
mainfrom
fix/stream-reasoning-content

Conversation

@dutifulbob

Copy link
Copy Markdown
Member

Opened on behalf of Onur Solmaz (osolmaz).

Summary

The streaming TTFT path from PR #14 failed valid responses on reasoning and diffusion models.
When a re-run of the GB10 sweep hit the two Gemma models, their prefill points all failed with response_shape: "stream produced no completion content" (diffusiongemma dropped from 31 completed to 12).
Root cause: the parser only counted delta.content as a token, so reasoning models — which stream their first tokens as delta.reasoning_content — and 1-token prefill points that finish with no streamed content were both rejected. It would also have nil-derefed firstTokenAt had that path been reachable.

What Changed

The stream parser now understands reasoning output and clean empty completions.

  • openAIMessage gains reasoning_content; streamChoiceText returns it when content is empty, so a reasoning model's first token seeds TTFT and ITL timing.
  • A stream that ends with a finish_reason but zero streamed tokens (a 1-token prefill, or a reasoning-only turn) now counts as completed rather than failing. It carries no TTFT — honest, not a fabricated zero.
  • Only a stream that ends with neither any token nor a finish reason is still response_shape-malformed.
  • applyToSample no longer dereferences firstTokenAt when nil (TTFT/TPOT/ITL are set only when a token was actually observed).
  • Terminal validation extracted into finalize() (keeps CRAP under budget).

Testing

  • New: TestStreamingReasoningContentCountsForTTFT (reasoning_content seeds TTFT+ITL), TestStreamingFinishWithNoContentCompletes (clean zero-content finish completes with no TTFT). Retargeted the old no-content test to the genuinely-malformed case (TestStreamWithNeitherTokenNorFinishFailsShape).
  • go test ./..., go vet, gofmt, simpledoc, slophammer check/dry, check-crap.sh, golangci-lint, dry-run + artifact check — all clean.
  • Live: re-running the two Gemma models with this build (verification in progress); qwen models were unaffected (they stream normal content).

Risks

Low. Non-streaming path untouched; qwen (normal content) behavior unchanged. The only behavior change is that clean zero-content streamed finishes now complete instead of failing, which matches the existing non-streaming zero-token handling.

…nishes

Reasoning/diffusion models stream first tokens as delta.reasoning_content
and 1-token prefill points can finish with no streamed content. The
strict content-only parser failed those requests (response_shape) and
would have nil-derefed firstTokenAt. Now reasoning_content seeds TTFT/ITL,
a clean finish_reason with zero tokens counts as completed (no fabricated
TTFT), and only a stream with neither token nor finish reason is malformed.
…mples

Codex P1: the reorder put the TPOT calc before sample.CompletionTokens was
set, dropping TPOT for all multi-token streamed completions. Assign tokens
first; test now asserts positive TPOT.
…for streamed completion tokens

Codex P2: a zero-content stream that finishes without a usage chunk would
borrow request.OutputTokensExpected and fabricate throughput. Fall back to
the observed streamed-chunk count instead (0 for empty streams); usage
still wins when present. Test covers no-usage empty and content cases.
@dutifulbob

Copy link
Copy Markdown
Member Author

Review complete: two consecutive P2-only rounds (no P0/P1), stopping per the review rule.

Addressed: reasoning_content TTFT, clean zero-content finish acceptance, nil-deref guard, and the TPOT-ordering P1.

Known limitation (documented, not blocking): when usage is absent from a streamed response, completion tokens fall back to the observed SSE chunk count. This can undercount for a non-conforming backend that both omits usage AND batches multiple tokens per chunk. In practice localperf always sends stream_options.include_usage=true and vLLM honors it, so the fallback is never exercised; it is strictly more honest than the prior behavior (which credited the requested output length). A tokenizer-based count would be the follow-up if a non-usage backend is ever targeted.

@dutifulbob dutifulbob merged commit 60ade80 into main Jul 6, 2026
2 checks passed
@dutifulbob dutifulbob deleted the fix/stream-reasoning-content branch July 6, 2026 12:12
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