Validate pytorchjob workers are configured when elasticpolicy is configured #2320
Conversation
Signed-off-by: tarat44 <[email protected]> Co-authored-by: ricardov1 <[email protected]> Co-authored-by: alenawang <[email protected]>
tarat44
changed the title
tarat44
changed the title
Pull Request Test Coverage Report for Build 12184193267Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
@tarat44 In advance, could you sign to DCO? https://github.com/kubeflow/training-operator/pull/2320/checks?check_run_id=32498218264 |
tarat44
force-pushed
the
validate-workers-for-elasticpolicy
branch
from
fad6922 to
a4a0050
Compare
@tenzen-y Thanks for bringing this to my attention, and I apologize for the delay. I combined the commits into one and provided the sign off message. |
|
@tenzen-y @andreyvelich @kuizhiqing I wanted to follow up on this to see if it would be possible to have this fix in the next release of the training operator version 1 |
There was a problem hiding this comment.
Thanks for this contribution @tarat44!
Overall, lgtm
/assign @kubeflow/wg-training-leads @Electronic-Waste
| workerPath := specPath.Child("pytorchReplicaSpecs").Child("Worker") | ||
| if !ok { | ||
| allErrs = append(allErrs, field.Required(workerPath, "must be configured if elastic policy is used")) | ||
| } else if workerReplicaSpec.Replicas != nil && int(*workerReplicaSpec.Replicas) < 1 { |
There was a problem hiding this comment.
Do you need this check ? Isn't the default value for Replicas is 1 ?
There was a problem hiding this comment.
We added this validation to cover the case where replicas is explicitly set to 0 since the default is only set when it is nil https://github.com/kubeflow/training-operator/blob/master/pkg/apis/kubeflow.org/v1/defaulting_utils.go#L43
There was a problem hiding this comment.
Maybe we should have a separate validation that disallow users to set Replicas < 1 since it is invalid option ?
There was a problem hiding this comment.
Yes, that makes sense- we will update it to a separate validation that won't depend on whether elastic policy is set. Is it okay to have both validations in the same PR or would you prefer we separate that one into its own PR?
There was a problem hiding this comment.
I think, it is fine to add this simple check in this PR if you can @tarat44!
There was a problem hiding this comment.
@andreyvelich, we separated the validation for worker replicas being at least 1. I also realized that we had worker as an attribute, instead of a key in the pytorchReplicaSpecs map, in the error field path so we fixed it to use the map representation. It should be good to re-review now
Co-authored-by: Andrey Velichkevich <[email protected]> Signed-off-by: Tara Tufano <[email protected]>
Signed-off-by: tarat44 <[email protected]> Co-authored-by: ricardov1 <[email protected]>
|
Thanks for this work @tarat44! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This PR adds a check in the Pytorchjob validating webhook to ensure that if a user configures an elastic policy that they have defined a worker spec. Previously defining an elastic policy with no worker spec was resulting in a nil pointer exception on this line. Additionally, we added validation to ensure that the worker spec defines at least one replica.
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes #2278
Checklist: