Fix DiscriminatedUnionMixin issues with Pydantic C bindings #1555
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fixes issues with DiscriminatedUnionMixin when used with Pydantic's C bindings (pydantic-core) where discriminator values could get mixed up between different subclasses. Key changes: - Remove direct assignment to __pydantic_core_schema__, __pydantic_validator__, and __pydantic_serializer__ - these are now managed by Pydantic internally - Remove model_rebuild() override that was modifying model_fields after class construction and using TypeAdapter to bypass Pydantic's lifecycle - Simplify __init_subclass__ to just set the kind field annotation and mark rebuild as required, rather than manually rebuilding abstract base classes - Keep __get_pydantic_core_schema__ for generating proper discriminated union schemas for abstract types - Remove unused TypeAdapter and ClassVar imports The refactored implementation: - Uses Pydantic's recommended hooks without bypassing its internal lifecycle - Maintains backward compatibility with existing code using the mixin - The kind field is still automatically set to the class name for concrete types - Serialization/deserialization of polymorphic hierarchies continues to work - JSON schema generation produces valid discriminated union schemas Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||
The previous implementation called model_json_schema() or TypeAdapter().json_schema() for abstract classes, which created new schema generation contexts. This caused infinite recursion when a concrete subclass (like PipelineCondenser) contained a field referencing the abstract parent type (like condensers: list[CondenserBase]). The fix always uses the provided handler to generate the base schema first, which participates in pydantic's reference tracking and avoids recursion. For abstract classes, we now only add discriminator info if the handler produces a oneOf schema. Co-authored-by: openhands <openhands@all-hands.dev>
tofarr
force-pushed
the
fix/discriminated-union-pydantic-cbindings
branch
from
eeafa0c to
5b43d17
Compare
tofarr
force-pushed
the
fix/discriminated-union-pydantic-cbindings
branch
from
92e9d3d to
9b82aa2
Compare
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
- Add thread-local storage for schema cache to ensure thread safety - Add detailed warning docstring about repr string parsing fragility - Replace dataclasses.MISSING with None as sentinel value - Add comment explaining sanity check assert statement - Update OpenHandsModel docstring with deprecation notice - Re-add missing ERROR status assertion in test Co-authored-by: openhands <openhands@all-hands.dev>
enyst
reviewed
Jan 2, 2026
Collaborator
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
Thank you for this!
Speaking of serialization... I think something happened here
https://github.com/OpenHands/docs/pull/229/files#r2656786421
Collaborator
|
@OpenHands /codereview-roasted |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
Code Review Summary (Roasted Style)I provided a critical code review of PR #1555 "Fix DiscriminatedUnionMixin issues with Pydantic C bindings" with a 🔴 Needs Improvement rating. Critical Issues Identified:
Improvement Opportunities:
Verdict:❌ Needs rework - The PR removed ~250 lines of "hackery" but replaced it with different hackery (repr parsing). Neither approach is sustainable. If Pydantic doesn't provide the APIs needed, the recommendation is to work with the Pydantic team to add those APIs rather than building on undocumented behavior. No code changes were made - this was a review-only request. |
xingyaoww
added
the
integration-test
Contributor
|
Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly. |
Contributor
🧪 Integration Tests ResultsOverall Success Rate: 0.0% 📁 Detailed Logs & ArtifactsClick the links below to access detailed agent/LLM logs showing the complete reasoning process for each model. On the GitHub Actions page, scroll down to the 'Artifacts' section to download the logs.
📊 Summary
📋 Detailed Resultslitellm_proxy_gpt_5.1_codex_max
Skipped Tests:
Failed Tests:
litellm_proxy_claude_sonnet_4_5_20250929
Failed Tests:
litellm_proxy_deepseek_deepseek_chat
Skipped Tests:
Failed Tests:
litellm_proxy_vertex_ai_gemini_3_pro_preview
Failed Tests:
litellm_proxy_mistral_devstral_2512
Skipped Tests:
Failed Tests:
litellm_proxy_moonshot_kimi_k2_thinking
Skipped Tests:
Failed Tests:
|
xingyaoww
reviewed
Jan 2, 2026
Collaborator
xingyaoww
left a comment
xingyaoww
left a comment
There was a problem hiding this comment.
This LGTM as long as everything in agent server is working correctly and pass all integration tests!
Appreciate this nice clean up 🙏
xingyaoww
added
integration-test
Contributor
|
Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly. |
Collaborator
Author
|
Unfortunately the repr parsing is the only way to get this working at present - given that the pydantic is using simple wrappers around rust functions. I call this out with the warning: Also, the extra unit tests mean that if pydantic change this format about 15 separate unit tests will break. |
Contributor
🧪 Integration Tests ResultsOverall Success Rate: 98.0% 📁 Detailed Logs & ArtifactsClick the links below to access detailed agent/LLM logs showing the complete reasoning process for each model. On the GitHub Actions page, scroll down to the 'Artifacts' section to download the logs.
📊 Summary
📋 Detailed Resultslitellm_proxy_deepseek_deepseek_chat
Skipped Tests:
litellm_proxy_gpt_5.1_codex_max
Skipped Tests:
litellm_proxy_moonshot_kimi_k2_thinking
Skipped Tests:
litellm_proxy_mistral_devstral_2512
Skipped Tests:
Failed Tests:
litellm_proxy_claude_sonnet_4_5_20250929
litellm_proxy_vertex_ai_gemini_3_pro_preview
|
xingyaoww
approved these changes
Jan 2, 2026
Collaborator
xingyaoww
left a comment
xingyaoww
left a comment
There was a problem hiding this comment.
LGTM! This is a definitely a net positive improvemnts
tofarr
pushed a commit
that referenced
this pull request
Jan 2, 2026
After a recent change in PR #1555, the discriminator field was removed from the generated JSON schema for discriminated unions. This caused the OpenAPI documentation tests to fail since Swagger UI needs the discriminator field to properly display discriminated unions. This commit re-adds the discriminator field with the proper mapping in the __get_pydantic_json_schema__ method. The test expectation was also updated to include the discriminator field. Co-authored-by: openhands <openhands@all-hands.dev>
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Refactors
DiscriminatedUnionMixinto work correctly with Pydantic's C bindings by leveraging Pydantic's native mechanisms instead of fighting against them.Key Changes
Architecture Simplification
kindis now a@computed_field- Returns class name dynamically instead of being a mutable field that gets patched after construction@model_validatorand@model_serializer- Handle polymorphic validation/serialization through Pydantic's supported hooks__get_pydantic_json_schema__- Generates properoneOfschemas for abstract base classesmodel_rebuild()hackery - No longer bypasses Pydantic'scomplete_model_class()or patchesmodel_fieldspost-constructionCode Quality Improvements
threading.local()instead of global dict for_schemas_in_progressOpenHandsModel- No longer required; new code should extendBaseModeldirectlyUnknown kind 'X' for module.Class; Expected one of: [...]model_computed_fields.keys()exclusion inmcp/tool.pyandtool/schema.pyWhat Was Removed (~250 lines)
_rebuild_requiredglobal flag andrebuild_all()function_create_enhanced_discriminated_union_error_message()and related helpers_patch_fastapi_discriminated_union_support()and_extract_discriminated_unions()__pydantic_core_schema__,__pydantic_validator__,__pydantic_serializer__Backward Compatibility
OpenHandsModelstill exists (as empty shell) for external code that extends itkindfield behavior unchanged from consumer perspectiveChecklist
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:5e92f01-pythonRun
All tags pushed for this build
About Multi-Architecture Support
5e92f01-python) is a multi-arch manifest supporting both amd64 and arm645e92f01-python-amd64) are also available if needed