Repository navigation
[https://nvbugs/6926284][fix] Align qwen3_coder parameter typing with vLLM - #20279
JunyiXu-nv merged 11 commits into
Conversation
… vLLM Qwen3CoderToolParser typed parameter values differently from vLLM's Qwen3 parser, so the same model output produced different tool-call arguments on the two servers: - Only one type was read from the schema, so type lists, anyOf/oneOf and enums took the wrong branch. vLLM tries every accepted type in the order null, integer, number, boolean, object, array, string. - A boolean was false unless it read "true". vLLM accepts true/1 and false/0 and keeps any other text. - "null" became None for every parameter, including string parameters and parameters without a schema. - Values were stripped and HTML-unescaped, which changed indentation, trailing whitespace and literal entities in string arguments. vLLM only removes the newline the chat template puts on each side. - Unknown types and non-JSON objects went through ast.literal_eval. - A non-finite number raised OverflowError and failed the request. Value typing now follows vLLM's extract_types_from_schema and coerce_to_schema_type, and a tool's parameter schemas are read from parameters.properties only. When several parameters complete in one streaming delta they are emitted in the order they were written, so the streamed arguments match the whole-text parse. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
… parameter tag When the model left out a parameter's closing tag, every parameter after it was swallowed into its value. vLLM's Qwen3 parser ends such a parameter at the next <parameter= tag and accepts whitespace inside the tags. Both the streaming and the whole-text paths now use vLLM's parameter pattern. As a result, the whole-text parse now drops an unterminated last parameter, as vLLM does and as the streaming path already did. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughQwen3 Coder parsing now uses property schemas to convert parameter values. Streaming and non-streaming parsing share parameter matching, including recovery when a closing tag is missing. Streaming output preserves model-written parameter order. ChangesQwen3 Coder parameter parsing
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 84.21% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 2 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title follows the required ticket, type, and summary format. It clearly identifies the main change: aligning Qwen3 Coder parameter typing with vLLM. |
| Description check | |
The description includes the required Description, Test Coverage, and PR Checklist sections. It explains the problem, solution, scope, compatibility differences, and extensive test coverage. The check… |
Full details: Description check
-
Explanation
The description includes the required Description, Test Coverage, and PR Checklist sections. It explains the problem, solution, scope, compatibility differences, and extensive test coverage. The checklist is included and reviewed.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
PR_Github #77776 [ run ] triggered by Bot. Commit: |
|
PR_Github #77776 [ run ] completed with state
|
…wlines, CRLF, escapes and true A value is the exact text between the parameter tags, minus one newline on each side: extra newlines, carriage returns and written escape sequences are kept, and true stays text unless the schema allows a boolean. Each case is checked whole and streamed in 3-character deltas. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…meout lint (NVIDIA#20296) Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #77915 [ run ] triggered by Bot. Commit: |
|
PR_Github #77915 [ run ] completed with state
|
… cases where TRT-LLM and vLLM disagreed Padded and entity-bearing strings, null as text for string and schema-less parameters, null where the schema allows it, a padded boolean, 1 and other text for a boolean, a padded null for an integer, digits for a string-or-null union in both type-list and anyOf form, and a Python-literal object, each checked whole and streamed. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…DIA#20362) Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #78098 [ run ] triggered by Bot. Commit: |
|
PR_Github #78098 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #78494 [ run ] triggered by Bot. Commit: |
|
PR_Github #78494 [ run ] completed with state |
|
PR_Github #78814 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #79071 [ run ] triggered by Bot. Commit: |
|
PR_Github #79071 [ run ] completed with state |
…coder-parser-parity Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com> # Conflicts: # tensorrt_llm/serve/tool_parser/qwen3_coder_parser.py
|
/bot run --disable-fail-fast |
|
PR_Github #79307 [ run ] triggered by Bot. Commit: |
|
PR_Github #79307 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #79384 [ run ] triggered by Bot. Commit: |
|
PR_Github #79384 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #79408 [ run ] triggered by Bot. Commit: |
|
PR_Github #79408 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #79512 [ run ] triggered by Bot. Commit: |
|
PR_Github #79512 [ run ] completed with state |
…coder-parser-parity Resolve qwen3_coder_parser.py against NVIDIA#20422 (required tool choice for Nemotron Super 3.5 VL), which restructured the same parser: - Main's structure is kept: build_required_structural_tag, parse_partial, finish, the complete flag and _find_closing_tag for call and function boundaries, and strict parameters in grammar-constrained output. - This branch's parameter handling is kept for unconstrained output: vLLM's schema typing, a parameter that ends at the next parameter tag when its closer is missing, a last unclosed parameter kept with a warning, and arguments streamed in the order written. - Without </function>, a function body now ends before the tool call's closer, and when the text is final a closer that ends it also ends an unclosed last parameter, so such a call is still parsed instead of returned as text. - The streaming half of test_unterminated_last_parameter_ends_with_function now calls finish(), as the serving layer does: an unclosed last parameter is only known to have ended once the stream does. Tool parser, Responses streaming tool-call, serving-extension and reasoning tests: 1377 passed. The vLLM parity corpus (1252 cases) still matches vLLM. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #80090 [ run ] triggered by Bot. Commit: |
|
PR_Github #80090 [ run ] completed with state
|
…rser-parity Picks up the waive of unittest/executor/test_forward_error_survival.py (NVIDIA#20895, nvbug 6981910), which fails on this branch's base in the CPU stages independently of this change. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #80164 [ run ] triggered by Bot. Commit: |
|
PR_Github #80164 [ run ] completed with state |
Dev Engineer Review
The supplied parser source implements schema-based coercion, limited newline trimming, and parameter recovery at the next parameter tag. The factory maps
nemotron_h_omnitoqwen3_coder. However, the reported working-tree diff contains no parser source change, so these behaviors are not established as changes in that diff. No current review findings or test-run results were supplied.QA Engineer Review
tests/unittest/llmapi/apps/test_tool_parsers.pyis modified, with 62 insertions and 119 deletions. The focused diff shows no Qwen3Coder behavior-test changes; it removes blank lines in that class. Other visible changes reformat imports and multiline statements. The file is listed in the CI test list attests/integration/test_lists/test-db/l0_cpu.yml. Coverage verdict: needs follow-up, because the current diff adds no Qwen3Coder behavior coverage for the described parser changes. Test execution status is unavailable.Per-File QA Perspective
tests/unittest/llmapi/apps/test_tool_parsers.py— The Qwen3Coder test changes shown are whitespace-only; the focused diff does not add or alter behavior assertions. The file is listed intests/integration/test_lists/test-db/l0_cpu.yml.tensorrt_llm/serve/tool_parser/qwen3_coder_parser.py— The supplied source includes schema coercion, newline trimming, and next-tag parameter boundaries, but this file is not modified in the reported working-tree diff.tensorrt_llm/serve/tool_parser/tool_parser_factory.py— The factory mapsnemotron_h_omnitoqwen3_coder; this file is not modified in the reported working-tree diff.Description
Fixes https://nvbugs/6926284.
Qwen3CoderToolParsertyped parameter values differently from vLLM's Qwen3 parser (nowvllm/parser/qwen3.py, withcoerce_to_schema_typeinvllm/tool_parsers/utils.py), so the same model output produced different tool-call arguments on the two servers. Across 1,252 schema/value combinations, main disagreed with vLLM main on 508 and raised on 4:["integer", "null"]),anyOf/oneOfandenumtook the wrong branch. vLLM tries every accepted type in the order null, integer, number, boolean, object, array, string.truebecamefalse. vLLM acceptstrue/1andfalse/0, ignoring case and surrounding whitespace, and keeps any other text.null. It becameNonefor every parameter, including string parameters and parameters without a schema. vLLM returnsNoneonly when the schema allows null.<in string arguments (code, for example). vLLM only removes the newline the chat template puts on each side of a value.ast.literal_eval; vLLM decodes JSON only.numberparameter ofinfraisedOverflowErrorand failed the request; vLLM keeps the text.This PR ports vLLM's
extract_types_from_schemaandcoerce_to_schema_type, trims only the wrapping newlines, and reads a tool's parameter schemas fromparameters.propertiesonly, as vLLM does. Streaming also emitted parameters that completed in the same delta in set order; they now follow the order they were written, so the streamed arguments equal the whole-text parse.Parameter boundaries. When the model left out a parameter's closing
</parameter>, every parameter after it was swallowed into its value. Both the streaming and the whole-text paths now use vLLM's parameter pattern, which ends such a parameter at the next<parameter=tag and accepts whitespace inside the tags. A last parameter without</parameter>runs to the end of its function, whole and streamed alike, and a warning names it; main and SGLang keep it the same way. The whole-text parse now ends a function at the</function>that is followed by the next function or by the end of the tool call, so a value that contains</function>stays whole, as it already did when streaming.Where this differs from vLLM. In two places, both where vLLM's result is not usable:
</parameter>: vLLM's whole-text parse drops it, and replaying its streaming argument path shows a partial value that is never closed ({"a": 1, "b": "never closed).Nonewhere a parameter accepts null but not a string (taken from [None][fix] Preserve string values in Qwen3-Coder tool arguments #20423 by @eopXD): vLLM keeps the text"None", which the schema rejects; this PR reads it as null. Where a string is accepted,Nonestays text.The rest of #20423 is not taken. HTML unescaping and Python-literal containers would break cases in the MLPerf issue linked in review (
keep &and{'x': 1}must stay as written), and trying the string type first would turn5underanyOf: [string, integer]into"5", unlike both main and vLLM.The
nemotron_h_omnimodel type uses this parser too, so its tool-call arguments change the same way.One existing test expectation changes: an unknown-type parameter that holds a Python literal (
{'arg1': 3, 'arg2': [1, 2]}) is now kept as text, as vLLM keeps it.Test Coverage
tests/unittest/llmapi/apps/test_tool_parsers.py::TestQwen3CoderToolParser(CPU-Generic-x86-1, CPU-Generic-arm-1): new tests cover schema-driven typing (booleans, type lists,anyOf,enum, null,None, non-finite numbers, Python literals), verbatim string values, values without a schema, streaming that matches the whole-text parse when several parameters complete in one delta, a parameter whose closing tag is missing (mid-call, and last with or without</function>), function tags inside a value, and the string/null cases from [None][fix] Preserve string values in Qwen3-Coder tool arguments #20423. Against main's parser, 31 of the class's 61 tests fail; on this branch all pass.be4ce2e505): 1,252 schema/value combinations, compared with vLLM's_qwen3_arg_converter,_fix_arg_typesandcoerce_to_schema_typerun from the upstream source. This branch differs on none of them, and streaming in 3-character deltas and in a single delta both match the whole-text parse. On malformed output, a missing</parameter>mid-call and whitespace inside a parameter tag also match vLLM; a last parameter without</parameter>is kept, as described above.TestCoerceToSchemaTypeandTestExtractTypesFromSchema(80 tests) pass against the ported functions.PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.🤖 Generated with Claude Code