Skip to content

[Frontend] Make finish_reason and Responses item status truthful - #59324

Merged
chaunceyjiang merged 5 commits into
vllm-project:mainfrom
sfeng33:fix-finish-reason-truthful
Oct 15, 2026
Merged

chaunceyjiang merged 5 commits into
vllm-project:mainfrom
sfeng33:fix-finish-reason-truthful

Conversation

@sfeng33

@sfeng33 sfeng33 commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

Purpose

Fixes #53255, #58228, #57998. Also covers the finish_reason half of #53363 and #50889.

Chat finish_reason and Responses item status both claimed more than what happened:

  • Chat, non-streaming. tool_choice="required" ending with engine stop but zero parsed tool calls returned finish_reason="tool_calls" next to an empty tool_calls array. An auto tool call parsed from a generation cut by max_tokens returned tool_calls instead of length, so a client could not tell a completed turn from one cut mid-arguments. The streaming path already guarded both ([Bugfix][Frontend] Keep length finish_reason for max_tokens-truncated streaming tool calls #46303), so the same request answered differently depending on stream. The /derender chat paths carried two more hand-written copies of the policy, one with the same max_tokens defect.
  • Responses, non-harmony. Every ResponseOutputMessage and ResponseFunctionToolCall was hardcoded status="completed", even inside a response whose top-level status is incomplete with incomplete_details.reason = max_output_tokens. The harmony path already did this correctly. In streaming the last response.output_item.done said completed and the stream ended with response.completed instead of the spec's response.incomplete.
  • Responses, harmony streaming. The terminal-event code is shared, so a gpt-oss stream whose response status is incomplete now also ends with response.incomplete instead of response.completed.

Relation to open PRs

This PR is a superset of the open partial fixes and makes all four sites share one rule:

Test Plan

Live A/B on H200 with Qwen/Qwen3-0.6B, --enable-auto-tool-choice --tool-call-parser hermes --reasoning-parser qwen3 --max-model-len 4096 (plus a second pair of servers with no tool parser), this branch vs merge-base dfc8e0f3e2, 29 + 9 requests (chat non-stream/stream, n=2, Responses non-stream/stream, Anthropic) replayed twice at temperature=0. max_tokens=18 cuts the model right after the closing brace of the tool-call JSON, so a call is parsed but the engine reports length.

Test Result

Unit: 21 finish_reason tests and 130 Responses tests pass; full test_serving_chat.py 195 passed (GPT-OSS fixture errors also occur on main). pre-commit clean including mypy.

Live A/B, only these fields changed; every other response (complete calls, named, streaming chat, plain text, n=2, 400s without a parser) was byte-identical:

Request before after
/v1/chat/completions, tool_choice=auto, max_tokens=18 (call parsed, cut) finish_reason: "tool_calls" "length"
same, stream=true "length" "length" (now consistent)
/v1/responses, max_output_tokens=6 (message cut) item status: "completed" "incomplete"
/v1/responses, tool_choice=required, max_output_tokens=18 (call cut) item "completed" "incomplete"
same two, stream=true output_item.done: completed, ends response.completed incomplete, ends response.incomplete
no-parser server, /v1/responses, max_output_tokens=6 item "completed" "incomplete"
/v1/messages, auto tool, max_tokens=18 stop_reason: "tool_use" "max_tokens"
required / named with a complete call "tool_calls" / "stop" unchanged
curl -s localhost:8000/v1/chat/completions -H 'Content-Type: application/json' -d '{
  "model": "Qwen/Qwen3-0.6B", "temperature": 0, "max_tokens": 18,
  "messages": [{"role": "user", "content": "What'"'"'s the weather in Paris? Use the tool."}],
  "tool_choice": "auto", "chat_template_kwargs": {"enable_thinking": false},
  "tools": [{"type": "function", "function": {"name": "get_weather",
    "parameters": {"type": "object", "properties": {"city": {"type": "string"}}, "required": ["city"]}}}]}'

curl -s localhost:8000/v1/responses -H 'Content-Type: application/json' -d '{
  "model": "Qwen/Qwen3-0.6B", "temperature": 0, "max_output_tokens": 6,
  "input": "Write a long paragraph about the sea.", "chat_template_kwargs": {"enable_thinking": false}}'

@mergify mergify Bot added the frontend label Sep 30, 2026
@sfeng33
sfeng33 force-pushed the fix-finish-reason-truthful branch from 4802065 to 8598b63 Compare September 30, 2026 00:18
@sfeng33 sfeng33 changed the title [Frontend] Report finish_reason truthfully for tool calls [Frontend] Make finish_reason and Responses item status truthful Sep 30, 2026
@mergify mergify Bot added the needs-rebase label Sep 30, 2026
Chat completions reported finish_reason="tool_calls" whenever
tool_choice="required" ended with an engine "stop", even when no tool
call was parsed, and whenever an auto tool call was parsed, even when
generation was truncated by max_tokens. The streaming path already
guarded both, so the same request answered differently depending on
`stream`. The /derender paths carried two more copies of the policy.

Introduce one shared rule, resolve_finish_reason(): "tool_calls" only
when the choice actually carries tool calls, the engine finish reason
is "stop" and tool_choice is not a single named tool (OpenAI documents
"stop" for a forced function). Any other engine reason such as "length"
is preserved. Use it from streaming and non-streaming chat and from
both derender paths. The Anthropic /v1/messages converter derives
stop_reason from this value and inherits the fix.

The non-harmony Responses path hardcoded status="completed" on every
output message and function call, even inside a response whose
top-level status is "incomplete" with reason max_output_tokens. Derive
the status of the last item of a generation from its finish reason,
matching the harmony path. In streaming, carry that status into the
final output_item.done event and end an incomplete response with the
spec's response.incomplete event instead of response.completed.

Fixes vllm-project#53255, vllm-project#58228, vllm-project#57998. Also covers the finish_reason half of

Signed-off-by: Flora Feng <4florafeng@gmail.com>
@sfeng33
sfeng33 force-pushed the fix-finish-reason-truthful branch from 8598b63 to 9341514 Compare September 30, 2026 00:24
@vllm-project vllm-project deleted a comment from mergify Bot Sep 30, 2026
@sfeng33
sfeng33 marked this pull request as ready for review September 30, 2026 00:31

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@hickeyma hickeyma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @sfeng33, this looks good. Having one resolve_finish_reason() for chat and both /derender paths is what I was hoping for. Its good that the returning length for a cut auto tool call matches streaming chat and the Rust frontend.

A few small things, none blocking but good to get them in if possible:

  1. If the cut lands in reasoning, non-streaming leaves the reasoning item with status=None but streaming marks it incomplete through emit_simple_reasoning_done. Harmony doesn't set a status on reasoning, so I'd drop it from the streaming done event.
  2. With parallel_tool_calls=false, the filter runs before build_response_output_items. So for [complete, cut], the complete call is the one that gets marked incomplete. It's rare and fine to leave but maybe add a comment.
  3. Harmony streams now end with response.incomplete too because the terminal event code is shared. That's correct but it's worth a line in the description.
  4. Tests:
    • Could you add one for background replay stopping on response.incomplete?
    • The batch derender test would fit better in test_derender_stream.py using the existing parsed_derenderer fixture, instead of bare MagicMocks in test_derender.py
    • An expect_tool_calls column in the parametrize would read better than working the expectation out from the input text

@mergify

mergify Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @sfeng33.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Oct 4, 2026
@hickeyma

hickeyma commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

@chaunceyjiang Do you mind taking a look when you get a chance?

@hickeyma

hickeyma commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@sfeng33 Do you mind pushing this along as it will unblock a lot of issues?

@chaunceyjiang

Copy link
Copy Markdown
Contributor

/cc @sfeng33 could you resolve the merge conflicts ?

@sfeng33
sfeng33 force-pushed the fix-finish-reason-truthful branch from 9341514 to 32ef089 Compare October 13, 2026 21:43
@sfeng33
sfeng33 requested a review from noooop as a code owner October 13, 2026 21:43
…ruthful

Signed-off-by: sfeng33 <4florafeng@gmail.com>
GenerateResponse is now a tagged union on main, so it has no model_validate.

Signed-off-by: sfeng33 <4florafeng@gmail.com>
@sfeng33
sfeng33 force-pushed the fix-finish-reason-truthful branch from 32ef089 to 2900085 Compare October 13, 2026 21:51
Without the stop, retrieving a stored stream that ended incomplete waits
forever for more events.

Signed-off-by: sfeng33 <4florafeng@gmail.com>
@sfeng33

sfeng33 commented Oct 13, 2026

Copy link
Copy Markdown
Member Author

Thanks @hickeyma, I merged main.

  1. Kept. Streaming used to say completed for a cut reasoning item, and incomplete is the true status. The None in non-streaming was there before this PR.
  2. Rare, and the response is incomplete anyway, so I left it.
  3. Added to the description.
  4. Added the background replay test. I skipped the two style changes.

@chaunceyjiang chaunceyjiang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks~

@chaunceyjiang chaunceyjiang added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 15, 2026
@chaunceyjiang

Copy link
Copy Markdown
Contributor

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #95074 for commit 88aa27a600de.

@chaunceyjiang
chaunceyjiang enabled auto-merge (squash) October 15, 2026 08:47

@hickeyma hickeyma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @sfeng33, LGTM.

On point 1, I agree incomplete is the right status for a reasoning item that got cut. My concern is that non-streaming still returns status=None for the same request. A generation cut inside reasoning comes back incomplete when streamed and None
otherwise which is the stream vs non-stream mismatch this PR is fixing elsewhere. It also matters for continuation. should_continue_final_message treats an incomplete reasoning item as something to continue, so the next turn behaves differently depending on which path made the item.

Rather than dropping it from streaming, could non-streaming set it too when reasoning is the last item? In build_response_output_items:

status=last_item_status if not content and not tool_calls else None

That keeps your semantics and makes both paths agree. Happy for it to be a follow-up.

@chaunceyjiang
chaunceyjiang merged commit 80dd705 into vllm-project:main Oct 15, 2026
106 checks passed
@sfeng33
sfeng33 deleted the fix-finish-reason-truthful branch October 15, 2026 13:30
michael-gpu pushed a commit to practical-inference/vllm-ampere-extended that referenced this pull request Oct 20, 2026
…m-project#59324)

Signed-off-by: Flora Feng <4florafeng@gmail.com>
Signed-off-by: sfeng33 <4florafeng@gmail.com>
Co-authored-by: Chauncey <chaunceyjiang@gmail.com>
wangyicong52 added a commit to bytedance-iaas/vllm that referenced this pull request Oct 26, 2026
Use the upstream finish-reason policy for streaming and non-streaming Chat
responses. Keep length visible after parsing a tool call, and report
normal tool completion only when a call was actually emitted.

Backport the Chat portion of 80dd705,
including the streaming guard previously merged in vllm-project#46303. Responses and
derender changes from that PR are outside this deployment's Chat path.

Validation: 3 serving regressions fail before the fix; all 7 targeted cases
pass afterward, including recovered truncated DeepSeek-V4.1 arguments.
Broader CPU serving checks: 35 passed, with the same 9 missing-tokenizer/config
cache failures reproduced on the unchanged baseline. Changed-file hooks pass.
No GPU model run.

Original-Pull-Request: vllm-project#59324
Related-Pull-Request: vllm-project#46303
Co-authored-by: Flora Feng <4florafeng@gmail.com>
Co-authored-by: Chauncey <chaunceyjiang@gmail.com>
Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: wangyicong <wangyicong@bytedance.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: finish_reason="tool_calls" is returned with an empty tool_calls array when tool_choice="required" produces no call

3 participants