Repository navigation
fix(inference): AUT-2539 hold back unstable reasoning in streamed chat deltas - #8254
svcnemo-autobot wants to merge 4 commits into
Conversation
StreamingChatParser committed reasoning deltas as soon as the reasoning parser returned them, so two cases streamed text that a later chunk moved: - A trailing partial end marker such as `</thi`, or a partial implicit end marker such as `<tool_call>`, was streamed as reasoning before the marker completed. - NemotronV3 with `enable_thinking=False` or `force_nonempty_content=True` returns the reasoning as content until a non-empty answer follows it, then moves that text to the reasoning channel. Content already streamed could not be retracted. While the reasoning is still open, hold back a trailing partial marker instead of committing it. For a parser that backfills content from reasoning (set with `force_content`, or detected by a one-time probe of `parse`), emit no deltas until the split is stable. The finished parse flushes everything. Add five unit tests to test_openai_streaming.py. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
This PR has been automatically converted to draft because all PRs must start as drafts. When you are ready for review, click Ready for Review to begin the review process. This will:
See the contribution guide for more details. |
|
/ok to test 72de487 |
Bring in main 3914483 so CI's main-based Testmon baseline matches the branch base. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
/ok to test b3e63ae |
Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
/ok to test 4364758 |
|
/review |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
[P2] Retain OpenAI logprobs while forced-content deltas are buffered
Failure: The new early return drops the standard OpenAI logprobs for buffered tokens. For example, use NemotronV3ReasoningParser with enable_thinking=False, stream two partial frames generating a and b with logprobs, then send a final frame containing the same two tokens. Both partial parses return []. At finalization, openai_stream finds no new tokens, sets logprobs=None, and emits the buffered {"content":"ab"} without its logprobs. The terminal generation_log_probs extension does not restore the missing OpenAI choices[].logprobs.content entries. This follows directly from the per-frame attachment logic.
Correction: Keep the reasoning/content holdback, but retain unattached logprob records in each choice's openai_stream state. Consume them once when the parser next emits a textual delta, including a final-only flush. The invariant is that delaying text must not discard its logprob records.
Suggested implementation in the state initialization and parser branch, preserving the existing policy of attaching records to the first textual delta:
# Add to each per-choice states entry:
pending_parser_logprobs=None,Replace the parser branch's logprob selection through the assignment to parsed_logprobs; retain its existing yield sse(...) below:
if parser is not None:
if logprobs is not None:
pending = state["pending_parser_logprobs"]
if pending is None:
state["pending_parser_logprobs"] = logprobs
else:
for key, entries in logprobs.items():
pending[key].extend(entries)
parsed_deltas = parser.parse(state["detokenizer"].text, finished=is_final)
for parsed_delta in parsed_deltas:
parsed_logprobs = None
if (
state["pending_parser_logprobs"] is not None
and (
parsed_delta.get("content") is not None
or parsed_delta.get("reasoning_content") is not None
)
):
parsed_logprobs = state["pending_parser_logprobs"]
state["pending_parser_logprobs"] = None
# Keep the existing yield sse(...) here.Preservation and acceptance — proposed checks, not executed:
- Add an async regression alongside the existing parser/logprob test: partial tokens
[1], then[2], followed by final tokens[1, 2], using the existing test tokenizer and forced Nemotron parser. Expect no early text, final contentab, and standard logprob entries for bothaandb, in order and exactly once. Include-infand top-logprobs to verify their existing finite-floor conversion survives buffering. Also test a final frame adding token3: expectabcwith all three entries exactly once. - Retain the existing immediate passthrough assertion: content
acarries its top-logprob entry withJSON_SAFE_LOGPROB_FLOOR. Unparsed streams must retain their existing chunking and logprobs. - Keep the new reasoning regressions unchanged: forced fallback remains withheld until stable/final, ordinary Nemotron reasoning still emits
helimmediately, and unfinished literal</thiflushes at finish. - Keep tool-only output free of invented text deltas; retain the existing single tool name/ID and complete JSON argument assertions. Pending records must remain choice-local when multiple streams are interleaved.
Static review only; tests were not executed. The suggested code requires author verification before adoption.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex
The streaming chat parser can hold text back for later frames: forced content waits for a stable reasoning split, and partial control markers wait for their next characters. openai_stream attached each frame's logprobs only to that frame's first textual delta, so tokens whose text was held back lost their choices[].logprobs entries. Keep unattached logprobs per choice and attach them, once and in order, to the next textual delta, including the final flush. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
/ok to test 9bd15b2 |
|
/review reset=true |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
LGTM. The prior buffered-logprobs finding is addressed by per-choice accumulation and one-time attachment, with regression cases covering final flushes with and without additional tokens.
Reviewed both changed files and the directly affected parser and endpoint contracts. No actionable findings in this light review. Static review only; tests were not executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex
Background
A review of recent inference changes on
mainfound thatStreamingChatParser, which drives OpenAI-compatible streaming in the dynamic text-gen server, commits reasoning deltas as soon as the reasoning parser returns them. A later chunk can still move that text, and text that was already streamed cannot be taken back.What changed
openai_streaming.py: while reasoning is still open, hold back a trailing partial end marker (for example</thi, or a partial implicit marker such as<tool_call>) instead of streaming it as reasoning. This adds_partial_marker_start(now also used by_safe_content_prefix) and_safe_reasoning_prefix.StreamingChatParsertakes two new arguments,reasoning_markersandforce_content. Some parsers return reasoning as content until a non-empty answer follows, then move it back to reasoning. NemotronV3 does this withenable_thinking=Falseorforce_nonempty_content=True. For those parsers no deltas are sent until the split is stable. The parser is either named explicitly or detected by a one-time probe ofparse. The final parse flushes everything.pending_logprobs) and attached to the next chunk that carries text, including the final flush.tests/unit_tests/inference/test_openai_streaming.py, includingtest_openai_stream_keeps_logprobs_of_parser_held_back_text. It covers a final flush with and without additional tokens.Details
The callers are
endpoints/chat_completions.pyandendpoints/completions.py. There are no dependency or lock changes.The branch merges
mainatee6baa2e(#8263, which seeds CPU initialization in the checkpoint load test that timed out on an earlier head). Against that base it changes only the two files above.Review 5547002215 (logprobs dropped for held-back text) is fixed in
9bd15b20. Review 5547215667 on this head reports no findings.Tested
test_openai_streaming.pyat this head: 15 passed, 0 failed, 15 skipped. Without the fix, the new logprobs test fails in 2 of 2 cases.tests/unit_tests/inference/**at72de4874: 3680 passed, 37 skipped, 0 failed.tests/unit_tests/**/*.py, merged withmainb8305562: 3424 passed, 142 skipped, 0 failed, then 24 of 24 in the second session.9bd15b20is pending.