Skip to content

[1st version] Enable tuple and NamedTuple#2732

Closed
Mecoli1219 wants to merge 7 commits into
flyteorg:masterfrom
Mecoli1219:tuple
Closed

[1st version] Enable tuple and NamedTuple#2732
Mecoli1219 wants to merge 7 commits into
flyteorg:masterfrom
Mecoli1219:tuple

Conversation

@Mecoli1219

@Mecoli1219 Mecoli1219 commented Sep 5, 2024

Copy link
Copy Markdown
Contributor

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

Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
@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 <michaellai901026@gmail.com>
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
@kumare3

kumare3 commented Oct 9, 2024

Copy link
Copy Markdown
Contributor

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

Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
Comment thread flytekit/core/promise.py

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
Copy Markdown
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

return getattr(pb_module, name)


def is_namedtuple(t: typing.Type[T]) -> bool:

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