Generate and forward recursive schemas for recursive types - #771
Conversation
`_inline_refs` followed every `$ref` to its target and deleted every `$defs`. On a recursive type there is no fixed point to reach, so it recurred until the stack ran out -- the `RecursionError` in #761. That reached users through `Encodable`, since `_pydantic_type_tuple` inlines the schema of a model built over the element types: type A = int | list[A] pydantic.TypeAdapter(Encodable[tuple[A, int]]).json_schema() Inlining was only ever a way to keep a `$ref` out of a `WithJsonSchema` value, which pydantic/pydantic#12145 rejects. Re-addressing the references achieves that without expanding anything, and a reference is what the providers want in any case: measured against five of them with the returned arguments validated against the intended schema, every ref-free encoding of a cycle either fails the request or corrupts the data, while `$defs` and `$ref` are accepted and correctly filled. It is also what OpenAI documents. So `_bundle_refs` renames each definition to a content hash, anchors it with an `$id` and points the references at that absolute URI -- the one ref form pydantic's counter tolerates, and the only one meaningful in a fragment, since `#/$defs/X` addresses the root of whatever document the fragment is embedded in rather than its own. `_rebundle` undoes that once the document is whole: definitions hoisted to the root, references back to pointers, and the keywords beside a `$ref` dropped, since a provider reads a reference as the whole subschema rather than composing it with what sits beside it. Recursive types remain unsupported on the `openrouter/google/...` route, which drops a `$ref` and sends the object it named as a string. That is OpenRouter's translation rather than Gemini -- a direct `gemini/` call on a 2.0+ model passes `$defs` through -- and it already affects ordinary repeated field types today, so it is left as its own issue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jfeser
left a comment
jfeser
left a comment
There was a problem hiding this comment.
Per chatgpt, hashing by the schema definition can cause us to conflate distinct definitions:
In module_a.py:
class Leaf(BaseModel):
value: int
class Parent(BaseModel):
leaf: Leaf
In module_b.py:
class Leaf(BaseModel):
value: str
class Parent(BaseModel):
leaf: Leaf
In module_c.py:
class Response(BaseModel):
int_val: module_a.Parent
str_val: module_b.Parent
I'm worried that we're starting to replicate one of the more complex parts of json schema handling, which previously we had delegated to pydantic. Chatgpt suggests that the use of pydantic.WithJsonSchema is problematic because it means that pydantic can't do this resolution for us.
This PR is simpler and more correct than what's in
Pydantic does not do the necessary resolution at all for user-generated schemas, hence the need for a workaround. Moving away from |
|
Chatgpt suggests using |
What does this mean? Slop in the form of runnable failing test cases is one thing but I can't make heads or tails of this suggestion. AFAIK the only mechanism other than |
The reference handling this branch added is unnecessary. Every `WithJsonSchema` it worked around was passing a schema *dict* generated from a type we were already holding, and pydantic cannot see into a dict -- hence pydantic/pydantic#12145, and hence the renaming. Naming the type instead, through `json_schema_input_type` on the validator and `return_type` on the serializer, hands pydantic the same shape in a form it understands. It then owns every definition and reference, including the recursive ones, which is what #761 needed and what the review asked for. So `_inline_refs` goes, and so do the `_bundle_refs`/`_rebundle` pair that replaced it, the definition hashing, and the three `_ensure_strict_json_schema` calls -- litellm applies that pass to the whole document anyway. The tests added here go with them; they covered machinery that no longer exists, and what remains is covered by the suite as it stands on master. Four serializers had to hand back the model rather than a dict, which `return_type` expects and which pydantic otherwise only warns about. That also retires the manual per-field adapter loops in the tuple encodings: the model's own fields carry the element encodings. The `NamedTuple` branch gains a validator, since `WithJsonSchema` with no mode had been covering both directions and `return_type` covers only one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
I found a different workaround that gets rid of the explicit schemas and associated schema-munging. It seems |
2 participants
Resolves #761
This PR removes the wonky inlining logic for schemas with references (which are ubiquitous in Pydantic output) in favor of simply renaming relative references to absolute ones at the appropriate points. This has the nice side effect of correctly handling recursive schemas like the failure case in #763.