fix(endpoints): count OpenAI Responses streaming output tokens once, not twice#1134
fix(endpoints): count OpenAI Responses streaming output tokens once, not twice#1134ajcasagrande wants to merge 3 commits into
Conversation
…not twice A streaming Responses turn carries the assistant text twice: as the chain of response.output_text.delta events and again, in full, as the terminal response.output_text.done event. The base extract_response_data parses every event, so both were tokenised and client-side output tokens (OSL) / output-token-throughput were inflated ~2x on every compliant streaming server. Fix: override extract_response_data to treat the terminal done as a structural envelope once any delta has carried text, while keeping done as the sole text carrier when NO delta did (a server that emits only the terminal event, or the non-streaming convenience field). This restores the semantics the original reviewer asked for. Provenance: the done handler was added in ai-dynamo#695 in response to review feedback (@debermudez) to parse response.output_text.done 'as a fallback when it includes text' for providers that emit final text only in done events. The implementation made it an unconditional second carrier rather than a fallback, which introduced the double-count; no follow-up caught it. This change makes done a true fallback. Validated against 2,399 real streamed messages captured from live OpenAI Responses traffic: pre-fix inflates output tokens 2.0000x, this fix yields exactly the ground-truth count (1.0000x), with concat(deltas) == done.text holding on 2,399/2,399 messages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anthony Casagrande <acasagrande@nvidia.com>
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@e774a7f3d688334ca0f3bd719a860a4bb86806e6Recommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@e774a7f3d688334ca0f3bd719a860a4bb86806e6Last updated for commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughResponses parsing now shares a JSON routing path and suppresses duplicate streamed output text when ChangesStreaming Output Text De-duplication
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/aiperf/endpoints/openai_responses.py`:
- Around line 306-322: The dedup logic in the response parsing loop is too broad
because a single saw_output_text_delta flag in the parsing method suppresses all
later response.output_text.done events, even for different (output_index,
content_index) pairs. Update the logic in the response handling path around
_route_parsed_json so deltas and terminal done events are tracked per
output/content pair instead of globally, and only skip a done event when the
matching pair has already seen a delta. Add a regression test covering multiple
output/content parts to verify one stream item does not suppress another.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 698ab980-0b04-4e01-ac1b-13a45aee5f8a
📒 Files selected for processing (2)
src/aiperf/endpoints/openai_responses.pytests/unit/endpoints/test_openai_responses_streaming_dedup.py
…nt_index) Addresses CodeRabbit review on ai-dynamo#1134: the dedup used a single global saw_output_text_delta flag, so once any part streamed a delta, EVERY later response.output_text.done was skipped - including a done-only part (a server that streamed only one item, or deltas dropped under load), silently dropping its text (an undercount). Track streamed parts per (output_index, content_index) and skip a done only when its own part already carried a delta. Adds a regression test for a multi-part response where part 0 streams and part 1 is done-only. Verified: single-item dedup and the 2,399-message real-trace validation are unchanged (1.0000x); the multi-part done-only case now keeps both parts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anthony Casagrande <acasagrande@nvidia.com>
| elif ( | ||
| event_type == "response.output_text.done" and part in streamed_parts | ||
| ): | ||
| continue |
There was a problem hiding this comment.
Skipping the duplicate response.output_text.done event removes its perf_ns from ParsedResponseRecord.content_responses, so RequestLatencyMetric and derived streaming metrics use the last delta timestamp while the worker still uses the done timestamp. Fix: preserve a non-tokenizing parsed response at the done event timestamp while excluding its text from client-side tokenization.
🤖 AI Fix
In src/aiperf/endpoints/openai_responses.py, update ResponsesEndpoint.extract_response_data so the duplicate response.output_text.done branch appends ParsedResponse(perf_ns=response.perf_ns, data=TextResponseData(text="")) and then continues instead of only continue, and add a regression in tests/unit/endpoints/test_openai_responses_streaming_dedup.py asserting token text is not duplicated while content timing still includes the done event timestamp.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…s text Addresses dynamo-review-agent on ai-dynamo#1134: a bare 'continue' past the deduped response.output_text.done dropped its perf_ns from content_responses, which RequestLatencyMetric (content_responses[-1].perf_ns) and InterChunkLatency (gaps between content_responses) consume — so the request-latency endpoint silently shifted from the done timestamp to the last-delta timestamp. Keep a zero-text ParsedResponse at the done timestamp instead: empty text contributes no output tokens (dedup intact), while the timestamp stays as the last content response so all content-timing metrics are byte-for-byte unchanged from pre-dedup behavior. Test now asserts the placeholder text is empty and its timestamp equals the done event's. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anthony Casagrande <acasagrande@nvidia.com>
Summary
The OpenAI Responses endpoint double-counts client-side output tokens on every streaming turn. Output sequence length (OSL) and output-token-throughput are reported at roughly 2× their true value against any compliant streaming server.
The bug
A streaming Responses turn delivers the assistant's text twice on the wire:
response.output_text.deltaevents (delta= the next text chunk), andresponse.output_text.doneevent (text= the complete finalized text).ResponsesEndpoint._streaming_event_datamaps both to aTextResponseData:The base
extract_response_datacallsparse_responsefor every event in the record, soInferenceResultParserconcatenates and tokenizes the delta chain and the done text. The streamed output is counted once from the deltas and again from the done.Concrete example — a turn that streams
"The quick brown fox":output_text.delta"The quick "output_text.delta"brown fox"output_text.done"The quick brown fox"← full text againTokenized output =
"The quick brown fox"+"The quick brown fox"= 2× OSL.The fix
Override
extract_response_dataso the terminaldoneis treated as a structural envelope once any delta has carried text, while remaining the sole carrier when no delta did (a server that emits only the terminal event, or the non-streaming convenience field):doneis now a fallback: counted only when the deltas didn't already carry the text. The single forward pass is correct becausedonealways trails its deltas in SSE arrival order, whichrecord.responsespreserves.parse_responseitself is unchanged — the per-event callers (first-token detection, request latency) treatdoneas a plain data event and never sum tokens, so they see no behavioral change.Validation
concat(deltas) == done.textheld on 2,399/2,399; pre-fix output tokens = 2.0000× ground truth, post-fix = 1.0000×.tests/unit/endpoints/test_openai_responses_streaming_dedup.pypins: deltas + a terminaldonetokenize to the delta-only count; adone-only stream (no deltas) still counts once; reasoning and tool-call streams are unaffected.Test plan
Summary by CodeRabbit
Bug Fixes
Tests