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.

Sign up for GitHub

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

Jump to bottom

VoidPromise and Promise chaining #981

Merged
merged 8 commits into from
May 5, 2022
Merged

VoidPromise and Promise chaining #981

merged 8 commits into from
May 5, 2022

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Apr 27, 2022 •
edited
Loading

Signed-off-by: Kevin Su [email protected]

TL;DR

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

image

Tracking Issue

flyteorg/flyte#2374

Follow-up issue

NA

@pingsutw
Promise chaining
c25d8d0 
Signed-off-by: Kevin Su <[email protected]>
@codecov
Copy link

codecov bot commented Apr 27, 2022 •
edited
Loading

Codecov Report

Merging #981 (3588e08) into master (858aa65) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #981      +/-   ##
==========================================
+ Coverage   86.26%   86.33%   +0.06%     
==========================================
  Files         252      252              
  Lines       24129    24177      +48     
  Branches     2745     2750       +5     
==========================================
+ Hits        20815    20873      +58     
+ Misses       2846     2839       -7     
+ Partials      468      465       -3
Impacted Files Coverage Δ
flytekit/tools/translator.py 84.95% <ø> (ø)
flytekit/core/condition.py 90.90% <100.00%> (+1.66%) ⬆️
flytekit/core/promise.py 76.20% <100.00%> (+0.99%) ⬆️
tests/flytekit/unit/core/test_conditions.py 98.77% <100.00%> (+0.05%) ⬆️
tests/flytekit/unit/core/test_node_creation.py 95.96% <100.00%> (+0.37%) ⬆️
...s/flytekit/unit/configuration/test_image_config.py 100.00% <0.00%> (ø)
flytekit/configuration/__init__.py 94.88% <0.00%> (+1.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 858aa65...3588e08. Read the comment docs.

kumare3
kumare3 previously approved these changes Apr 27, 2022
View reviewed changes
Copy link
Contributor

@kumare3 kumare3 left a comment

Choose a reason for hiding this comment

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

Is this chaining void promise? What about promises with outputs?

@pingsutw pingsutw marked this pull request as draft April 27, 2022 19:57
@pingsutw
Void promise chaining
152b38f 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw marked this pull request as ready for review April 28, 2022 10:12
@pingsutw pingsutw changed the title Promise chaining VoidPromise and Promise chaining Apr 28, 2022
@pingsutw
Copy link
Member Author

Is this chaining void promise? What about promises with outputs?

Updated it. Now it works for both the promise and the void promise.

eapolinario
eapolinario reviewed Apr 29, 2022
View reviewed changes
flytekit/core/promise.py Outdated
Comment on lines 339 to 341
def __lshift__(self, other: typing.Union[Promise, VoidPromise]):
if not self.is_ready:
other.ref.node.runs_before(self.ref.node)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find the right shift syntax more clear, but maybe I'm missing some use case that could benefit from support for left shift as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

how about we just document in the flytesnacks PR that you can set either upstream or downstream dependencies?

Copy link
Member Author

Choose a reason for hiding this comment

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

I find Airflow support left shift, so I add that. https://airflow.apache.org/docs/apache-airflow/stable/tutorial.html#adding-dag-and-tasks-documentation.

how about we just document in the flytesnacks PR that you can set either upstream or downstream dependencies?

will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like we can deprecate create_node if we have promise chaining. https://docs.flyte.org/projects/cookbook/en/latest/auto/core/control_flow/chain_tasks.html#sphx-glr-auto-core-control-flow-chain-tasks-py
What do you think? also cc @wild-endeavor

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we should deprecate create_node right now. The API proposed in this PR is better, but there's no automated migration for people who are using create_node, so unless supporting that is going to cause trouble for us down the line we should keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we not add lshift actually?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, removed it

flytekit/core/promise.py
@@ -332,6 +332,14 @@ def __init__(self, var: str, val: Union[NodeOutput, _literal_models.Literal]):
def __hash__(self):
return hash(id(self))

def __rshift__(self, other: typing.Union[Promise, VoidPromise]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not for this PR, but we should support chaining of multiple tasks/nodes, for example, this should work:

t1 >> t2 >> t3

but right now we only have support for pair-wise constraints, in other words, we have to specify separately:

t1 >> t2
t2 >> t3

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can get pretty crazy, imagine someone doing:

t1 >> t2 << t3

😂

wild-endeavor
wild-endeavor reviewed May 4, 2022
View reviewed changes
Copy link
Contributor

@wild-endeavor wild-endeavor left a comment

Choose a reason for hiding this comment

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

Yeah could we remove the lshift actually? I feel like having only one symbolic way and one text-based way is good enough. another one can be confusing.

@pingsutw
Remove lshift
3588e08 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw merged commit 493dfc1 into master May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eapolinario eapolinario eapolinario left review comments

@wild-endeavor wild-endeavor wild-endeavor approved these changes

@kumare3 kumare3 kumare3 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pingsutw @eapolinario @wild-endeavor @kumare3