-
Notifications
You must be signed in to change notification settings - Fork 304
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
[WIP][need discussion] Pydantic Transformer guess python type #3060
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #b2a53cActionable Suggestions - 2
Review Details
|
Changelist by BitoThis pull request implements the following key changes.
|
except Exception as e: | ||
raise TypeTransformerFailedError(f"Failed to create Pydantic model from schema: {e}") |
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.
Exception handling can be improved by using 'raise ... from err' pattern and avoiding f-strings in exceptions.
Code suggestion
Check the AI-generated fix before applying
except Exception as e: | |
raise TypeTransformerFailedError(f"Failed to create Pydantic model from schema: {e}") | |
except Exception as err: | |
error_msg = f"Failed to create Pydantic model from schema: {err}" | |
raise TypeTransformerFailedError(error_msg) from err |
Code Review Run #b2a53c
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
json_type = json_type[0] | ||
else: | ||
# More complex unions can be handled here if needed | ||
json_type = "string" # default fallback |
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 implementing more robust handling of complex union types instead of defaulting to string
. The current implementation may lead to data loss or incorrect type conversions for union types with multiple variants.
Code suggestion
Check the AI-generated fix before applying
json_type = "string" # default fallback | |
variants = [type_mapping.get(t, Any) for t in json_type] | |
if len(variants) > 1: | |
return Union[tuple(variants)] # type: ignore | |
return variants[0] |
Code Review Run #b2a53c
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3060 +/- ##
===========================================
- Coverage 72.87% 36.01% -36.86%
===========================================
Files 205 202 -3
Lines 21553 21414 -139
Branches 2746 2752 +6
===========================================
- Hits 15707 7713 -7994
- Misses 5062 13593 +8531
+ Partials 784 108 -676 ☔ View full report in Codecov by Sentry. |
Need Discussion
[need discussion]
I found a problem when supporting guess python type for pydantic basemodel.
json schema -> python class
Since the JSON schema can be converted into either a dataclass or a Pydantic BaseModel, we need to include additional metadata in the literal type’s metadata.
This ensures we know whether to generate a Pydantic BaseModel or a dataclass from the JSON schema.
Tracking issue
flyteorg/flyte#5318
Related issue
pydantic/pydantic#643
flyteorg/flyte#6081
Why are the changes needed?
People want to use remote api to execute workflow with pydantic basemodel input.
This work in single binary and Union Cluster.
What changes were proposed in this pull request?
Implement a non-perfect
guess_python_type
in pydantic basemodel transformer.How was this patch tested?
integration test and remote execution.
It's hard to test in unit test because we make lots of type
Any
.Setup process
Screenshots
single binary
Union cluster
Check all the applicable boxes
Related PRs
Docs link