[Bugifx] [Qwen3CoderTool] Restore supports_required_and_named for required tool_choice#42292
Conversation
…uired tool_choice Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request removes the supports_required_and_named attribute from the Qwen3CoderToolParser class. The review feedback indicates that this change introduces a conflict with the parser's XML-based structural tags, as it forces the system to use a JSON extractor that is incompatible with the XML output, leading to failures when tool_choice is set to 'required'.
…uired tool_choice Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…uired tool_choice Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
bbrowning
left a comment
There was a problem hiding this comment.
Agree we don't want to regress here for user's that have not opted into the strict tool calling enforcement via Xgrammar.
|
A thought - it feels like to integrate xgrammar's stag templates better with more model archs, major refactoring of output parser (like RFC #32713) is necessary. xgrammar clearly moves toward a "unified" parser; it supports full-time structured decoding (contrary to the in-the-middle-of-tool-calling structured decoding that vllm currently takes) and each stag template binds tightly to the target model, and this kind of movement is not unique to xgrammar, smg as another example - any token parser framework that works atop of token-in-token-out inference engine would naturally look like that. And vllm's current parser system that guarantees independency of reasoning and tool call parser makes it hard to develop such a unified parser design without a sort of backward incompatibility - one instance is this bugfix, and supporting a request-level thinking on/off alongside forced stag is also quite messy in its current form (currently stag is bound to a tool call parser, which has little info on reasoning parser counterpart). There are several places in the scheduler code that handle reasoning / tool call boundary which explicitly assume that reasoning / tool call parser are independent. If a backward incompatibility is unavoidable, it might be better starting to write "vllm parser V2" that lies in separate codepath from the vllm parser status quo and put xgrammar stag feature to parser V2 I think. |
…uired tool_choice (vllm-project#42292) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…uired tool_choice (vllm-project#42292) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…uired tool_choice (vllm-project#42292) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…uired tool_choice (vllm-project#42292) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Conflict resolutions (qwen3coder_tool_parser.py + its test file): - _convert_param_value: kept this branch's detailed type-coercion logic (nullable string/None handling, container double-decode for buggy templates) instead of main's refactor to utils.coerce_to_schema_type / extract_types_from_schema (vllm-project#38973). Restored `import ast` that vllm-project#38973 had removed. Kept main's vllm-project#42292 change (supports_required_and_named = not VLLM_ENFORCE_STRICT_TOOL_CALLING). - Tests: kept this branch's rewritten Coder test file. Re-integrated the two anyOf tests vllm-project#38973 added: the comprehensive non-streaming + streaming cases stay Coder-specific (they assert {"type": ["integer","null"]} -> int and whitespace-stripped values, which only hold for the Coder parser); the genuinely cross-parser anyOf[array,null] -> list case was added to the shared test file, parametrized over both XML and Coder parsers. All 190 qwen3 tool-parser tests pass; ruff check/format unchanged vs the pre-merge branch head. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Purpose
Follow of #40894
As a widely used tool parser, Qwen3Coder should still support supports_required_and_named = True at this stage.
Otherwise, tool_choice="required" no longer works correctly now.
I think this should continue to be properly supported at least before structural_tag is enabled by default.
@sfeng33 @aarnphm @Seven-Streams @aarnphm @bbrowning
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.