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 execution mode to ArrayNode proto #5512

Merged
merged 6 commits into from
Jul 26, 2024
Merged

Conversation

pvditt
Copy link
Contributor

@pvditt pvditt commented Jun 26, 2024

Why are the changes needed?

Support selecting toggling ArrayNode execution/storage and retain backwards compatibility.

What changes were proposed in this pull request?

Add execution_version to ArrayNode proto

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

@pvditt pvditt marked this pull request as ready for review June 26, 2024 06:04
@pvditt pvditt requested review from hamersaw and pingsutw June 26, 2024 06:05
@pvditt pvditt changed the title add execution_version to array node idl add execution_version to ArrayNode proto Jun 26, 2024
Copy link

codecov bot commented Jun 26, 2024

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 35.90%. Comparing base (337088e) to head (84c4e42).
Report is 135 commits behind head on master.

Files with missing lines Patch % Lines
flyteidl/gen/pb-go/flyteidl/core/workflow.pb.go 4.76% 20 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #5512       +/-   ##
===========================================
- Coverage   60.98%   35.90%   -25.09%     
===========================================
  Files         796     1301      +505     
  Lines       51676   109419    +57743     
===========================================
+ Hits        31515    39287     +7772     
- Misses      17261    66035    +48774     
- Partials     2900     4097     +1197     
Flag Coverage Δ
unittests-datacatalog 51.37% <ø> (-17.95%) ⬇️
unittests-flyteadmin 53.73% <ø> (-5.00%) ⬇️
unittests-flytecopilot 12.17% <ø> (-5.62%) ⬇️
unittests-flytectl 62.28% <ø> (-5.16%) ⬇️
unittests-flyteidl 7.09% <4.76%> (-71.98%) ⬇️
unittests-flyteplugins 53.31% <ø> (-8.46%) ⬇️
unittests-flytepropeller 41.75% <ø> (-15.73%) ⬇️
unittests-flytestdlib 55.27% <ø> (-10.42%) ⬇️

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.

@pvditt pvditt requested a review from hamersaw July 22, 2024 18:25
hamersaw
hamersaw previously approved these changes Jul 24, 2024

// execution_version determines the execution path for ArrayNode. The default, 0, will
// utilize the minimized storage implementation. 1 will utilize full subNode status storage.
uint32 execution_version = 5;
Copy link
Contributor

@hamersaw hamersaw Jul 24, 2024

Choose a reason for hiding this comment

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

Unsure if execution_mode sounds better? Then it should probably be an enum. Either way, whatever you decide is good.

@pvditt pvditt changed the title add execution_version to ArrayNode proto add execution mode to ArrayNode proto Jul 25, 2024
@pvditt pvditt merged commit 30d3314 into master Jul 26, 2024
48 of 50 checks passed
@pvditt pvditt deleted the exec-version-array-node branch July 26, 2024 06:43
vlibov pushed a commit to vlibov/flyte that referenced this pull request Aug 16, 2024
* add execution_version to array node idl

Signed-off-by: Paul Dittamo <[email protected]>

* make execution_version for array node spec optional

Signed-off-by: Paul Dittamo <[email protected]>

* Revert "make execution_version for array node spec optional"

This reverts commit f958241.

Signed-off-by: Paul Dittamo <[email protected]>

* update comment

Signed-off-by: Paul Dittamo <[email protected]>

* use enum instead of int for execution mode for array node

Signed-off-by: Paul Dittamo <[email protected]>

---------

Signed-off-by: Paul Dittamo <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants