Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Add interruptible override to execution #minor #287

Merged
merged 7 commits into from
May 2, 2022

Conversation

MorpheusXAUT
Copy link
Contributor

TL;DR

Adds an interruptible flag to the ExecutionSpec, allowing for workflows to be flagged as interruptible for a single execution.

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

An additional field has been added to the ExecutionSpec, allowing for the interruptible flag of a workflow to be override for a single execution (existing overrides on task level still remain in place). Default value of false falls back to workflow/task config.
The override is internally passed along via the WorkflowExecutionSpec in flyteadmin, some interfaces require the LaunchPlanSpec to implement the interruptible flag as well, although it is currently not used.

Tracking Issue

flyteorg/flyte#2284

Follow-up issue

NA

@welcome
Copy link

welcome bot commented Apr 21, 2022

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).

Allows distinguishment between a value not being provided and the go zerovalue false

Signed-off-by: Nick Müller <[email protected]>
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

looks good, just one nit with comments!

@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #287 (de51e71) into master (237f025) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #287   +/-   ##
=======================================
  Coverage   74.77%   74.77%           
=======================================
  Files          15       15           
  Lines         991      991           
=======================================
  Hits          741      741           
  Misses        218      218           
  Partials       32       32           
Flag Coverage Δ
unittests 75.02% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 237f025...de51e71. Read the comment docs.

@MorpheusXAUT
Copy link
Contributor Author

Not quite sure why the generate test fails, DELTA_CHECK=true make generate shows everything being fine locally for me 🤔
@katrogan any ideas? looks like some ordering of the generated documentation changed locally on my machine... Re-running make generate doesn't change anything for me though.

Included documentation for  in proto generation

Signed-off-by: Nick Müller <[email protected]>
@EngHabu
Copy link
Contributor

EngHabu commented Apr 27, 2022

This could be due to difference in tooling versions... Make sure you're using the same versions used in CI... Apologies about that

Signed-off-by: Nick Müller <[email protected]>
@MorpheusXAUT MorpheusXAUT force-pushed the execution-interruptible branch from 973cdbc to de51e71 Compare April 27, 2022 21:23
@MorpheusXAUT
Copy link
Contributor Author

@EngHabu Just re-generated the same on my Windows instead of Linux machine, seems to be working now... Not sure what exactly changed, as I believe I've installed the identical versions in both environments, but I'll have another look at that tomorrow.
Should go good now though 🙂

@katrogan
Copy link
Contributor

@EngHabu mind reviewing too?

@MorpheusXAUT
Copy link
Contributor Author

@katrogan @hamersaw Any way to get this merged so I can update the other outstanding PRs with the updated version? 🙂

@hamersaw hamersaw merged commit e749b82 into flyteorg:master May 2, 2022
@MorpheusXAUT MorpheusXAUT mentioned this pull request May 3, 2022
8 tasks
eapolinario pushed a commit that referenced this pull request Sep 8, 2023
* Added interruptible flag for execution to protos

Signed-off-by: Nick Müller <[email protected]>

* Changed execution interruptible flag to use regular bool

Signed-off-by: Nick Müller <[email protected]>

* Changed interruptible overrides to use BoolValue
Allows distinguishment between a value not being provided and the go zerovalue false

Signed-off-by: Nick Müller <[email protected]>

* Interruptible flag comment/documentation

Signed-off-by: Nick Müller <[email protected]>

* Interruptible flag comment/documentation

Signed-off-by: Nick Müller <[email protected]>

* Removed unescaped quotes from proto comments
Included documentation for  in proto generation

Signed-off-by: Nick Müller <[email protected]>

* Re-generated documentation

Signed-off-by: Nick Müller <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants