Refactor compile_annotation to use Pydantic TypeAdapter throughout #3194
Conversation
- Replace manual BaseModel field iteration with model_json_schema() - Add _strip_titles helper to remove title keys from generated schemas - Handle nested from Pydantic's schema generation - Remove PydanticUndefined import (no longer needed) - Remove test_compile_function_definition__lambda test (dropping support for unannotated lambda parameters) Co-Authored-By: [email protected] <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Replace manual type handling with TypeAdapter for all annotation types - Simplify nested handling by merging Pydantic's output directly - Update tests to match Pydantic's schema generation (inlined schemas, enum values, etc.) - Remove unused Literal import and fix whitespace issues Co-Authored-By: [email protected] <[email protected]>
devin-ai-integration
bot
changed the title
- Change #//vellum.client.types.chat_message.ChatMessage to #//ChatMessage - Update test_basic_tool_calling_node_serialization.py expectations - Update simple_composio_tool_calling_node.json fixture - Aligns with Pydantic's TypeAdapter schema generation behavior Co-Authored-By: [email protected] <[email protected]>
- Remove type_map dict that was only used in the now-removed fallback - Replace fallback with clearer ValueError that wraps the original exception - All tests pass - fallback was never hit in the test suite - Simplifies code by fully trusting Pydantic's TypeAdapter Co-Authored-By: [email protected] <[email protected]>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Use Pydantic's TypeAdapter for everything else | ||
| try: | ||
| schema = TypeAdapter(annotation).json_schema() | ||
| schema = _strip_titles(schema) |
There was a problem hiding this comment.
Parameters without type hints now hit the TypeAdapter call and raise instead of defaulting to {"type": "null"} as before. Any call to compile_function_definition on a function whose parameters are unannotated (where param.annotation is inspect._empty) will now propagate a ValueError from TypeAdapter, whereas the previous type_map explicitly handled that case. This breaks existing usage of untyped functions.
Useful? React with 👍 / 👎.
| # Merge any nested $defs into the top-level defs dict | ||
| if "$defs" in schema: | ||
| nested_defs = schema.pop("$defs") | ||
| defs.update(nested_defs) |
There was a problem hiding this comment.
Merging nested $defs directly into the shared defs dict uses the short names emitted by TypeAdapter (typically the class __name__), so two different models with the same class name (e.g., foo1.Foo and foo2.Foo) will overwrite one another and both $refs will resolve to whichever schema was processed last. The previous implementation used module-qualified names to keep definitions unique; the new approach can yield incorrect schemas whenever duplicate class names appear in a single function/workflow.
Useful? React with 👍 / 👎.
devin-ai-integration
bot
force-pushed
the
devin/1763843204-use-pydantic-model-json-schema
branch
from
6b61cb2 to
3bf693e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
devin-ai-integration
bot
force-pushed
the
devin/1763843204-use-pydantic-model-json-schema
branch
from
3bf693e to
6b61cb2
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Use Pydantic's TypeAdapter for everything else | ||
| try: | ||
| schema = TypeAdapter(annotation).json_schema() |
There was a problem hiding this comment.
Parameters without type hints arrive as inspect._empty, but compile_annotation now falls through to TypeAdapter(annotation) which raises because _empty is not a valid type. Previously these were mapped to {"type": "null"}, so compiling an unannotated tool/workflow function succeeded; now the same call surfaces a ValueError and blocks schema generation for any function with untyped args.
Useful? React with 👍 / 👎.
| # Merge any nested $defs into the top-level defs dict | ||
| if "$defs" in schema: | ||
| nested_defs = schema.pop("$defs") | ||
| defs.update(nested_defs) |
There was a problem hiding this comment.
Nested $defs emitted by TypeAdapter are merged with a raw defs.update, so if two different dataclasses/Pydantic models share the same class name (e.g., two ChatMessage types from different modules), the second schema overwrites the first while both parameters still point to #/$defs/ChatMessage. That produces an incorrect schema for the first parameter; the prior implementation used fully qualified names to avoid this collision.
Useful? React with 👍 / 👎.
Comprehensively refactors
compile_annotationto use Pydantic'sTypeAdapterfor all annotation types instead of manual type handling. This simplifies the code from ~100 lines to ~35 lines and ensures we leverage Pydantic's native schema generation capabilities.Summary
The
compile_annotationfunction previously manually handled each annotation type (Union, Literal, dict, list, tuple, dataclasses, BaseModel, etc.) with custom logic. This PR refactors it to use Pydantic'sTypeAdapterfor all types except special cases (None, Any, datetime, Json Union, ForwardRef).Key changes:
TypeAdapter(annotation).json_schema()for all annotation types_strip_titles()helper to recursively remove"title"keys that Pydantic adds to schemas$defshandling by merging Pydantic's output directly into top-level defs dictLiteralimport andtest_compile_function_definition__lambdatest$refnaming (e.g.,#/$defs/ChatMessageinstead of#/$defs/vellum.client.types.chat_message.ChatMessage)Behavioral changes:
$refnames (e.g.,ChatMessage) instead of fully qualified names (e.g.,vellum.client.types.chat_message.ChatMessage)$ref+$defs(e.g., the schema is directly embedded in the property rather than referenced)MyEnum.FOObecomes"foo")listnow includes"items": {}and unparameterizeddictincludes"additionalProperties": true(more correct JSON Schema)Review & Testing Checklist for Human
$refnames like#/$defs/ChatMessageinstead of fully qualified names. Verify that no downstream code relies on the old fully qualified naming convention (e.g., string matching, schema validation, serialization/deserialization logic).$ref+$defs. Verify this doesn't break downstream consumers (LLM function calling, workflow serialization, etc.) that may expect or rely on$refs.inspect._empty(unannotated parameters). Functions with unannotated parameters will now raiseValueErrorinstead of returning{"type": "null"}. Verify this is acceptable or add back support if needed.defs.update(nested_defs)merge. If two different models have the same class name from different modules (e.g.,foo1.Fooandfoo2.Foo), the second will overwrite the first. Verify this edge case doesn't occur in practice or add collision detection if needed.Test Plan
$refnames (e.g.,vellum.client.types.chat_message.ChatMessage)Notes
All existing tests pass after updating expectations to match Pydantic's output. The refactor significantly simplifies the code and delegates schema generation to Pydantic, which should be more maintainable long-term. However, the behavioral changes (especially
$refnaming and schema inlining) are significant and warrant careful testing with downstream consumers. Only two places in the codebase referenced the old fully qualified$refnames (both in tests/fixtures), which have been updated.