-
Notifications
You must be signed in to change notification settings - Fork 310
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
[1st version] Enable tuple and NamedTuple #2732
Conversation
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
woah!!!!!!!! |
Signed-off-by: Mecoli1219 <[email protected]>
@@ -103,6 +108,41 @@ def my_wf(in1: int, in2: int) -> int: | |||
return result | |||
|
|||
|
|||
def output_to_tuple(output: "Output") -> Tuple: # type: ignore | |||
""" | |||
This function is used to convert an output object to a tuple. This is used to convert the output object to a tuple |
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.
duplicated?
This function is used to convert an output object to a tuple. This is used to convert the output object to a tuple | |
This is used to convert the output object to a tuple |
@@ -24,3 +24,14 @@ def load_type_from_tag(tag: str) -> typing.Type[T]: | |||
raise ValueError(f"Could not find the protobuf named: {name} @ {module}.") | |||
|
|||
return getattr(pb_module, name) | |||
|
|||
|
|||
def is_namedtuple(t: typing.Type[T]) -> bool: |
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.
It would be great to have a unit test for it.
expected: LiteralType, | ||
) -> Literal: | ||
if expected.tuple_type.tuple_name != TupleTransformer.default_tuple_name(): | ||
# We are expected a NamedTuple |
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.
Do you think we need a separate transformer for NamedTuple? NamedTupleTransformer
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.
I have discussed that in Section 6 Alternatives of this RFC: flyteorg/flyte#5699
In Flytekit, we can separate the
NamedTuple
andTuple
into two different transformers, and this will make the code more readable and maintainable. However, the main problem is that theNamedTuple
is not a type in Python, it is a function that returns a type. Separating them will make the registries of the transformers more complex and harder to maintain.
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.
Also, I have tried it before (maybe you can see that in some commits in this Pull Request), and it is really hard to maintain that.
Tracking issue
Why are the changes needed?
Check out the RFC
What changes were proposed in this pull request?
Currently, the flyte Console doesn't support tuple type on UI, we will open another PR to address that.
How was this patch tested?
NamedTuple
Tuple
Setup process
Screenshots
Check all the applicable boxes
The tests require IDL to be merged.
Related PRs
Docs link