Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

[BUG] The primaryContainerName of podtemplate tasks is not overrode with kubeflow specific name #340

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

ByronHsu
Copy link
Contributor

@ByronHsu ByronHsu commented Apr 3, 2023

TL;DR

Please replace this text with a description of what this PR accomplishes.

The primaryContainerName of podtemplate tasks is not overrode with kubeflow specific name

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

In the previous implementation, we assumed all tasks are container task, so we used taskCtx.TaskExecutionMetadata().GetTaskExecutionID().GetGeneratedName() to get the primary container name. However, in the case of pod template, the primary container name can vary.

The solution is to expose primaryContainerName from ToK8sPodSpec and pass to OverridePrimaryContainerName (which was named OverrideDefaultContainerName).

Tracking Issue

flyteorg/flyte#3567

@ByronHsu ByronHsu changed the title [BUG] The primaryContainerName of podtemplate tasks are not overrode with kubeflow specific ones [BUG] The primaryContainerName of podtemplate tasks are not overrode with kubeflowContainerName Apr 3, 2023
@ByronHsu ByronHsu changed the title [BUG] The primaryContainerName of podtemplate tasks are not overrode with kubeflowContainerName [BUG] The primaryContainerName of podtemplate tasks is not overrode with kubeflow specific name Apr 3, 2023
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #340 (d18c987) into master (38e8a07) will increase coverage by 1.33%.
The diff coverage is 66.66%.

❗ Current head d18c987 differs from pull request most recent head c4e531a. Consider uploading reports for the commit c4e531a to get more accurate results

@@            Coverage Diff             @@
##           master     #340      +/-   ##
==========================================
+ Coverage   62.66%   63.99%   +1.33%     
==========================================
  Files         146      146              
  Lines       12226     9907    -2319     
==========================================
- Hits         7661     6340    -1321     
+ Misses       3983     2985     -998     
  Partials      582      582              
Flag Coverage Δ
unittests ?

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

Impacted Files Coverage Δ
.../plugins/k8s/kfoperators/common/common_operator.go 80.20% <0.00%> (+8.90%) ⬆️
go/tasks/pluginmachinery/flytek8s/pod_helper.go 69.81% <50.00%> (-3.21%) ⬇️
go/tasks/plugins/k8s/kfoperators/mpi/mpi.go 75.00% <100.00%> (ø)
...o/tasks/plugins/k8s/kfoperators/pytorch/pytorch.go 73.07% <100.00%> (+0.73%) ⬆️
...s/plugins/k8s/kfoperators/tensorflow/tensorflow.go 75.00% <100.00%> (+0.24%) ⬆️

... and 125 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@fg91 fg91 self-requested a review April 13, 2023 18:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants