Skip to content

[Bugfix][Frontend] Keep the trailing parameter when qwen3 closes the function without it - #57707

Open
he-yufeng wants to merge 1 commit into
vllm-project:mainfrom
he-yufeng:fix/qwen3-unclosed-last-param
Open

he-yufeng wants to merge 1 commit into
vllm-project:mainfrom
he-yufeng:fix/qwen3-unclosed-last-param

Conversation

@he-yufeng

Copy link
Copy Markdown
Contributor

Purpose

Fixes #57699. _qwen3_arg_converter ended a parameter value only at </parameter> or the next <parameter=. Models sometimes close </function> without closing the last parameter, and then:

  • non-streaming: the final conversion (partial=False) dropped the trailing value — arguments came back without it (with several parameters, the call silently lost the last one);
  • streaming: partial conversions had already streamed the value, but the flush at TOOL_CALL_END produced {} — shorter than what was already sent, so nothing more was emitted and the client kept the clipped version.

Not a duplicate: the two earlier qwen3 parser fixes I checked (#43243's JSON-first parsing, and the null-handling change) cover different shapes; nothing open touches this one.

The fix is to run the trailing-parameter capture in both modes instead of only in partial mode. The accumulated raw body never contains the </function> marker (the engine consumes it as a terminal event), so the trailing value runs cleanly to the end of the buffer. Empty trailing values behave exactly like explicitly-closed empty parameters.

Test Plan

python -m pytest tests/parser/engine/test_qwen3.py -q
python -m pytest tests/parser/ -q --ignore=tests/parser/cohere -p no:randomly
ruff check / ruff format --check on the two touched files

Three new tests pin the shape: the BFCL example from the issue (non-streaming), earlier params kept with an unclosed tail (non-streaming), and no truncation in the streamed flush.

Test Result

  • Red-green: all three new tests fail on the unpatched converter (non-streaming returns {}/drops the tail; the streamed flush truncates), pass with the fix.
  • tests/parser/engine/test_qwen3.py: 54 passed (51 existing + 3 new).
  • tests/parser/: 4608 passed with -p no:randomly (the default random-order run surfaces a pre-existing ordering-sensitive flake in tests/parser/mistral/test_reasoning.py, present with and without this change; that file passes standalone in both states). tests/parser/cohere needs the optional cohere_melody package, not installed here.
  • ruff check / ruff format: clean.
  • No GPU model eval: parser-level change; the issue's raw model text is pinned as a unit test.

AI assistance: prepared with AI assistance (Kimi Code). I reviewed every changed line and ran the tests above.

@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.

@he-yufeng

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (54020c3) after the Streaming Parser Engine rewrite landed.

Worth being explicit about what changed around this PR: #45701 moved qwen3 parsing into the engine, but the bug this PR fixes still reproduces on main. The engine's flush converts with partial=False, and the one-shot answer comes from that conversion, so the unclosed trailing parameter is still dropped there. Measured on current main before this rebase:

extract_tool_calls('<tool_call><function=get_weather>'
  '<parameter=city>Seattle</parameter>'
  '<parameter=unit>celsius</function></tool_call>')
-> {"city": "Seattle"}            # main: trailing unit dropped
-> {"city": "Seattle", "unit": "celsius"}   # this head

(The engine never puts the marker into the accumulated raw body, so the trailing capture runs cleanly to end of buffer. The sibling tail-class PRs for minimax_m2/glm47/deepseek_v4 were covered by the rewrite and closed; qwen3's was not.)

Verification on the rebased head: regression input and the fully-closed control both convert identically (no double counting), and the engine qwen3 suite passes 54/54 including the new TestUnclosedLastParameter cases (.venv/bin/python -m pytest tests/parser/engine/test_qwen3.py -q).

@mergify

mergify Bot commented Sep 29, 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, @he-yufeng.

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 Sep 29, 2026
@he-yufeng
he-yufeng force-pushed the fix/qwen3-unclosed-last-param branch from 08cdd8b to fe595e4 Compare September 29, 2026 15:33
@mergify mergify Bot removed the needs-rebase label Sep 29, 2026
…function without it

_qwen3_arg_converter only flushed a parameter value at </parameter> or
the next <parameter=. When the model closes </function> without closing
the last parameter, the final conversion (partial=False) dropped that
trailing value: non-streaming returned arguments without it, and the
TOOL_CALL_END flush emitted "{}" shorter than what partial conversions
had already streamed, clipping it away client-side (vllm-project#57699).

Run the trailing-parameter capture in both modes instead of only in
partial mode; the accumulated raw body never contains the </function>
marker, so the value runs cleanly to the end of the buffer.

Co-authored-by: Kimi Code <kimi-code@moonshot.cn>
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
@he-yufeng
he-yufeng force-pushed the fix/qwen3-unclosed-last-param branch from fe595e4 to 5b3884a Compare September 30, 2026 07:37
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (8ee4069). The conflict mergify flagged no longer exists against the current tree: nothing under vllm/parser/ has moved since the flagged base, so the rebase applied cleanly with no manual resolution.

pytest tests/parser/engine/test_qwen3.py -q on the rebased head: 69 passed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working qwen Related to Qwen models tool-calling

Projects

Status: No status

2 participants