Skip to content

fix(openai): align first-chunk fields and cover response combinations - #529

Merged
arekay-nv merged 2 commits into
mainfrom
arekay/content_type_coverage
Oct 8, 2026
Merged

arekay-nv merged 2 commits into
mainfrom
arekay/content_type_coverage

Conversation

@arekay-nv

@arekay-nv arekay-nv commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Mixed OpenAI chat deltas can contain content, reasoning, and tool calls in the same first response chunk. TPOT used to infer which field held the first token from the final fields, which is only right when a response starts with pure reasoning. #526 kept reasoning from mixed deltas (fixing OSL) but kept that inference, so TPOT counted content already delivered with the first token, or dropped tokens that arrived later:

Stream (delta order) Correct TPOT text Before (#526) This PR
reasoning → content R1 C0 C1 ✅ ✅
reasoning + content in first delta C1 ❌ C0 C1 ✅
content → reasoning R0 C1 ❌ C0 C1 ✅
content + tool start → tool tail tool tail fragment ❌ whole tool call ✅
tool start → content C0 C1 ❌ C1 + first tool fragment ✅

Align entry zero of each streamed TextModelOutput field with the first meaningful delta. Fields that start later get an empty first entry; text tails are aggregated and tool-call batches retain their indices for merging. The accumulator keeps a reference to the first delta alongside its existing contributions. Full-output consumers combine all entries, while TPOT skips entry zero independently in every field. No new output class or wire field is added.

Existing event logs keep their original TPOT on replay. Earlier revisions stored reasoning responses as a joined output string beside streamed reasoning. The current accumulator never writes that shape, so TextModelOutput and the steady-state diagnostics count all of that string as tail, matching the TPOT those runs reported. This covers scripts/early_stopping_estimate_from_events.py and the diagnostics CLI. Neither replays tool-call samples, so the older tool-call layout needs no separate rule.

Adds 292 response-contract test cases, including:

  • All eight content/reasoning/tool combinations across two deltas, both reasoning aliases, and both streaming modes (256 cases).
  • Single-delta output, empty/role-only/null/control messages, interleaved tool arguments, Unicode, and JSON/MessagePack round trips.
  • Hypothesis-generated sequences and chunk splitting after a fixed first delta.
  • Real-tokenizer OSL/TPOT checks through the production event codec for mixed-first, tools-before-text, content-before-reasoning, and single-mixed-delta responses.

Also covers replay of the joined-output layout through TextModelOutput and the offline diagnostics, and updates existing fixtures and offline diagnostics to the aligned-field contract. Follow-up to merged PR #526.

Validation:

  • 852 tests passed across OpenAI adapters, core serialization, metrics aggregation, metrics and steady-state diagnostics, and the early-stopping replay script. One existing Linux-only CPU-affinity test was deselected on macOS; a short temporary directory avoids the macOS IPC path limit.
  • Replaying a pre-change reasoning event log through scripts/early_stopping_estimate_from_events.py gives the same TPOT as the revision that produced it (1111 ns; 2500 ns without the replay rule).
  • All pre-commit hooks passed. mypy targeting Linux reports no issues (335 files); the default macOS target reports the three existing errors for Linux CPU-affinity APIs.
  • Synthetic response inputs and the checked-in hermetic chat tokenizer; no live GPU endpoint runs.

🤖 Generated with Claude Code

@arekay-nv
arekay-nv requested a review from a team October 5, 2026 17:23
@github-actions
github-actions Bot requested a review from nvzhihanj October 5, 2026 17:23
@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@github-actions github-actions Bot added the size/normal PR Review Policy: <=500 non-test lines & <=20 files label Oct 5, 2026
@arekay-nv
arekay-nv requested review from hvagadia and leopck October 5, 2026 17:24
@arekay-nv
arekay-nv marked this pull request as draft October 5, 2026 17:24
@arekay-nv
arekay-nv force-pushed the arekay/content_type_coverage branch from 6967d7c to b71b593 Compare October 5, 2026 17:53
@arekay-nv arekay-nv changed the title fix(openai): cover content types and preserve first-chunk boundaries fix(openai): align first-chunk fields and cover response combinations Oct 5, 2026
@arekay-nv
arekay-nv requested a review from nv-alicheng October 5, 2026 21:58
@arekay-nv
arekay-nv marked this pull request as ready for review October 5, 2026 21:58
@arekay-nv arekay-nv added the priority: P0 Critical — blocks release or users label Oct 6, 2026
arekay-nv and others added 2 commits October 7, 2026 21:14
Event logs written before the aligned first-chunk layout store reasoning
responses as a joined str output beside streamed reasoning. The aligned
rule treated that str as non-streaming and dropped all content from the
TPOT denominator on replay. The current accumulator never produces that
shape, so TextModelOutput and the steady-state diagnostics count all of
it as tail, restoring each archived run's original TPOT.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@arekay-nv
arekay-nv force-pushed the arekay/content_type_coverage branch from eb375b8 to eefe1b9 Compare October 8, 2026 02:19

@leopck leopck left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@arekay-nv
arekay-nv merged commit d3c57f2 into main Oct 8, 2026
18 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: P0 Critical — blocks release or users size/normal PR Review Policy: <=500 non-test lines & <=20 files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants