-
Notifications
You must be signed in to change notification settings - Fork 307
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
Improve Union Transformer Ambiguous Error Message #3076
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #da8f08Actionable Suggestions - 1
Review Details
|
Changelist by BitoThis pull request implements the following key changes.
|
match=("Potential types:") | ||
): |
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.
The error message assertion match=("Potential types:")
seems too generic. Consider making it more specific to match the actual error message that would be raised when there are ambiguous union types.
Code suggestion
Check the AI-generated fix before applying
match=("Potential types:") | |
): | |
match="Cannot determine which type to choose from Union[A, B] for value of type A. Potential types:" | |
): |
Code Review Run #da8f08
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3076 +/- ##
===========================================
- Coverage 93.28% 73.22% -20.07%
===========================================
Files 36 202 +166
Lines 1728 21432 +19704
Branches 0 2760 +2760
===========================================
+ Hits 1612 15693 +14081
- Misses 116 4956 +4840
- Partials 0 783 +783 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #5172b2Actionable Suggestions - 0Review Details
|
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.
For your example:
@dataclass
class A:
a: int
@dataclass
class B:
a: int
what should we recommend the user to make it work?
except Exception as e: | ||
logger.warning( | ||
f"UnionTransformer failed attempt to convert from {python_val} to {t} error: {e}", | ||
) | ||
continue | ||
|
||
if is_ambiguous: | ||
raise TypeError("Ambiguous choice of variant for union type") | ||
raise TypeError(f"Ambiguous choice of variant for union type.\n" f"Potential types: {potential_types}\n") |
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.
Can we add more to this error message stating why they are ambiguous? Something like:
These types are structurally the same, because it's attributes have the same names and associated types.
Otherwise, it's hard to tell from the error what is wrong.
Tracking issue
https://flyte-org.slack.com/archives/CP2HDHKE1/p1737474492222339
Why are the changes needed?
Users can debug more effectively if having potential types.
What changes were proposed in this pull request?
potential types
variables when doing type transformpotential types
if there's an ambiguous case.How was this patch tested?
unit test and local execution.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Summary by Bito
Enhanced error messaging system for ambiguous union type transformations, providing more detailed type information. The update includes improved error messages that now display both the ambiguous choice notification and a comprehensive list of potential matching types. These changes aim to improve developer debugging experience.Unit tests added: True
Estimated effort to review (1-5, lower is better): 1