Refine validation messages for requiring worker replica when elastic …

…policy is used Co-authored-by: ricardov1 <[email protected]> Co-authored-by: alenawang <[email protected]>
kubeflow · Nov 4, 2024 · fad6922 · fad6922
1 parent 603bf22
commit fad6922
Show file tree

Hide file tree

Showing 2 changed files with 8 additions and 5 deletions.
diff --git a/pkg/webhooks/pytorch/pytorchjob_webhook.go b/pkg/webhooks/pytorch/pytorchjob_webhook.go
@@ -95,9 +95,12 @@ func validateSpec(spec trainingoperator.PyTorchJobSpec) (admission.Warnings, fie
 	var warnings admission.Warnings
 	if spec.ElasticPolicy != nil {
 		workerReplicaSpec, ok := spec.PyTorchReplicaSpecs[trainingoperator.PyTorchJobReplicaTypeWorker]
-		if !ok || (workerReplicaSpec.Replicas != nil && int(*workerReplicaSpec.Replicas) < 1) {
-			workerPath := specPath.Child("pytorchReplicaSpecs").Child("Worker")
-			allErrs = append(allErrs, field.Required(workerPath, "at least one worker must be configured if elastic policy is used"))
+		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 {
+			workerReplicasPath := workerPath.Child("replicas")
+			allErrs = append(allErrs, field.Forbidden(workerReplicasPath, "must be at least 1 if elastic policy is used"))
 		}
 		if spec.ElasticPolicy.NProcPerNode != nil {
 			elasticNProcPerNodePath := specPath.Child("elasticPolicy").Child("nProcPerNode")

diff --git a/pkg/webhooks/pytorch/pytorchjob_webhook_test.go b/pkg/webhooks/pytorch/pytorchjob_webhook_test.go
@@ -389,7 +389,7 @@ func TestValidateV1PyTorchJob(t *testing.T) {
 				},
 			},
 			wantErr: field.ErrorList{
-				field.Required(field.NewPath("spec", "pytorchReplicaSpecs", "Worker"), "at least one worker must be configured if elastic policy is used"),
+				field.Required(field.NewPath("spec", "pytorchReplicaSpecs", "Worker"), "must be configured if elastic policy is used"),
 			},
 		},
 		"attempt to configure elasticPolicy when worker replicas is 0": {
@@ -417,7 +417,7 @@ func TestValidateV1PyTorchJob(t *testing.T) {
 				},
 			},
 			wantErr: field.ErrorList{
-				field.Required(field.NewPath("spec", "pytorchReplicaSpecs", "Worker"), "at least one worker must be configured if elastic policy is used"),
+				field.Forbidden(field.NewPath("spec", "pytorchReplicaSpecs", "Worker", "replicas"), "must be at least 1 if elastic policy is used"),
 			},
 		},
 	}