Skip to content
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

Closed
wants to merge 7 commits into from

Conversation

Mecoli1219
Copy link
Contributor

@Mecoli1219 Mecoli1219 commented Sep 5, 2024

Tracking issue

Why are the changes needed?

Check out the RFC

What changes were proposed in this pull request?

  • Run local test
  • Run remote test
    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

import typing
from typing import NamedTuple

class TupleClass(NamedTuple):
    keyi: int
    keys: str

@task(container_image=image)
def nt_t1_1(a1: TupleClass, b1: str) -> typing.Tuple[TupleClass, str]:
    return TupleClass(a1[0], a1[1]), b1

@task(container_image=image)
def nt_t2_1(a2: TupleClass, b2: str) -> typing.Tuple[TupleClass, str]:
    return TupleClass(a2[0], b2), a2[1]

@workflow
def nt_wf1_1(b: str) -> typing.Tuple[TupleClass, str]:
    t, s = nt_t1_1(a1=TupleClass(12345, "NamedTupleData"), b1=b)
    return nt_t2_1(a2=TupleClass(t[0], t[1]), b2=s)

@workflow
def nt_wf2_1(b: str) -> typing.Tuple[TupleClass, str]:
    (at, bt), s = nt_t1_1(a1=TupleClass(12345, "NamedTupleData"), b1=b)
    return nt_t2_1(a2=TupleClass(at, bt), b2=s)

Tuple

@task
def t_t1(
    a1: typing.Tuple[int, str], b1: str
) -> typing.Tuple[typing.Tuple[int, str], str]:
    return (a1[0], a1[1]), b1

@task
def t_t2(
    a2: typing.Tuple[str, int], b2: str
) -> typing.Tuple[typing.Tuple[int, str], str]:
    return (a2[1], b2), a2[0]

@workflow
def t_wf1(b: str) -> typing.Tuple[typing.Tuple[int, str], str]:
    t, s = t_t1(a1=(54321, "tuple_data"), b1=b)
    return t_t2(a2=(t[1], t[0]), b2=s)

@workflow
def t_wf2(b: str) -> typing.Tuple[typing.Tuple[int, str], str]:
    (at, bt), s = t_t1(a1=(54321, "tuple_data"), b1=b)
    return t_t2(a2=(bt, at), b2=s)

Setup process

Screenshots

image

Check all the applicable boxes

  • I updated the documentation accordingly.
  • [] All new and existing tests passed.
  • All commits are signed-off.

The tests require IDL to be merged.

Related PRs

Docs link

@Mecoli1219 Mecoli1219 changed the title [WIP] Enable tuple and NamedTuple Enable tuple and NamedTuple Sep 7, 2024
@Mecoli1219 Mecoli1219 marked this pull request as ready for review September 7, 2024 08:21
@Mecoli1219 Mecoli1219 marked this pull request as draft September 7, 2024 08:21
@Mecoli1219 Mecoli1219 marked this pull request as ready for review September 7, 2024 14:40
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
@kumare3
Copy link
Contributor

kumare3 commented Oct 9, 2024

woah!!!!!!!!
@Mecoli1219 you rock!

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicated?

Suggested change
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:
Copy link
Member

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
Copy link
Member

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

Copy link
Contributor Author

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 and Tuple into two different transformers, and this will make the code more readable and maintainable. However, the main problem is that the NamedTuple 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.

Copy link
Contributor Author

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.

@Mecoli1219 Mecoli1219 changed the title Enable tuple and NamedTuple [1st version] Enable tuple and NamedTuple Oct 24, 2024
@Mecoli1219 Mecoli1219 closed this Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants