Pod tasks can be used for map tasks #182

katrogan · 2021-06-07T21:39:27Z

TL;DR

Pod tasks can now be mapped over too. As a long term goal we should introduce Array Nodes as proposed here but this PR is an interim solution given how much refactoring introducing the new node type entails.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

flyteorg/flyte#1051

Follow-up issue

NA

Signed-off-by: Katrina Rogan <[email protected]>

codecov · 2021-06-07T21:44:17Z

Codecov Report

Merging #182 (1259fd0) into master (fea5d84) will increase coverage by 0.08%.
The diff coverage is 74.19%.

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
+ Coverage   60.36%   60.45%   +0.08%     
==========================================
  Files         135      135              
  Lines        7292     7348      +56     
==========================================
+ Hits         4402     4442      +40     
- Misses       2447     2457      +10     
- Partials      443      449       +6

Flag	Coverage Δ
unittests	`60.45% <74.19%> (+0.08%)`	⬆️

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

Impacted Files	Coverage Δ
go/tasks/plugins/array/core/state.go	`48.52% <0.00%> (ø)`
go/tasks/plugins/array/k8s/monitor.go	`61.80% <16.66%> (-1.97%)`	⬇️
go/tasks/plugins/array/k8s/transformer.go	`77.08% <75.40%> (+2.66%)`	⬆️
go/tasks/plugins/array/k8s/task.go	`52.17% <78.57%> (-1.02%)`	⬇️
go/tasks/pluginmachinery/flytek8s/pod_helper.go	`67.66% <100.00%> (+2.62%)`	⬆️
go/tasks/plugins/k8s/sidecar/sidecar.go	`78.57% <100.00%> (-1.31%)`	⬇️

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 fea5d84...1259fd0. Read the comment docs.

Signed-off-by: Katrina Rogan <[email protected]>

go/tasks/plugins/array/core/state.go

go/tasks/plugins/array/k8s/task.go

EngHabu · 2021-06-10T00:08:02Z

go/tasks/plugins/array/k8s/transformer.go

@@ -49,6 +50,48 @@ func GetNamespaceForExecution(tCtx core.TaskExecutionContext, namespaceTemplate
 	return namespace
 }

+// Initializes a pod from an array job task template with a K8sPod set as the task target.
+// TODO: This should be removed (it duplicates the pod plugin logic) once we improve array job handling


nit: add a date here... let's see when will we actually get to this... any bets?

done, let me know if you have other suggestions :)

go/tasks/plugins/array/k8s/transformer.go

Signed-off-by: Katrina Rogan <[email protected]>

EngHabu · 2021-06-10T00:34:35Z

go/tasks/plugins/array/k8s/task.go

 	taskTemplate, err := tCtx.TaskReader().Read(ctx)
 	if err != nil {
 		return LaunchError, errors2.Wrapf(ErrGetTaskTypeVersion, err, "Unable to read task template")
 	} else if taskTemplate == nil {
 		return LaunchError, errors2.Wrapf(ErrGetTaskTypeVersion, err, "Missing task template")
 	}
 	inputReader := array.GetInputReader(tCtx, taskTemplate)
