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

Add escape for scalars to binding during union handling #2460

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented Jun 4, 2024

Tracking issue

Related to flyteorg/flyte#5321. This was discovered while implementing that PR.

Why are the changes needed?

The main issue is highlighted in the test added to the test type engine file. When you call TypeEngine.to_literal on a Union type, you should get back a Flyte literal with a Union message. This was happening correctly in the Type Engine but for some reason this wasn't happening for Binding Data.

That is, if you had

@task
def say_hello(a: int | str) -> str:
    print(f"Type is {type(a)}")
    if type(a) == str:
        return f"Hello, {a}!"
    return "Hello, World!"

@workflow
def hello_world_wf()-> str:
    res = say_hello(a="world")
    return res

the node for the say_hello invocation was being bound to a BindingData Literal Scalar that was just the raw string "world", not a Union that contained that string literal.

What changes were proposed in this pull request?

This adds an escape mechanism and some comments in the binding data from python std function.

How was this patch tested?

Before: https://www.test.cluster/console/projects/flytesnacks/domains/development/executions/f6ba77e8684e94e1092a
After: https://www.test.cluster/console/projects/flytesnacks/domains/development/executions/f06eaea2298ad4884952
Both runs succeed.

Before: https://www.test.cluster/api/v1/workflows/flytesnacks/development/unions.hello_world_wf/4jItAJN7eckVByTXKxjsPg
After: https://www.test.cluster/api/v1/workflows/flytesnacks/development/unions.hello_world_wf/gmbOol0QMeiiHF_y99A97g
Note the after definition binds the node to a union literal, not a plain scalar.

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

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
@wild-endeavor
Copy link
Contributor Author

cc @MortalHappiness can you take a look at this? i think with this we can revert some of the changes in your pr...

@wild-endeavor wild-endeavor marked this pull request as ready for review June 4, 2024 20:36
@MortalHappiness
Copy link
Contributor

LGTM

@wild-endeavor wild-endeavor merged commit 72af7c6 into master Jun 5, 2024
44 of 45 checks passed
@wild-endeavor wild-endeavor changed the title add escape hatch to binding union handling Add escape for scalars to binding during union handling Jun 5, 2024
MortalHappiness added a commit to MortalHappiness/flytekit that referenced this pull request Jun 6, 2024
Resolves: flyteorg/flyte#5321
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: Jan Fiedler <jan@union.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants