Skipping of cached task outputs via execution config #482
Conversation
Codecov Report
@@ Coverage Diff @@
## master #482 +/- ##
==========================================
+ Coverage 61.63% 61.67% +0.04%
==========================================
Files 158 158
Lines 11530 11534 +4
==========================================
+ Hits 7106 7114 +8
+ Misses 3688 3683 -5
- Partials 736 737 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| // SkipCache indicates all workflows and tasks should skip all their cached results and re-compute their outputs, | ||
| // overwriting any already stored data. | ||
| // Note that setting this setting to `true` effectively disabled all caching in Flyte as all executions launched | ||
| // will have their SkipCache setting enabled. | ||
| SkipCache bool `json:"skipCache"` |
There was a problem hiding this comment.
Is there a known use-case for this? Or just adding for completeness?
There was a problem hiding this comment.
There is no use-case I could think of that would make sense, however since MergeExecutionConfig is using a WorkflowExecutionConfigInterface and the application configuration is used as a fallback for other values as well, this was required to satisfy the interface.
It currently returns the actual setting (in case someone wants to set that), however we could also just always have it return false for the ApplicationConfig and not make it configurable at all on application level.
MorpheusXAUT
force-pushed
the
cache-eviction-execution-override
branch
from
0e5d9e3
to
342b05e
Compare
hamersaw
changed the title
Signed-off-by: Nick Müller <[email protected]>
Signed-off-by: Nick Müller <[email protected]>
Signed-off-by: Nick Müller <[email protected]>
Updated to latest released versions of flyteidl, flyteplugins and flytestdlib Updated to latest unmerged version of flytepropeller Signed-off-by: Nick Müller <[email protected]>
…ersions Signed-off-by: Nick Müller <[email protected]>
MorpheusXAUT
force-pushed
the
cache-eviction-execution-override
branch
from
342b05e
to
f7d9a91
Compare
MorpheusXAUT
requested review from
katrogan,
wild-endeavor,
anandswaminathan,
EngHabu,
kumare3 and
pmahindrakar-oss
as code owners
Signed-off-by: Nick Müller <[email protected]>
| @@ -291,5 +291,10 @@ func MergeIntoExecConfig(workflowExecConfig admin.WorkflowExecutionConfig, spec | |||
| if workflowExecConfig.GetInterruptible() == nil && spec.GetInterruptible() != nil { | |||
| workflowExecConfig.Interruptible = spec.GetInterruptible() | |||
| } | |||
|
|
|||
| if !workflowExecConfig.GetOverwriteCache() && spec.GetOverwriteCache() { | |||
There was a problem hiding this comment.
Interesting - so we can essentially disable caching for an entire project+domain?
There was a problem hiding this comment.
Correct, in theory, we could disable the caching for an entire project/domain or even the whole application via the matchable resources or application config.
Whilst that's not one of the main use cases I had in mind, it's definitely an option and might be useful at some point.
There was a problem hiding this comment.
I do worry that there's no way to tell whether the value is omitted in the execution request spec, in which case it defaults to false vs. explicitly set to false. It's hard to tell by the false value alone whether the workflow exec config value should be overwritten.
Perhaps we can amend flyteidl to use the nullable google.protobuf.BoolValue like we do here: https://github.com/flyteorg/flyteidl/blob/master/protos/flyteidl/admin/launch_plan.proto#L127
There was a problem hiding this comment.
I thought about that, similar to how I previously added the interruptible flag, however I didn't see the exact use case of overwriting it to false (being able to distinguish between not set/false).
Since the main intended use case was for a single execution, I didn't anticipate we'd need the "tri-state" bool, especially since it's also less confusing in the console UI for end users.
@katrogan If you'd prefer to have the option, I can certainly adapt it since it'll be a minor change anyways.
There was a problem hiding this comment.
Yeah I suppose the project-domain and even application setting use cases are themselves probably very nice to haves that I can't imagine are used frequently. 👍 We can always revisit this if it ends up being a feature request
* Implemented SkipCache handling for execution config Signed-off-by: Nick Müller <[email protected]> * Added cache skip override to RelaunchExecution endpoint Signed-off-by: Nick Müller <[email protected]> * Updated to latest version of flytepropeller Signed-off-by: Nick Müller <[email protected]> * Renamed SkipCache flag to OverwriteCache Updated to latest released versions of flyteidl, flyteplugins and flytestdlib Updated to latest unmerged version of flytepropeller Signed-off-by: Nick Müller <[email protected]> * Updated flyteidl, flytepropeller and flytestdlib to latest released versions Signed-off-by: Nick Müller <[email protected]> * Reworded comment for clarity Signed-off-by: Nick Müller <[email protected]> Signed-off-by: Nick Müller <[email protected]>
TL;DR
flyteadmin sets the
SkipCacheflag in the execution config to instruct flytepropeller to ignore cached data for an execution.Type
Are all requirements met?
Complete description
Similar to the
Interruptibleoverride, a newSkipCacheexecution config flag was introduced. flyteadmin ensures the flag is passed onto flytepropeller accordingly forCreateExecutionandRelaunchExecutionrequests.As this uses currently unmerged versions of
flyteidl,flyteplugins,flytestdlibandflytepropeller, the PR is created as a draft (for review) until the respective new versions are available.Tracking Issue
flyteorg/flyte#2867
Follow-up issue
NA