[RFC] Tuple IDL

[Mecoli1219]

Tracking issue

#5908
#1337
#3158
#4358

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

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

Related PRs

Docs link

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 36.31%. Comparing base (881d7a2) to head (e692c7c).
⚠️ Report is 385 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5699   +/-   ##
=======================================
  Coverage   36.31%   36.31%           
=======================================
  Files        1304     1304           
  Lines      110072   110072           
=======================================
  Hits        39974    39974           
  Misses      65936    65936           
  Partials     4162     4162           

Flag	Coverage Δ
unittests-datacatalog	51.37% <ø> (ø)
unittests-flyteadmin	55.60% <ø> (ø)
unittests-flytecopilot	12.17% <ø> (ø)
unittests-flytectl	62.21% <ø> (ø)
unittests-flyteidl	7.12% <ø> (ø)
unittests-flyteplugins	53.35% <ø> (ø)
unittests-flytepropeller	41.91% <ø> (ø)
unittests-flytestdlib	55.37% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[pingsutw]

We need to discuss whether to check the name of each field and the tuple itself

yup. I think we need it

[pingsutw]

cc @fg91 I know you want this too

[fg91]

cc @fg91 I know you want this too

Yes, I started an incubator discussion about this a while back: #4544. I closed this incubator discussion now in favor of this RFC here (and this one).

I 100% support that tuples finally get support in flytekit, python users expect this and have very little understanding for it not being supported 🫠

In the now closed incubator discussion @EngHabu had some thoughts about tuple support in the discussion that probably are of relevance to this discussion here.

After having read the RFC, I have to admit that I don't have enough knowledge of flyteidl to judge how exactly the protobuf messages for tuple types should look like though so TL;DR I do very much support the idea but have to defer to somebody else to judge the exact implementation in flyteidl.

[Mecoli1219]

@task
def my_task() -> Tuple[Tuple[int, str], str]:
return (1, "foo"), "bar"
# protobuf: {
# o1: {
# tuple_name: ""
# fields: [
# {name: "", value: 1},
# {name: "", value: "foo"}
# ]
# }
# o2: "bar"
# }

[cosmicBboy]

One comment here re: FlyteRemote API. When accessing outputs in the {Workflow, Task}Execution objects, one thought is to introduce another attribute execution.output (as opposed to the current outputs attribute) since tasks will only ever output single output (since multiple outputs should be wrapped in Tuple[...])

[Mecoli1219]

I thought Flyte implements multiple outputs for task/workflow (aka tuple-wrapped output) with LiteralMap, which is quite different from the new IDL proposed by this RFC. In my implementation #5720, I used other types that are more similar to the original implementation, including a map[string]Literal type and also two more extra fields (string tuple_name and []string order) to enable the reconstruction of the Python tuple and NamedTuple types.

---

@fg91 After reading the original incubator discussion, there are some points I would like to address.

1. I don't think def my_task() -> (int, str) is the legal output annotation in Python, but Flyte supports it somehow.
2. As mentioned in official doc, typing.Tuple is a deprecated alias for tuple after Python version 3.9, so I think we can view them as the same type in flytekit. In my current implementation, I will treat both of them the same.

---

one thought is to introduce another attribute execution.output (as opposed to the current outputs attribute) since tasks will only ever output a single output (since multiple outputs should be wrapped in Tuple[...])

I believe that this can be enabled in the future. I have tried to make my implementation close to the multiple outputs structure.
However, I am not sure whether we should add a new attribute output, or replace outputs with output. Also, I am not really sure how this will affect the flyte system, could anyone provide more comments?

I'll create a Pull Request for Flytekit as soon as possible.

[Future-Outlier]

Why int and str tuple key attribute access?
NamedTuple: order(int) and key(str) both supported
Tuple: order(int) only

[davidmirror-ops]

10/24/2024 Contributor's sync notes: looking to have a less controversial approach by not creating a new Literal for Tuple. Michael running experiments

[github-actions]

"Hello 👋, this PR has been inactive for over 9 months. To help maintain a clean and focused backlog, we'll be marking this PR as stale and will close the PR if we detect no activity in the next 14 days. Thank you for your contribution and understanding! 🙏"