-
Notifications
You must be signed in to change notification settings - Fork 302
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
* Store protos in local cache (#3022) * Store proto obj instead of model Literal in local cache Signed-off-by: Eduardo Apolinario <[email protected]> * Remove unused file Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]> * Bump aiohttp from 3.9.5 to 3.10.11 (#3018) Bumps [aiohttp](https://github.com/aio-libs/aiohttp) from 3.9.5 to 3.10.11. - [Release notes](https://github.com/aio-libs/aiohttp/releases) - [Changelog](https://github.com/aio-libs/aiohttp/blob/master/CHANGES.rst) - [Commits](aio-libs/aiohttp@v3.9.5...v3.10.11) --- updated-dependencies: - dependency-name: aiohttp dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fix bug in FlyteDirectory.listdir on local files (#2926) * Fix issue in FlyteDirectory.listdir Fixes flyteorg/flyte#6005 Signed-off-by: Pim de Haan <[email protected]> * Added test Signed-off-by: Pim de Haan <[email protected]> * Run make lint Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Pim de Haan <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]> * Fix unit tests in airflow plugin (#3024) Signed-off-by: Kevin Su <[email protected]> * fix: Fix resource meta typos for async agent (#3023) Signed-off-by: JiaWei Jiang <[email protected]> * fix: format commands output (#3026) * Fix pydantic basemodel default input (#3013) * Fix pydantic default input Signed-off-by: Future-Outlier <[email protected]> * add pydantic integration test Signed-off-by: Future-Outlier <[email protected]> * Use duck typing by Thomas's advice Signed-off-by: Future-Outlier <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]> * lint Signed-off-by: Future-Outlier <[email protected]> --------- Signed-off-by: Future-Outlier <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]> * [BUG] Open FlyteFile from remote path (#2991) * fix: Open FlyteFile from remote path Signed-off-by: JiaWei Jiang <[email protected]> * Add integration test Signed-off-by: JiaWei Jiang <[email protected]> * refactor: Use ctx as param instead of recreation Signed-off-by: JiaWei Jiang <[email protected]> * refactor: Clean test logic 1. Remove redundant prints 2. Use `mock.patch.dict` to setup `os.environ` for the current test fn * Avoid contaminating other tests running in the same process Signed-off-by: JiaWei Jiang <[email protected]> * refactor: Setup local path and downloader in constructor Signed-off-by: JiaWei Jiang <[email protected]> * refactor: Move SimpleFileTransfer to an utility file Signed-off-by: JiaWei Jiang <[email protected]> * Remove redundant env var setup Please refer to #3001 Signed-off-by: JiaWei Jiang <[email protected]> * test: Add another ff use case Create ff in one task pod and read it in another task pod. Signed-off-by: JiaWei Jiang <[email protected]> --------- Signed-off-by: JiaWei Jiang <[email protected]> * vllm inference plugin (#2967) * vllm inference plugin Signed-off-by: Daniel Sola <[email protected]> * fixed default value Signed-off-by: Daniel Sola <[email protected]> --------- Signed-off-by: Daniel Sola <[email protected]> * Add poetry to image spec (#3025) * Add poetry to image spec Signed-off-by: Thomas J. Fan <[email protected]> * Add stricter check Signed-off-by: Thomas J. Fan <[email protected]> --------- Signed-off-by: Thomas J. Fan <[email protected]> * [test] Add integration test for accessing sd sttr in dc (#2969) * test: Add integration test for attr access of sd Signed-off-by: JiaWei Jiang <[email protected]> * Correct file path Signed-off-by: JiaWei Jiang <[email protected]> * test: Support interaction with minio s3 bucket 1. Upload a local parquet file to minio s3 bucket 2. Access StructuredDataset attr from a dataclass 3. Open StructuredDataset from a remote path Signed-off-by: JiaWei Jiang <[email protected]> * Delete an unmerged integration test Signed-off-by: JiaWei Jiang <[email protected]> * Try imagespec with commit sha of corresponding fix Signed-off-by: JiaWei Jiang <[email protected]> * Remove redundant test Signed-off-by: JiaWei Jiang <[email protected]> * Remove default_factory and create sd dc from input uri Signed-off-by: JiaWei Jiang <[email protected]> * refactor: Clean test logic 1. Remove redundant prints 2. Use `mock.patch.dict` to setup `os.environ` for the current test fn * Avoid contaminating other tests running in the same process Signed-off-by: JiaWei Jiang <[email protected]> * Remove redundant minio env var setup and add test comments Signed-off-by: JiaWei Jiang <[email protected]> * Support uploading tmp pqt file Signed-off-by: JiaWei Jiang <[email protected]> * Udpate deprecated module Signed-off-by: JiaWei Jiang <[email protected]> * Remove redundant and unused imports Signed-off-by: JiaWei Jiang <[email protected]> --------- Signed-off-by: JiaWei Jiang <[email protected]> --------- Signed-off-by: Eduardo Apolinario <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Pim de Haan <[email protected]> Signed-off-by: Kevin Su <[email protected]> Signed-off-by: JiaWei Jiang <[email protected]> Signed-off-by: Future-Outlier <[email protected]> Signed-off-by: Daniel Sola <[email protected]> Signed-off-by: Thomas J. Fan <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pim de Haan <[email protected]> Co-authored-by: Kevin Su <[email protected]> Co-authored-by: 江家瑋 <[email protected]> Co-authored-by: V <[email protected]> Co-authored-by: Han-Ru Chen (Future-Outlier) <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]> Co-authored-by: Daniel Sola <[email protected]>
* Fix pydantic default input Signed-off-by: Future-Outlier <[email protected]> * add pydantic integration test Signed-off-by: Future-Outlier <[email protected]> * Use duck typing by Thomas's advice Signed-off-by: Future-Outlier <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]> * lint Signed-off-by: Future-Outlier <[email protected]> --------- Signed-off-by: Future-Outlier <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]> Signed-off-by: Shuying Liang <[email protected]>
* Fix pydantic default input Signed-off-by: Future-Outlier <[email protected]> * add pydantic integration test Signed-off-by: Future-Outlier <[email protected]> * Use duck typing by Thomas's advice Signed-off-by: Future-Outlier <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]> * lint Signed-off-by: Future-Outlier <[email protected]> --------- Signed-off-by: Future-Outlier <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]> Signed-off-by: Shuying Liang <[email protected]>
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