TaskTemplate offloading and available to task #166
Conversation
Signed-off-by: Ketan Umare <[email protected]>
Signed-off-by: Ketan Umare <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #166 +/- ##
==========================================
+ Coverage 60.72% 61.21% +0.48%
==========================================
Files 131 132 +1
Lines 7109 7185 +76
==========================================
+ Hits 4317 4398 +81
+ Misses 2364 2359 -5
Partials 428 428
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: Ketan Umare <[email protected]>
Signed-off-by: Ketan Umare <[email protected]>
Signed-off-by: Ketan Umare <[email protected]>
Signed-off-by: Ketan Umare <[email protected]>
kumare3
changed the title
| "github.com/pkg/errors" | ||
| ) | ||
|
|
||
| var ( |
There was a problem hiding this comment.
sad... i don't remember my golang. what does this do?
There was a problem hiding this comment.
just to make sure, we do not deviate from the core interface
| val = perRetryUniqueKey.ReplaceAllString(val, perRetryKey) | ||
|
|
||
| inputs, err := in.Get(ctx) | ||
| // For Task template, we will replace only if there is a match. This is because, task template replacement | ||
| // may be expensive, as we may offload |
There was a problem hiding this comment.
don't we always offload? r.store.WriteProtobuf? or is store sometimes local?
There was a problem hiding this comment.
no, we offload only when ttt params.Task.Path is invoked! which is only if the taskTemplateRegex match happens.
Again this is done so that its transparent to the templating method?
| @@ -50,13 +50,26 @@ func FlyteTaskToBatchInput(ctx context.Context, tCtx pluginCore.TaskExecutionCon | |||
| return nil, errors.Errorf(errors.BadTaskSpecification, "config[%v] is missing", DynamicTaskQueueKey) | |||
| } | |||
|
|
|||
| cmd, err := template.ReplaceTemplateCommandArgs(ctx, tCtx.TaskExecutionMetadata(), taskTemplate.GetContainer().GetCommand(), tCtx.InputReader(), tCtx.OutputWriter()) | |||
| inputReader := array.GetInputReader(tCtx, taskTemplate) | |||
There was a problem hiding this comment.
is the inputReader the same? previously it was tCtx.InputReader()
There was a problem hiding this comment.
Would it be possible to try to either implement or at least go through the mental exercise of writing other plugins based on this? My worry is that exposing just the task template feels like it'll be a short-sight as we try to enrich the FlyteKit-based plugins' writing experience and we will soon want to pass another Blah proto to the pod... Unfortunately, I do not know what that will contain just yet... just a gut feeling..
I would rather have had this implemented as "--task-plugin-data-path" param with a proto that -maybe for today- only has the task template... but tomorrow can add other things (maybe information about available resources or how to allocate resources... etc.)
I'm approving the PR though. Looks good for what it's trying to do.
* [wip]: TaskTemplate offloading and available to task Signed-off-by: Ketan Umare <[email protected]> * Fixed tests Signed-off-by: Ketan Umare <[email protected]> * IOUtils for working with Automated task template uploading Signed-off-by: Ketan Umare <[email protected]> * Introduced the simple reader interface Signed-off-by: Ketan Umare <[email protected]> * Helper method for TaskTemplate path Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]> Signed-off-by: Chao-Han Tsai <[email protected]>
* [wip]: TaskTemplate offloading and available to task Signed-off-by: Ketan Umare <[email protected]> * Fixed tests Signed-off-by: Ketan Umare <[email protected]> * IOUtils for working with Automated task template uploading Signed-off-by: Ketan Umare <[email protected]> * Introduced the simple reader interface Signed-off-by: Ketan Umare <[email protected]> * Helper method for TaskTemplate path Signed-off-by: Ketan Umare <[email protected]> * updated Signed-off-by: Ketan Umare <[email protected]>
Signed-off-by: Ketan Umare [email protected]
TL;DR
this PR updates the TaskReader interface to include a storage.DataReference (path) where the task template can be loaded from by the Container. It also creates helper implementation that lazily uploads the TaskTemplate to a protobuf store at the given reference when it is accessed and does it only once.
Type
Are all requirements met?
Tracking Issue
flyteorg/flyte#850