-
Notifications
You must be signed in to change notification settings - Fork 288
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
Type Mismatching while Serializing Dataclass with Union #2859
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: mao3267 <[email protected]>
Signed-off-by: mao3267 <[email protected]>
@@ -967,6 +967,7 @@ class TestFileStruct(DataClassJsonMixin): | |||
b: typing.Optional[FlyteFile] | |||
b_prime: typing.Optional[FlyteFile] | |||
c: typing.Union[FlyteFile, None] | |||
c_prime: typing.Union[None, FlyteFile] |
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 you make this
c_prime: typing.Union[None, FlyteFile] | |
c_prime: typing.Union[None, StructuredDataset, int, FlyteFile] |
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.
actually can you write one more unit test for me please? (and add it under test_dataclass.py
this file is getting too big).
@dataclass
class A():
x: int
@dataclass
class B():
x: FlyteFile
then call _make_dataclass_serializable
on Union[None, A, B]
where b = B(x="s3://tmp)
or something.
Signed-off-by: mao3267 <[email protected]>
Signed-off-by: mao3267 <[email protected]>
Signed-off-by: mao3267 <[email protected]>
Signed-off-by: mao3267 <[email protected]>
Signed-off-by: mao3267 <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2859 +/- ##
===========================================
+ Coverage 45.53% 79.15% +33.61%
===========================================
Files 196 196
Lines 20418 20545 +127
Branches 2647 2647
===========================================
+ Hits 9298 16262 +6964
+ Misses 10658 3546 -7112
- Partials 462 737 +275 ☔ View full report in Codecov by Sentry. |
flytekit/core/type_engine.py
Outdated
|
||
def get_expected_type(python_val: T, types: tuple) -> Type[T | None]: | ||
if len(set(types) & {FlyteFile, FlyteDirectory, StructuredDataset}) > 1: | ||
raise ValueError("Cannot have two Flyte types in a Union type") |
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 make the error
Cannot have more than one Flyte type in the Union when attempting to use the string shortcut. Please specify the full object (e.g. FlyteFile(...)) instead of just passing a string.
instead?
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.
thank you! left one more comment to update the error message. otherwise good. thank you.
Signed-off-by: mao3267 <[email protected]>
@@ -967,6 +967,7 @@ class TestFileStruct(DataClassJsonMixin): | |||
b: typing.Optional[FlyteFile] | |||
b_prime: typing.Optional[FlyteFile] | |||
c: typing.Union[FlyteFile, None] | |||
c_prime: typing.Union[None, FlyteFile] |
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.
Although we didn’t expect typing.Union[None, StructuredDataset, int, FlyteFile]
to work in our tests due to multiple FlyteTypes ambiguity, we anticipated something like typing.Union[None, int, FlyteFile]
might function correctly. However, after tracing the code, I discovered that there are some bugs while msgpack handling unions with more than two types. cc @Future-Outlier
Tracking issue
Closes flyteorg/flyte#5910
Why are the changes needed?
While converting dataclasses from Python types to literal types through the
to_literal
function, the dataclass will pass through the_make_dataclass_serializable
function to ensure it can be serialized later. For dataclasses withUnion
properties, under theif UnionTransformer.is_optional_type(python_type):
statement, it usesget_args(python_type)[0]
as the type of the property without verifying its compatibility. This will cause an error if the first argument is not the expected type.What changes were proposed in this pull request?
get_args(python_type)
How was this patch tested?
flytefile_serialize.py
serialize_union.py
test_type_engine.py
Setup process
Screenshots
Union[None, int, str]
Union[None, int, str]
Check all the applicable boxes
Related PRs
None
Docs link
None