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 various executions phases which are available in flyteidl, but not in flytekit #2022

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

mdjong1
Copy link
Member

@mdjong1 mdjong1 commented Dec 5, 2023

Tracking issue

None afaik

Why are the changes needed?

So that we can access all possible phases returned by Flyte within flytekit.

Also suggesting a refactoring to enum_to_string() in the second commit. Happy to drop that if it's deemed unnecessary.

How was this patch tested?

I ran make test locally and it ran through. I didn't add any more tests because as much as I could find everything would fall apart already if I messed up enum_to_string(), so expecting that that is still functional after this PR :)

Check all the applicable boxes

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

Maarten de Jong added 2 commits December 5, 2023 15:40
…t not in flytekit

Signed-off-by: Maarten de Jong <mdejong@blackshark.ai>
Signed-off-by: Maarten de Jong <mdejong@blackshark.ai>
Copy link

welcome bot commented Dec 5, 2023

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@mdjong1 mdjong1 changed the title Add various executions phases which are available in the protobuf, but not in flytekit Add various executions phases which are available in flyteidl, but not in flytekit Dec 5, 2023
Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (5c6802c) 86.22% compared to head (c3ed4e6) 63.19%.

Files Patch % Lines
flytekit/models/core/execution.py 29.41% 12 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2022       +/-   ##
===========================================
- Coverage   86.22%   63.19%   -23.03%     
===========================================
  Files         320      320               
  Lines       23531    23494       -37     
  Branches     3464     3446       -18     
===========================================
- Hits        20289    14847     -5442     
- Misses       2650     8233     +5583     
+ Partials      592      414      -178     

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

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

LGTM, thank you. the code becomes much simpler

@pingsutw pingsutw merged commit 88818fd into flyteorg:master Dec 13, 2023
74 of 76 checks passed
Copy link

welcome bot commented Dec 13, 2023

Congrats on merging your first pull request! 🎉

@mdjong1 mdjong1 deleted the add-missing-execution-phases branch December 13, 2023 19:09
RRap0so pushed a commit to RRap0so/flytekit that referenced this pull request Dec 15, 2023
…t in flytekit (flyteorg#2022)

* Add various executions phases which are available in the protobuf, but not in flytekit

Signed-off-by: Maarten de Jong <mdejong@blackshark.ai>

* Simplify `enum_to_string` function

Signed-off-by: Maarten de Jong <mdejong@blackshark.ai>

---------

Signed-off-by: Maarten de Jong <mdejong@blackshark.ai>
Signed-off-by: Rafael Raposo <rafaelraposo@spotify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants