Fix: Handle overriding of container image in backend #4858

fg91 · 2024-02-07T22:03:06Z

Tracking issue

Why are the changes needed?

The container_image task node override, among others, is currently broken (doesn't work with pyflyte register and only once with pyflyte run, see tracking issue.

What changes were proposed in this pull request?

Add container_image to TaskNodeOverride proto, set override in v1alpha1.NodeSpec in flytepropeller and actually apply the image override in the pod spec.

How was this patch tested?

Ran flyteadmin and flytepropeller on my dev machine. Was able to override container image in flytekit and confirm overridden image in the pod in the cluster.

I tested this for normal python function tasks, kubeflow pytorch elastic tasks, map tasks, and array node map tasks.
Added unit tests.

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

flyteorg/flytekit#2176

Docs link

codecov · 2024-02-09T19:16:22Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (1ae5a8f) 58.92% compared to head (53f1f4d) 58.94%.

Files	Patch %	Lines
...ks/pluginmachinery/flytek8s/plugin_exec_context.go	0.00%	2 Missing ⚠️
...propeller/pkg/apis/flyteworkflow/v1alpha1/nodes.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4858      +/-   ##
==========================================
+ Coverage   58.92%   58.94%   +0.01%     
==========================================
  Files         645      645              
  Lines       55380    55398      +18     
==========================================
+ Hits        32635    32652      +17     
- Misses      20161    20163       +2     
+ Partials     2584     2583       -1

Flag	Coverage Δ
unittests	`58.94% <77.77%> (+0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fg91 · 2024-02-09T21:51:10Z

@hamersaw @jeevb I tagged you because I saw that both of you recently touched overriding of either extended resources or caching. Do you think I should rather tag somebody else though?

flyteidl/protos/flyteidl/core/workflow.proto

Signed-off-by: Fabio Graetz <[email protected]>

pingsutw

LGTM, thank you!

* Add container image to TaskNodeOverrides proto Signed-off-by: Fabio Graetz <[email protected]> * Add container image override to v1alpha1.NodeSpec Signed-off-by: Fabio Graetz <[email protected]> * Override container image in pod Signed-off-by: Fabio Graetz <[email protected]> * Add unit test for creation of task node with image override Signed-off-by: Fabio Graetz <[email protected]> * Add unit test for building a pod with container image override Signed-off-by: Fabio Graetz <[email protected]> * Re-generate flytidl Signed-off-by: Fabio Graetz <[email protected]> * Mock GetContainerImage for ExecutableNode Signed-off-by: Fabio Graetz <[email protected]> * Make dask tests pass Signed-off-by: Fabio Graetz <[email protected]> * Lint Signed-off-by: Fabio Graetz <[email protected]> * Make kubeflow tests pass Signed-off-by: Fabio Graetz <[email protected]> * Make spark tests pass Signed-off-by: Fabio Graetz <[email protected]> * Make ray tests pass Signed-off-by: Fabio Graetz <[email protected]> * Fix remaining flyteplugins tests Signed-off-by: Fabio Graetz <[email protected]> * Add container test for image overriding Signed-off-by: Fabio Graetz <[email protected]> * Add container image override test for pytorch jobs Signed-off-by: Fabio Graetz <[email protected]> * Add ray test for container image overriding Signed-off-by: Fabio Graetz <[email protected]> * Re-generate flyteidl Signed-off-by: Fabio Graetz <[email protected]> * Add comment about container_image to TaskNodOverride proto Signed-off-by: Fabio Graetz <[email protected]> * Re-generate flyteidl Signed-off-by: Fabio Graetz <[email protected]> * Re-generate propeller Signed-off-by: Fabio Graetz <[email protected]> * Lint Signed-off-by: Fabio Graetz <[email protected]> * Re-generate flyteidl Signed-off-by: Fabio Graetz <[email protected]> --------- Signed-off-by: Fabio Graetz <[email protected]>

fg91 mentioned this pull request Feb 9, 2024

[BUG] Task node overrides not handled in flyteidl are broken #4543

Open

4 tasks

fg91 force-pushed the fg91/fix/handle-container-image-override-backend branch from 8df6fe5 to eb687b8 Compare February 9, 2024 19:09

fg91 force-pushed the fg91/fix/handle-container-image-override-backend branch from f8b37d5 to bd96c79 Compare February 9, 2024 20:39

fg91 mentioned this pull request Feb 9, 2024

Fix: Handle overriding of container image in backend flyteorg/flytekit#2176

Merged

3 tasks

fg91 changed the title ~~[WIP] Fix: Handle overriding of container image in backend~~ Fix: Handle overriding of container image in backend Feb 9, 2024

fg91 marked this pull request as ready for review February 9, 2024 20:44

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Feb 9, 2024

fg91 self-assigned this Feb 9, 2024

dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Feb 9, 2024

fg91 requested review from jeevb and hamersaw February 9, 2024 21:48

pingsutw reviewed Feb 14, 2024

View reviewed changes

flyteidl/protos/flyteidl/core/workflow.proto Show resolved Hide resolved

fg91 force-pushed the fg91/fix/handle-container-image-override-backend branch from 7f934cb to faf3d3e Compare February 15, 2024 14:50

fg91 added 13 commits February 15, 2024 16:36

Add container image to TaskNodeOverrides proto

13f41ae

Signed-off-by: Fabio Graetz <[email protected]>

Add container image override to v1alpha1.NodeSpec

296bd99

Signed-off-by: Fabio Graetz <[email protected]>

Override container image in pod

4c38cfb

Signed-off-by: Fabio Graetz <[email protected]>

Add unit test for creation of task node with image override

59851d6

Signed-off-by: Fabio Graetz <[email protected]>

Add unit test for building a pod with container image override

dc282b5

Signed-off-by: Fabio Graetz <[email protected]>

Re-generate flytidl

0ec9f92

Signed-off-by: Fabio Graetz <[email protected]>

Mock GetContainerImage for ExecutableNode

81e812e

Signed-off-by: Fabio Graetz <[email protected]>

Make dask tests pass

38752db

Signed-off-by: Fabio Graetz <[email protected]>

Lint

3ea65a0

Signed-off-by: Fabio Graetz <[email protected]>

Make kubeflow tests pass

d4960e6

Signed-off-by: Fabio Graetz <[email protected]>

Make spark tests pass

d974679

Signed-off-by: Fabio Graetz <[email protected]>

Make ray tests pass

6298ec0

Signed-off-by: Fabio Graetz <[email protected]>

Fix remaining flyteplugins tests

38652fc

Signed-off-by: Fabio Graetz <[email protected]>

fg91 added 9 commits February 15, 2024 16:36

Add container test for image overriding

cc1ea28

Signed-off-by: Fabio Graetz <[email protected]>

Add container image override test for pytorch jobs

4b0fae2

Signed-off-by: Fabio Graetz <[email protected]>

Add ray test for container image overriding

1fe78e3

Signed-off-by: Fabio Graetz <[email protected]>

Re-generate flyteidl

cdc8d2f

Signed-off-by: Fabio Graetz <[email protected]>

Add comment about container_image to TaskNodOverride proto

6ec7c00

Signed-off-by: Fabio Graetz <[email protected]>

Re-generate flyteidl

b870093

Signed-off-by: Fabio Graetz <[email protected]>

Re-generate propeller

7d4883d

Signed-off-by: Fabio Graetz <[email protected]>

Lint

fb52e40

Signed-off-by: Fabio Graetz <[email protected]>

Re-generate flyteidl

53f1f4d

Signed-off-by: Fabio Graetz <[email protected]>

fg91 force-pushed the fg91/fix/handle-container-image-override-backend branch from 4b0979d to 53f1f4d Compare February 15, 2024 15:38

fg91 requested a review from pingsutw February 15, 2024 16:31

pingsutw approved these changes Feb 16, 2024

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Feb 16, 2024

hamersaw approved these changes Feb 21, 2024

View reviewed changes

hamersaw merged commit e39acc7 into master Feb 21, 2024
49 checks passed

hamersaw deleted the fg91/fix/handle-container-image-override-backend branch February 21, 2024 02:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Handle overriding of container image in backend #4858

Fix: Handle overriding of container image in backend #4858

fg91 commented Feb 7, 2024 •

edited

Loading

codecov bot commented Feb 9, 2024 •

edited

Loading

fg91 commented Feb 9, 2024

pingsutw left a comment

Fix: Handle overriding of container image in backend #4858

Fix: Handle overriding of container image in backend #4858

Conversation

fg91 commented Feb 7, 2024 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Check all the applicable boxes

Related PRs

Docs link

codecov bot commented Feb 9, 2024 • edited Loading

Codecov Report

fg91 commented Feb 9, 2024

pingsutw left a comment

Choose a reason for hiding this comment

fg91 commented Feb 7, 2024 •

edited

Loading

codecov bot commented Feb 9, 2024 •

edited

Loading