Add config override idl #412
Conversation
Signed-off-by: byhsu <[email protected]>
Signed-off-by: byhsu <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #412 +/- ##
==========================================
+ Coverage 75.90% 78.48% +2.57%
==========================================
Files 18 18
Lines 1457 1250 -207
==========================================
- Hits 1106 981 -125
+ Misses 294 212 -82
Partials 57 57
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: byhsu <[email protected]>
| @@ -249,4 +253,37 @@ message WorkflowTemplate { | |||
| message TaskNodeOverrides { | |||
| // A customizable interface to convey resources requested for a task container. | |||
| Resources resources = 1; | |||
|
|
|||
| // Boolean that indicates if caching should be enabled | |||
| oneof cache_value { | |||
There was a problem hiding this comment.
For all of these oneofs we can also use google.protobuf.BoolValue, google.protobuf.Int32Value, etc which differentiate between being set and unset in addition to the values.
There was a problem hiding this comment.
Whats the concern of oneof?
There was a problem hiding this comment.
It's easier to explicitly mark and check for whether a field was intended to be set.
Oneof implies that there may be other flavors/varieties when that's not really the intention, right?
There was a problem hiding this comment.
Since the fields you added to the TaskNodeOverrides are also used in matchable attributes WorkflowExecutionConfig, ExecutionSpec, LaunchPlanSpec and more, could we take this PR as an opportunity to encapsulate all these fields in a well-defined proto message (which we can then embed in TaskNodeOverrides) and in the future hopefully refactor all the existing specs to share as part of a separate GH issue? Currently adding a new execution-type field and understanding the pattern of overrides is onerous so it would be super helpful to establish a singular message/source of truth in this PR we could later migrate to elsewhere
| @@ -249,4 +253,37 @@ message WorkflowTemplate { | |||
| message TaskNodeOverrides { | |||
| // A customizable interface to convey resources requested for a task container. | |||
| Resources resources = 1; | |||
|
|
|||
| // Boolean that indicates if caching should be enabled | |||
| oneof cache_value { | |||
There was a problem hiding this comment.
It's easier to explicitly mark and check for whether a field was intended to be set.
Oneof implies that there may be other flavors/varieties when that's not really the intention, right?
TL;DR
Added idl as discussed in flyteorg/flyte#3553
Type
Are all requirements met?
Tracking Issue
flyteorg/flyte#475