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

node_dependency_hints for dynamic tasks #2015

Merged
merged 20 commits into from
Jan 31, 2024
Merged

node_dependency_hints for dynamic tasks #2015

merged 20 commits into from
Jan 31, 2024

Conversation

Tom-Newton
Copy link
Contributor

@Tom-Newton Tom-Newton commented Dec 1, 2023 •
edited
Loading

Tracking issue

Closes flyteorg/flyte#4516

Why are the changes needed?

Streamline launching launchplans from dynamic tasks.

What changes were proposed in this pull request?

How was this patch tested?

Updated unit tests. I have also been using very similar changes in a real flyte deployment for weeks.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly. - I've updated docstrings. Please let me know if there is more documentation I should update.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

@Tom-Newton Tom-Newton mentioned this pull request Dec 1, 2023
Closed
2 tasks
@Tom-Newton Tom-Newton force-pushed the tomnewton/upsream_output_entity_hints branch from 5fe0ef7 to 45b9b19 Compare December 1, 2023 20:21
@Tom-Newton
Copy link
Contributor Author

Looks like I created a circular imports with the type hints when rebasing

@Tom-Newton Tom-Newton force-pushed the tomnewton/upsream_output_entity_hints branch from 6920d5b to 25fa4dd Compare December 13, 2023 18:53
@Tom-Newton
Copy link
Contributor Author

Tom-Newton commented Dec 13, 2023 •
edited
Loading

@pingsutw do you think we should go ahead with this change? The issue is still tagged as needs-decision.

If yes, I'll write a unit test for this and request review.

@codecov Codecov
Copy link

codecov bot commented Dec 13, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8861d0a) 85.80% compared to head (5901cff) 85.80%.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #2015   +/-   ##
=======================================
  Coverage   85.80%   85.80%           
=======================================
  Files         313      313           
  Lines       23579    23594   +15     
  Branches     3530     3534    +4     
=======================================
+ Hits        20232    20246   +14     
  Misses       2738     2738           
- Partials      609      610    +1

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

@Tom-Newton Tom-Newton force-pushed the tomnewton/upsream_output_entity_hints branch 3 times, most recently from 23aaf8f to b629c6f Compare December 14, 2023 14:11
@Tom-Newton Tom-Newton changed the title WIP Output entity hints for dynamic tasks Output entity hints for dynamic tasks Dec 14, 2023
@Tom-Newton Tom-Newton force-pushed the tomnewton/upsream_output_entity_hints branch 3 times, most recently from 7d3e25a to 5fc0f47 Compare December 15, 2023 14:15
@Tom-Newton Tom-Newton changed the title Output entity hints for dynamic tasks node_dependency_hints for dynamic tasks Dec 15, 2023
@Tom-Newton Tom-Newton marked this pull request as ready for review December 15, 2023 14:16
@Tom-Newton Tom-Newton force-pushed the tomnewton/upsream_output_entity_hints branch 2 times, most recently from 8f8e46d to 920a5d2 Compare December 19, 2023 20:52
@Tom-Newton
Fix tests
e18510c 
Signed-off-by: Thomas Newton <[email protected]>
@Tom-Newton
Auto-format
4672cfc 
Signed-off-by: Thomas Newton <[email protected]>
@Tom-Newton
Add positive test case
2be6e69 
Signed-off-by: Thomas Newton <[email protected]>
@Tom-Newton
Add a test for disallowing entity hints on static tasks
7f7ab2a 
Signed-off-by: Thomas Newton <[email protected]>
@Tom-Newton
raise instead of assert
89ef3e9 
Signed-off-by: Thomas Newton <[email protected]>
@Tom-Newton
Rename output_entity_hints -> node_dependency_hints
a1d1016 
Signed-off-by: Thomas Newton <[email protected]>
@Tom-Newton
Update docstrings
fb97a71 
Signed-off-by: Thomas Newton <[email protected]>
@Tom-Newton
Auto-format
6cf0205 
Signed-off-by: Thomas Newton <[email protected]>
@Tom-Newton
Fix test_serialize_vscode
cf1cba5 
Signed-off-by: Thomas Newton <[email protected]>
@Tom-Newton
Maybe fix the docs error
8d1e734 
Signed-off-by: Thomas Newton <[email protected]>
@Tom-Newton Tom-Newton force-pushed the tomnewton/upsream_output_entity_hints branch from 920a5d2 to a29cd98 Compare December 20, 2023 00:15
@Tom-Newton Tom-Newton force-pushed the tomnewton/upsream_output_entity_hints branch from a29cd98 to 62c5209 Compare December 20, 2023 00:15
@Tom-Newton
Fix monodocs build
6ae0e89 
Signed-off-by: Thomas Newton <[email protected]>
@Tom-Newton Tom-Newton requested a review from pingsutw December 21, 2023 10:27
@eapolinario eapolinario merged commit 35f8a8c into flyteorg:master Jan 31, 2024
84 checks passed
@Tom-Newton Tom-Newton mentioned this pull request Dec 9, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pingsutw pingsutw pingsutw approved these changes

@eapolinario eapolinario eapolinario approved these changes

@wild-endeavor wild-endeavor Awaiting requested review from wild-endeavor wild-endeavor is a code owner

@kumare3 kumare3 Awaiting requested review from kumare3 kumare3 is a code owner

@cosmicBboy cosmicBboy Awaiting requested review from cosmicBboy cosmicBboy is a code owner

Assignees

@Tom-Newton Tom-Newton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Core feature] [flytekit] Add "entity hints" to dynamic tasks
3 participants
@Tom-Newton @eapolinario @pingsutw