-	pod.Spec.Containers[0].Args, err = template.Render(ctx, args,
+	pod.Spec.Containers[containerIndex].Args, err = template.Render(ctx, args,


I guess this should apply to cmd as well?

we already combine the command and args: https://github.com/flyteorg/flyteplugins/pull/182/files#diff-01ee392f44bbd91e1e5f4afd999327e02f8f1df54fd5b5b576478e903cbcb3c2R92

(for the record, primary container always has an sdk supplied command + args)

oh... is that even right? combing args... that can alter docker run behavior if a custom container is used...

I generally dislike that we have two ways of building the pod that seem to diverge and do things at different places... Any refactoring in mind to make that more readable?

for the primary container, I think flytekit only puts its command into args... and leaves the command empty to run whatever the default entrypoint in the container is... (unless the user explicitly modifies it)
right? CC @wild-endeavor

updated

do we support custom containers for map tasks? i thought the underlying task had to be a PythonAutoContainer task

I don't think, generally speaking, we should make assumptions based on how we implemented Flytekit... the underlying spec allows cmd & args and I don't think this plugin should change that behavior... What do yo think?

Also, I thought we are getting rid of the args/cmd manipulation altogether from this function, right?

oh derp, yes. thanks & done

go/tasks/plugins/array/k8s/transformer.go

Signed-off-by: Katrina Rogan <[email protected]>

katrogan · 2021-06-11T17:32:17Z

PTAL @EngHabu

EngHabu · 2021-06-11T21:14:49Z

go/tasks/plugins/array/k8s/task.go

 	taskTemplate, err := tCtx.TaskReader().Read(ctx)
 	if err != nil {
 		return LaunchError, errors2.Wrapf(ErrGetTaskTypeVersion, err, "Unable to read task template")
 	} else if taskTemplate == nil {
 		return LaunchError, errors2.Wrapf(ErrGetTaskTypeVersion, err, "Missing task template")
 	}
 	inputReader := array.GetInputReader(tCtx, taskTemplate)
-	pod.Spec.Containers[0].Args, err = template.Render(ctx, args,
+	pod.Spec.Containers[containerIndex].Args, err = template.Render(ctx, args,


I don't think, generally speaking, we should make assumptions based on how we implemented Flytekit... the underlying spec allows cmd & args and I don't think this plugin should change that behavior... What do yo think?

go/tasks/plugins/array/k8s/transformer.go

EngHabu · 2021-06-11T21:16:37Z

go/tasks/plugins/array/k8s/task.go

 	taskTemplate, err := tCtx.TaskReader().Read(ctx)
 	if err != nil {
 		return LaunchError, errors2.Wrapf(ErrGetTaskTypeVersion, err, "Unable to read task template")
 	} else if taskTemplate == nil {
 		return LaunchError, errors2.Wrapf(ErrGetTaskTypeVersion, err, "Missing task template")
 	}
 	inputReader := array.GetInputReader(tCtx, taskTemplate)
-	pod.Spec.Containers[0].Args, err = template.Render(ctx, args,
+	pod.Spec.Containers[containerIndex].Args, err = template.Render(ctx, args,


Also, I thought we are getting rid of the args/cmd manipulation altogether from this function, right?

Signed-off-by: Katrina Rogan <[email protected]>

katrogan added 2 commits June 7, 2021 14:33

wip

e36af16

Signed-off-by: Katrina Rogan <[email protected]>

merge conflicts

07f7f8b

Signed-off-by: Katrina Rogan <[email protected]>

EngHabu marked this pull request as draft June 7, 2021 21:39

katrogan added 5 commits June 8, 2021 13:09

logs

107df46

Signed-off-by: Katrina Rogan <[email protected]>

logs

b73ba30

Signed-off-by: Katrina Rogan <[email protected]>

again

c898d9d

Signed-off-by: Katrina Rogan <[email protected]>

some tests, clean-up debug logs

eb86cc9

Signed-off-by: Katrina Rogan <[email protected]>

more tests

6eca121

Signed-off-by: Katrina Rogan <[email protected]>

katrogan changed the title ~~wip - Pod tasks can be mapped over too~~ Pod tasks can be used for map tasks Jun 9, 2021

katrogan marked this pull request as ready for review June 9, 2021 23:23

katrogan requested review from EngHabu and kumare3 June 9, 2021 23:25

EngHabu reviewed Jun 10, 2021

View reviewed changes

pessimism

82ef021

Signed-off-by: Katrina Rogan <[email protected]>

EngHabu reviewed Jun 10, 2021

View reviewed changes

feedback

5455858

Signed-off-by: Katrina Rogan <[email protected]>

EngHabu reviewed Jun 11, 2021

View reviewed changes

katrogan added 2 commits June 11, 2021 14:32

review comments

0d4edbb

Signed-off-by: Katrina Rogan <[email protected]>

issue

1259fd0

Signed-off-by: Katrina Rogan <[email protected]>

EngHabu approved these changes Jun 11, 2021

View reviewed changes

katrogan merged commit 6e4c9c2 into master Jun 11, 2021

eapolinario pushed a commit that referenced this pull request Sep 6, 2023

Pod tasks can be used for map tasks (#182)

7c143b2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pod tasks can be used for map tasks #182

Pod tasks can be used for map tasks #182

katrogan commented Jun 7, 2021 •

edited

Loading

codecov bot commented Jun 7, 2021 •

edited

Loading

EngHabu Jun 10, 2021

katrogan Jun 10, 2021

EngHabu Jun 10, 2021

katrogan Jun 10, 2021

EngHabu Jun 10, 2021

katrogan Jun 10, 2021 •

edited

Loading

EngHabu Jun 11, 2021

EngHabu Jun 11, 2021

katrogan Jun 11, 2021

katrogan commented Jun 11, 2021

EngHabu Jun 11, 2021

EngHabu Jun 11, 2021

Pod tasks can be used for map tasks #182

Pod tasks can be used for map tasks #182

Conversation

katrogan commented Jun 7, 2021 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov bot commented Jun 7, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katrogan Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katrogan commented Jun 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katrogan commented Jun 7, 2021 •

edited

Loading

codecov bot commented Jun 7, 2021 •

edited

Loading

katrogan Jun 10, 2021 •

edited

Loading