-
Notifications
You must be signed in to change notification settings - Fork 671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[flyteplugins] Make the TF Job Worker Spec Correct #4169
[flyteplugins] Make the TF Job Worker Spec Correct #4169
Conversation
Signed-off-by: Future Outlier <[email protected]>
@@ -122,7 +122,7 @@ func (tensorflowOperatorResourceHandler) BuildResource(ctx context.Context, task | |||
workerReplicaSpec := kfTensorflowTaskExtraArgs.GetWorkerReplicas() | |||
if workerReplicaSpec != nil { | |||
err := common.OverrideContainerSpec( | |||
replicaSpecMap[kubeflowv1.MPIJobReplicaTypeWorker].PodSpec, | |||
replicaSpecMap[kubeflowv1.TFJobReplicaTypeWorker].PodSpec, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add a test for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes no problem, I will do it!
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4169 +/- ##
==========================================
+ Coverage 54.59% 59.29% +4.69%
==========================================
Files 178 550 +372
Lines 16038 39692 +23654
==========================================
+ Hits 8756 23535 +14779
- Misses 6568 13836 +7268
- Partials 714 2321 +1607
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's really no need for a test here right?
I can add it! |
Describe your changes
As title.
#4167