fix: unwrap single-variant anyOf to preserve sibling fields in Vertex AI schema - #19270
fix: unwrap single-variant anyOf to preserve sibling fields in Vertex AI schema#19270VedantMadane wants to merge 1 commit into
Conversation
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
c73cf62 to
2ed3fcc
Compare
e529bad to
4399ac3
Compare
67a3926 to
6e7252a
Compare
Greptile SummaryThis PR fixes a schema field-loss bug in Vertex AI schema transformation where Pydantic-generated Key changes:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| litellm/llms/vertex_ai/common_utils.py | Adds single-variant anyOf unwrapping to preserve sibling fields (default, examples, description). Logic is correct for the Optional[X] use-case, but the unwrap is also applied to single-variant anyOf without null — a broader change than the PR description implies. No critical bugs. |
| tests/test_litellm/llms/vertex_ai/test_vertex_ai_common_utils.py | New tests cover the sibling-field preservation case, the variant-wins-on-conflict case, and updated existing tests to reflect the new unwrapping behaviour. No network calls; all mocked. Good coverage of the happy paths. |
| litellm/completion_extras/litellm_responses_transformation/transformation.py | Single-line change: moves the # noqa: PLR0915 directive from a standalone comment (where ruff ignores it) onto the def line where it is effective. No functional change. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["convert_anyof_null_to_nullable(schema)"] --> B{"anyOf present?"}
B -- No --> G["Recurse into properties / items"]
B -- Yes --> C["Scan variants: remove null, set contains_null=True"]
C --> D{"len(anyof) == 0?"}
D -- Yes --> E["raise ValueError (only-null schema)"]
D -- No --> F{"contains_null?"}
F -- Yes --> H["Set nullable=True on every remaining variant"]
F -- No --> I["(no-op)"]
H --> J{"len(anyof) == 1?"}
I --> J
J -- No --> G
J -- Yes --> K["Collect siblings (all keys except anyOf)"]
K --> L["schema.clear()"]
L --> M["schema.update(siblings)"]
M --> N["schema.update(single_variant)\n(variant fields win on conflict)"]
N --> G
G --> O["Return (schema mutated in-place)"]
O --> P["filter_schema_fields calls _filter_anyof_fields"]
P --> Q{"anyOf key present?"}
Q -- Yes --> R["Strip sibling fields (old behaviour)"]
Q -- No --> S["No stripping — siblings preserved ✓"]
Last reviewed commit: 88c7c3f
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| previous_response_id=getattr( | ||
| chat_completion_response, "previous_response_id", None | ||
| ), | ||
| previous_response_id=getattr( | ||
| chat_completion_response, "previous_response_id", None | ||
| ), |
There was a problem hiding this comment.
Duplicate keyword argument causes SyntaxError
The previous_response_id keyword argument is passed twice to the ResponsesAPIResponse constructor. This is a Python syntax error introduced by the merge conflict resolution in commit 6e7252aad ("Resolve merge conflict in responses transformation"). Python will raise SyntaxError: keyword argument repeated at parse time, meaning this file cannot be imported at all.
Remove the duplicate:
| previous_response_id=getattr( | |
| chat_completion_response, "previous_response_id", None | |
| ), | |
| previous_response_id=getattr( | |
| chat_completion_response, "previous_response_id", None | |
| ), | |
| previous_response_id=getattr( | |
| chat_completion_response, "previous_response_id", None | |
| ), |
| if len(anyof) == 1: | ||
| single_variant = anyof[0] | ||
| # Collect sibling fields (everything except 'anyOf') | ||
| siblings = {k: v for k, v in schema.items() if k != "anyOf"} | ||
| # Clear the schema and replace with merged content | ||
| schema.clear() | ||
| # Add sibling fields first (can be overwritten by variant fields if there's a conflict) | ||
| schema.update(siblings) | ||
| # Merge the single variant's fields (these take precedence) | ||
| schema.update(single_variant) |
There was a problem hiding this comment.
Sibling fields may silently override variant fields
The merge order gives the single variant precedence over siblings (schema.update(single_variant) last). However, if a sibling field has the same key as a field inside the variant (e.g., both have "description"), the variant's value silently wins with no warning. This is the correct priority for most cases, but consider the scenario where the user sets a "description" as a sibling of anyOf and the inner variant also has a "description" — the sibling's description would be lost.
This is a minor concern and may be acceptable since it's an uncommon edge case, but it would be worth documenting this merge precedence in a brief inline comment so future maintainers understand the tradeoff.
| def _map_optional_params_to_responses_api_request( # noqa: PLR0915 | ||
| def _map_optional_params_to_responses_api_request( |
There was a problem hiding this comment.
Duplicate def line causes SyntaxError
The fix for the ruff: noqa placement (commit 57dd237d1) converted the comment into a full def line but did not remove the original def on line 224. This results in two consecutive function definition lines — Python will raise a SyntaxError because line 223 is a def with no body. The file cannot be imported at all.
| def _map_optional_params_to_responses_api_request( # noqa: PLR0915 | |
| def _map_optional_params_to_responses_api_request( | |
| def _map_optional_params_to_responses_api_request( # noqa: PLR0915 |
|
Automated patch bundle from next-100 unresolved backlog expansion.\nGenerated due limited direct branch-write access; please apply/cherry-pick minimal edits below.\n\n## PR #19270 — Unresolved thread summary
Minimal patch proposals
|
| if len(anyof) == 1: | ||
| single_variant = anyof[0] | ||
| # Collect sibling fields (everything except 'anyOf') | ||
| siblings = {k: v for k, v in schema.items() if k != "anyOf"} | ||
| # Clear the schema and replace with merged content | ||
| schema.clear() | ||
| # Start with sibling metadata, then let variant fields override | ||
| # so core schema-defining fields (type, enum, etc.) from the | ||
| # variant are never silently clobbered by siblings. | ||
| schema.update(siblings) | ||
| schema.update(single_variant) |
There was a problem hiding this comment.
Unwrap fires for all single-variant anyOf, not only post-null-removal
The if len(anyof) == 1: check is not gated on contains_null, so it triggers for any schema that already has exactly one variant (without a null peer), e.g. {"anyOf": [{"type": "string"}], "default": "hello"}. The PR description says "after convert_anyof_null_to_nullable removes the null type", implying this is only for Optional patterns, but the implementation is broader.
For Vertex AI this is generally safe (single-variant anyOf is semantically identical to the unwrapped form), and it's arguably better behaviour because it also preserves siblings in the non-null single-variant case. However, it is a wider behaviour change than the PR description suggests and there's no test covering this path. Consider either:
- Guarding the unwrap with
if contains_null:to match the documented intent, or - Adding a test for the non-null single-variant case to document the intended broader scope.
# Option 1 – narrower, matches PR description:
if contains_null and len(anyof) == 1:
88c7c3f to
14429dd
Compare
- Fix duplicate def line in transformation.py (SyntaxError) - Ensure correct sibling field merging in Vertex AI schema transformation - Rebase onto latest upstream/main
14429dd to
f63fbf4
Compare
|
Addressed review feedback from #pullrequestreview-3874764358.
Please take another look. Thank you! |
4 participants
Relevant issues
Fixes #19255
Summary
When Pydantic generates
Optional[X], it creates:{ "anyOf": [{"type": "integer", "minimum": 0}, {"type": "null"}], "default": null, "examples": [7, 30, 90], "description": "Filter by number of days" }The constraints (
default,examples,pattern) are siblings ofanyOf.After
convert_anyof_null_to_nullableremoves the null type, if only ONE variant remains inanyOf, we now unwrap it and merge the sibling fields. This prevents_filter_anyof_fieldsfrom stripping them.Before (sibling fields lost):
{"anyOf": [{"type": "integer", "minimum": 0}], "default": null, "examples": [...]} -> {"anyOf": [{"type": "integer", "minimum": 0, "nullable": true}]} // default and examples are LOST!After (sibling fields preserved):
{"anyOf": [{"type": "integer", "minimum": 0}], "default": null, "examples": [...]} -> {"type": "integer", "minimum": 0, "nullable": true, "default": null, "examples": [...]}Pre-Submission checklist
tests/litellm/directory - Addedtest_anyof_conversion_preserves_sibling_fieldsChanges
litellm/llms/vertex_ai/common_utils.py:convert_anyof_null_to_nullableto unwrap single-variant anyOf arrays and merge sibling fieldstests/test_litellm/llms/vertex_ai/test_vertex_ai_common_utils.py:test_anyof_conversion_preserves_sibling_fieldsto verify the fixtest_basic_anyof_conversionto expect unwrapped single-variant anyOftest_build_vertex_schemaandtest_build_vertex_schema_empty_propertiesto reflect new behavior