[Bugfix] Fix Qwen3CoderToolParser anyOf/oneOf type resolution for nullable params#37831
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
There was a problem hiding this comment.
Code Review
This pull request provides a crucial bugfix for tool parameter type resolution in Qwen3CoderToolParser, specifically for anyOf/oneOf schemas representing nullable types. The change correctly extracts the actual non-null type instead of hardcoding it as "object", which resolves issues with non-object nullable parameters. My review identifies a remaining edge case where the new logic does not handle a valid JSON Schema construct where the type field is an array (e.g., {"type": ["integer", "null"]}). I've provided a suggestion to make the type extraction more robust to cover this scenario.
aa6a797 to
82f5622
Compare
…lable params The previous fix (vllm-project#36032) hardcoded param_type = "object" for all anyOf schemas. This breaks nullable parameters with non-object types (e.g. Optional[int], Optional[str], Optional[list]) because they get routed through json.loads instead of their correct conversion branch (int(), float(), etc.). Pydantic v2 emits anyOf for every Optional[T] field: Optional[int] → {"anyOf": [{"type": "integer"}, {"type": "null"}]} This fix extracts the first non-null type from anyOf/oneOf variants, so each parameter hits its correct type conversion path. Signed-off-by: AAISSJ <maze0717@g.skku.edu>
Address review feedback: JSON Schema allows type to be an array
(e.g. {"type": ["integer", "null"]}). The previous code would
stringify the array, leading to unrecognized param_type. Now we
extract the first non-null element from type arrays as well.
Signed-off-by: AAISSJ <maze0717@g.skku.edu>
- Handle {"type": ["integer", "null"]} at the top-level type field,
not just inside anyOf/oneOf variants
- Add test_extract_tool_calls_anyof_type_conversion covering:
anyOf[integer|null], anyOf[string|null], anyOf[array|null],
anyOf[object|null], and top-level type-as-array
Signed-off-by: AAISSJ <maze0717@g.skku.edu>
The anyOf type conversion is implemented in Qwen3CoderToolParser, not Qwen3XMLToolParser. Use the coder parser fixture directly instead of the parametrized fixture (which only runs xml). Signed-off-by: AAISSJ <maze0717@g.skku.edu>
- Extract duplicated non-null type extraction into _first_non_null_type static method, reused for both top-level type-as-array and anyOf variants - Extract _resolve_param_type to flatten nesting in _convert_param_value - Add multi non-null type test case: anyOf[string, integer, null] verifies "first non-null type wins" policy Signed-off-by: AAISSJ <maze0717@g.skku.edu>
Parameters using $ref (e.g. Pydantic model inputs) have no "type" field. Treat them as "object" to route through json.loads, matching the behavior proposed in vllm-project#37652 but integrated into the _resolve_param_type helper. Added ref_param test case to verify $ref → object → json.loads path. Signed-off-by: AAISSJ <maze0717@g.skku.edu>
82f5622 to
5fd2434
Compare
| resolved = self._first_non_null_type(param_def["type"]) | ||
| return resolved or "string" | ||
|
|
||
| if "anyOf" in param_def or "oneOf" in param_def: |
There was a problem hiding this comment.
LGTM.
Thanks. Could you write an end-to-end test for this?
There was a problem hiding this comment.
Hi, @chaunceyjiang
I've added a streaming e2e test (test_extract_tool_calls_anyof_type_conversion_streaming) that exercises the full pipeline: tokenize → incremental decode → extract_tool_calls_streaming with anyOf nullable schemas. Both non-streaming and streaming paths now have coverage for type resolution.
Update: Also added $ref coverage to the streaming e2e test — the filters parameter uses {"$ref": "#/$defs/SearchFilters"} to verify that $ref schemas are correctly resolved to "object" in the streaming pipeline.
Please let me know if there's anything else you'd like me to address.
Covers the full streaming pipeline (tokenize → incremental decode → extract_tool_calls_streaming) with anyOf nullable schemas to verify that string/integer/boolean types are correctly resolved and returned. Signed-off-by: AAISSJ <maze0717@g.skku.edu>
The anyOf streaming test only covered string, integer, and boolean params. Add a $ref parameter (filters) to verify that object type resolution works correctly in the full streaming pipeline. Signed-off-by: AAISSJ <maze0717@g.skku.edu>
fed85bc to
22cdf04
Compare
|
I'd be in favor of using this more comprehensive PR instead of mine (#37652). |
|
Thanks~ @schoennenbeck @AAISSJ Please fix the pre-commit errors. |
Signed-off-by: <> Signed-off-by: AAISSJ <maze0717@g.skku.edu>
b963182 to
d552b46
Compare
Thanks, @chaunceyjiang @schoennenbeck |
|
Hi, @chaunceyjiang |
|
@chaunceyjiang Would be great to get this merged before the next release. Qwen3.5-family are some of the most capable free models and currently in vllm they are incompatible with up to date versions of pydantic-ai. |
|
This PR broke "Async Engine, Inputs, Utils, Worker, Config (CPU)" in CI, proof: |
… for nullable params (vllm-project#37831)" This reverts commit 582340f.
|
@AAISSJ I can tackle this. But since this is your PR I thought I'd ask first. |
|
It looks like we had an ordering issue between #38189 and this one. CI didn't catch this because nothing forced CI to re-run between when 38189 merged and when this merged, so this one had stale valid CI checks making it look good to merge. |
…lable params (vllm-project#37831) Signed-off-by: AAISSJ <maze0717@g.skku.edu> Signed-off-by: <> Co-authored-by: 세덩 <saison@sedeong-ui-MacBookAir.local>
… for nullable params (vllm-project#37831)" (vllm-project#38751)
Purpose
Fix incorrect type resolution for
anyOf/oneOfschemas, type-as-array patterns, and$refschemas inQwen3CoderToolParser._convert_param_value.The previous fix (#36032) hardcoded
param_type = "object"for allanyOfschemas. While this solved the double-encoding issue for object-typedanyOfparams, it breaks nullable parameters with non-object types — they get routed throughjson.loadsinstead of their correct conversion branch (int(),float(), etc.).This PR also subsumes the
$refhandling proposed in #37652, integrated cleanly into the same_resolve_param_typehelper.Root Cause
Pydantic v2 emits
anyOffor everyOptional[T]field:With the #36032 fix, this
anyOfis resolved toparam_type = "object", so the value"5"goes throughjson.loads("5")— works by accident for integers, but for strings it would try to parse a plain string as JSON and fail or behave unexpectedly.Similarly, Pydantic model inputs produce
$refschemas (e.g.{"$ref": "#/$defs/ToolInput"}) with no"type"field, which also fell through to"string".Fix
Two new helper methods to keep type resolution logic clean and DRY:
_first_non_null_type(type_value)— static method that extracts the first non-null type from either a scalar ("integer") or a type-as-array (["integer", "null"]). Reused in both top-level type resolution and anyOf variant scanning._resolve_param_type(param_def)— resolves the effective type string from a parameter definition. Handles four cases:"type"field (including type-as-array)anyOf/oneOfvariants (extracts first non-null type from variants)$refschemas (treated as"object"to triggerjson.loads)"string"This replaces the previous inline logic with deeply nested if/elif/else blocks, using early returns for clarity.
Affected Cases
"object")anyOf[integer, null]"integer"→int()✅anyOf[string, null]"string"→ as-is ✅anyOf[array, null]"array"→json.loads✅anyOf[object, null]"object"→json.loads✅anyOf[object, object](#36032 case)"object"→json.loads✅anyOf[string, integer, null]"string"(first non-null) ✅{"type": ["integer", "null"]}(top-level)"integer"→int()✅anyOf[{"type": ["integer", "null"]}](nested)"integer"→int()✅{"$ref": "#/$defs/Model"}(#37652 case)"object"→json.loads✅Also handles
oneOf(used by FastAPI and other frameworks) and filters null types expressed as both{"type": "null"}(string) and{"type": null}(Python None, seen in some Pydantic outputs).Test Plan
Added
test_extract_tool_calls_anyof_type_conversioncovering 7 patterns:Existing test suite passes with no regressions:
Follow-up
Qwen3XMLToolParser._get_param_typehas the same issue — it usesproperties[param_name].get("type", "string")which falls back to"string"foranyOfschemas. This PR intentionally scopes the fix toQwen3CoderToolParseronly. A follow-up PR for the XML parser will be submitted once this is merged.