-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix pydantic basemodel default input #3013
Conversation
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
if is_imported("pydantic"): | ||
try: | ||
from pydantic import BaseModel as BaseModelV2 | ||
from pydantic.v1 import BaseModel as BaseModelV1 | ||
|
||
if issubclass(python_type, BaseModelV2): | ||
default_val = default_val.model_dump_json() | ||
elif issubclass(python_type, BaseModelV1): | ||
default_val = default_val.json() | ||
except ImportError: | ||
# Pydantic BaseModel v1 | ||
default_val = default_val.json() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it enough to duck type here?
if hasattr(default_val, "model_dump_json"):
# pydantic v2
default_val = default_val.model_dump_json()
elif hasattr(default_val, "json"):
# pydantic v1
default_val = default_val.json()
else:
encoder = JSONEncoder(python_type)
default_val = encoder.encode(default_val)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it is, you are amazing Thomas, always learn something from you!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3013 +/- ##
===========================================
+ Coverage 51.08% 96.54% +45.45%
===========================================
Files 201 11 -190
Lines 21231 723 -20508
Branches 2731 0 -2731
===========================================
- Hits 10846 698 -10148
+ Misses 9787 25 -9762
+ Partials 598 0 -598 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Future-Outlier <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #c862a3Actionable Suggestions - 1
Review Details
|
Changelist by BitoThis pull request implements the following key changes.
|
if hasattr(default_val, "model_dump_json"): | ||
# pydantic v2 | ||
default_val = default_val.model_dump_json() | ||
elif hasattr(default_val, "json"): | ||
# pydantic v1 | ||
default_val = default_val.json() | ||
else: | ||
encoder = JSONEncoder(python_type) | ||
default_val = encoder.encode(default_val) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider consolidating the JSON serialization logic into a single helper function to handle both Pydantic v1 and v2 cases along with the default case using JSONEncoder
. This would improve maintainability and reduce code duplication.
Code suggestion
Check the AI-generated fix before applying
@@ -478,9 +478,15 @@
- if hasattr(default_val, "model_dump_json"):
- # pydantic v2
- default_val = default_val.model_dump_json()
- elif hasattr(default_val, "json"):
- # pydantic v1
- default_val = default_val.json()
- else:
- encoder = JSONEncoder(python_type)
- default_val = encoder.encode(default_val)
+ default_val = _serialize_to_json(default_val, python_type)
+
def _serialize_to_json(val, python_type):
+ if hasattr(val, "model_dump_json"):
+ # pydantic v2
+ return val.model_dump_json()
+ elif hasattr(val, "json"):
+ # pydantic v1
+ return val.json()
+ else:
+ encoder = JSONEncoder(python_type)
+ return encoder.encode(val)
Code Review Run #c862a3
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Tracking issue
flyteorg/flyte#5318
Why are the changes needed?
We should make default input for pydantic basemodel works.
What changes are being proposed in this pull request?
Support default input handling for Pydantic
BaseModel
:Updated the
to_click_option
function to properly handle default values for PydanticBaseModel
inputs.Adopted Duck Typing for cleaner and more Pythonic code:
Replaced explicit type checks with
Duck Typing
to improve code readability and align with Python's philosophy of emphasizing behavior over specific types.referecne: PEP-0020, the Zen of Python: https://peps.python.org/pep-0020/#the-zen-of-python
How was this patch tested?
local execution, remote execution, and integration test.
Setup process
Screenshots
local execution
remote execution
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR implements enhanced support for Pydantic BaseModel default inputs in Flytekit by adding proper serialization handling for both Pydantic v1 and v2 versions. The changes focus on improving default value handling in CLI options through duck typing and best practices, with comprehensive test coverage.Unit tests added: True
Estimated effort to review (1-5, lower is better): 1