KEP-2170: Adding validation webhook for v2 trainjob #2307

akshaychitneni · 2024-10-24T21:34:20Z

Adds validation webhook for v2 trainjob.
Relates to #2209

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes # #2209

Checklist:

Docs included if any changes are user facing

google-oss-prow · 2024-10-24T21:34:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andreyvelich for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

akshaychitneni · 2024-10-24T21:35:28Z

cc @tenzen-y @andreyvelich

coveralls · 2024-10-25T17:12:16Z

Pull Request Test Coverage Report for Build 11709371378

Details

6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 11663764609:	0.0%
Covered Lines:	78
Relevant Lines:	78

💛 - Coveralls

tenzen-y

Thank you for taking this, and moving this forward.
And Sorry for the delay.

pkg/controller.v2/trainjob_controller.go

tenzen-y · 2024-10-28T19:06:14Z

pkg/runtime.v2/core/clustertrainingruntime.go

+		Namespace: new.Namespace,
+		Name:      new.Spec.RuntimeRef.Name,


Have you ever seen the isseus when we use the old object names?

Why do we get new object here and not old ?

pkg/runtime.v2/core/trainingruntime.go

pkg/runtime.v2/framework/plugins/jobset/jobset.go

tenzen-y · 2024-10-28T19:12:31Z

pkg/runtime.v2/framework/plugins/jobset/jobset.go

@@ -140,3 +143,115 @@ func (j *JobSet) ReconcilerBuilders() []runtime.ReconcilerBuilder {
 		},
 	}
 }
+
+func (j *JobSet) Validate(oldObj, newObj *kubeflowv2.TrainJob, runtimeInfo *runtime.Info) (admission.Warnings, field.ErrorList) {


It seems that there are some conflicts between @andreyvelich PR and this.
@akshaychitneni Could you consult with @andreyvelich, then which PRs should we merge into the main, first.

I rebased with @andreyvelich's changes

tenzen-y · 2024-10-28T19:20:28Z

pkg/webhook.v2/setup.go

@@ -31,7 +31,7 @@ func Setup(mgr ctrl.Manager, runtimes map[string]runtime.Runtime) (string, error
 		return kubeflowv2.TrainingRuntimeKind, err
 	}
 	if err := setupWebhookForTrainJob(mgr, runtimes); err != nil {
-		return "TrainJob", err
+		return kubeflowv2.TrainJobKind, err


Nice catch.

pkg/webhook.v2/trainjob_webhook.go

tenzen-y · 2024-10-28T19:24:35Z

pkg/webhook.v2/trainjob_webhook.go

Cool! This is what I imagined architechture in my KubeflowJobPipeline framework design phase.

tenzen-y · 2024-10-28T19:25:43Z

test/integration/framework/framework.go

-	failedCtrlName, err := controllerv2.SetupControllers(mgr, runtimes)
-	gomega.ExpectWithOffset(1, err).NotTo(gomega.HaveOccurred(), "controller", failedCtrlName)
-	gomega.ExpectWithOffset(1, failedCtrlName).To(gomega.BeEmpty())
+	if startControllers {


Have you ever seen any issues like null pointer when we start the controllers for webhook testing, right?

I think I have seen but we might not need to start the controllers just to validate create/update requests and leave to reconciler tests to cover reconciliation

fixing runtime Signed-off-by: Akshay Chitneni <[email protected]>

andreyvelich

Thank you for this effort @akshaychitneni!
I left initial comments.

andreyvelich · 2024-11-07T15:30:39Z

pkg/constants/constants.go

+	// JobExporter is the Job name for the exporter.
+	JobExporter string = "exporter"
+


Please can we implement the validation for exporter in the future once we design it as part of: #2245 ?
We should discuss whether we want to use sidecar container or another ReplicatedJob for model checkpointing.
cc @saileshd1402 @akshaychitneni @tenzen-y

andreyvelich · 2024-11-07T15:30:49Z

pkg/constants/constants.go

 	// ContainerModelInitializer is the container name for the model initializer.
 	ContainerModelInitializer string = "model-initializer"

+	// ContainerModelExporter is the container name for the model exporter.
+	ContainerModelExporter string = "model-exporter"


The same for the container.

andreyvelich · 2024-11-07T15:33:01Z

pkg/util.v2/runtime/runtime.go

@@ -0,0 +1,17 @@
+package runtime


If it is a runtime util, don't we want to move it under /pkg/runtime.v2/util @akshaychitneni @tenzen-y ?

andreyvelich · 2024-11-07T15:38:13Z

pkg/runtime.v2/core/trainingruntime.go

+	return r.framework.RunComponentBuilderPlugins(ctx, jobSetTemplate.DeepCopy(), info, trainJob)
+}
+
+func (r *TrainingRuntime) runtimeInfo(


Should this be part of Runtime interface:

training-operator/pkg/runtime.v2/interface.go

Line 32 in a93ffb7

type Runtime interface {

And should we name this API more explicit (e.g. getRuntimeInfo() or initializeRuntimeInfo()) ?

andreyvelich · 2024-11-07T15:40:00Z

pkg/runtime.v2/core/clustertrainingruntime.go

+		Namespace: new.Namespace,
+		Name:      new.Spec.RuntimeRef.Name,


Why do we get new object here and not old ?

andreyvelich · 2024-11-07T15:56:26Z

pkg/runtime.v2/framework/plugins/mpi/mpi.go

+		numProcPerNodePath := specPath.Child("trainer").Child("numProcPerNode")
+		if runtimeInfo.MLPolicy.MPI != nil {
+			if _, err := strconv.Atoi(*newJobObj.Spec.Trainer.NumProcPerNode); err != nil {
+				allErrs = append(allErrs, field.Invalid(numProcPerNodePath, newJobObj.Spec.Trainer.NumProcPerNode, "should have an int value"))


I think so, is this value compatible with the k8s API conventions: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md ?

andreyvelich · 2024-11-07T15:59:26Z

pkg/runtime.v2/framework/plugins/torch/torch.go

+		numProcPerNodePath := specPath.Child("trainer").Child("numProcPerNode")
+		if runtimeInfo.RuntimePolicy.MLPolicy.Torch != nil && newObj.Spec.Trainer.NumProcPerNode != nil {
+			allowedStringValList := []string{"auto", "cpu", "gpu"}
+			numProcPerNode := *newObj.Spec.Trainer.NumProcPerNode


@akshaychitneni @tenzen-y Can't we use CEL for that validation since we just validate values for .nProcPerNode parameter ?

andreyvelich · 2024-11-07T16:00:47Z

pkg/runtime.v2/framework/plugins/torch/torch.go

-// TODO: Need to implement validateions for TorchJob.
-func (t *Torch) Validate(oldObj, newObj *kubeflowv2.TrainJob) (admission.Warnings, field.ErrorList) {
-	return nil, nil
+func (t *Torch) Validate(runtimeJobTemplate client.Object, runtimeInfo *runtime.Info, oldObj, newObj *kubeflowv2.TrainJob) (admission.Warnings, field.ErrorList) {


As I mentioned here, we should also validate that TrainJob doesn't have PET_ Trainer envs

training-operator/pkg/runtime.v2/framework/plugins/torch/torch.go

Line 73 in a93ffb7

// TODO (andreyvelich): Add validation to check that TrainJob doesn't have "PET_" envs.

andreyvelich · 2024-11-07T16:07:01Z

pkg/runtime.v2/framework/plugins/jobset/jobset.go

+		return nil, nil
+	}
+
+	if newObj.Spec.ModelConfig != nil && newObj.Spec.ModelConfig.Input != nil {


I think, for now we should check the initContainers in JobSet, as I mentioned here: https://github.com/kubeflow/training-operator/blob/master/pkg/runtime.v2/framework/plugins/jobset/builder.go#L87-L89

andreyvelich · 2024-11-07T16:09:46Z

test/integration/webhook.v2/trainjob_test.go

+		gomega.Expect(k8sClient.DeleteAllOf(ctx, &kubeflowv2.TrainJob{}, client.InNamespace(ns.Name))).To(gomega.Succeed())
+	})
+
+	ginkgo.When("Creating TrainJob", func() {


@tenzen-y @akshaychitneni What is right way to test our validations with integration or unit tests ?

google-oss-prow bot requested a review from jinchihe October 24, 2024 21:34

google-oss-prow bot requested a review from kuizhiqing October 24, 2024 21:34

google-oss-prow bot added the size/XL label Oct 24, 2024

akshaychitneni force-pushed the webhookv2 branch 5 times, most recently from 892a40b to f1a06c4 Compare October 25, 2024 16:36

google-oss-prow bot added size/L and removed size/XL labels Oct 25, 2024

akshaychitneni force-pushed the webhookv2 branch 3 times, most recently from ce983eb to 736a759 Compare October 25, 2024 17:09

akshaychitneni force-pushed the webhookv2 branch 2 times, most recently from f85da83 to ba32e68 Compare October 25, 2024 18:23

google-oss-prow bot added size/XL and removed size/L labels Oct 25, 2024

akshaychitneni force-pushed the webhookv2 branch from ba32e68 to 20136ef Compare October 25, 2024 18:46

google-oss-prow bot added size/L and removed size/XL labels Oct 25, 2024

akshaychitneni force-pushed the webhookv2 branch from 20136ef to 0aa9ee0 Compare October 25, 2024 18:50

tenzen-y reviewed Oct 28, 2024

View reviewed changes

akshaychitneni force-pushed the webhookv2 branch from 0aa9ee0 to 1b675c5 Compare November 3, 2024 17:52

google-oss-prow bot added size/XL and removed size/L labels Nov 3, 2024

akshaychitneni force-pushed the webhookv2 branch 2 times, most recently from 0e12bb4 to a3ea261 Compare November 4, 2024 22:39

akshaychitneni force-pushed the webhookv2 branch 4 times, most recently from 9d15d26 to f4d1430 Compare November 6, 2024 17:57

Adding v2 trainjob validation webhook

a93ffb7

fixing runtime Signed-off-by: Akshay Chitneni <[email protected]>

akshaychitneni force-pushed the webhookv2 branch from f4d1430 to a93ffb7 Compare November 6, 2024 18:08

andreyvelich reviewed Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2170: Adding validation webhook for v2 trainjob #2307

KEP-2170: Adding validation webhook for v2 trainjob #2307

akshaychitneni commented Oct 24, 2024 •

edited

Loading

google-oss-prow bot commented Oct 24, 2024

akshaychitneni commented Oct 24, 2024

coveralls commented Oct 25, 2024 •

edited

Loading

tenzen-y left a comment

tenzen-y Oct 28, 2024

andreyvelich Nov 7, 2024

tenzen-y Oct 28, 2024

akshaychitneni Nov 6, 2024 •

edited

Loading

tenzen-y Oct 28, 2024

tenzen-y Oct 28, 2024

tenzen-y Oct 28, 2024

akshaychitneni Oct 30, 2024

andreyvelich left a comment

andreyvelich Nov 7, 2024

andreyvelich Nov 7, 2024

andreyvelich Nov 7, 2024

andreyvelich Nov 7, 2024

andreyvelich Nov 7, 2024

andreyvelich Nov 7, 2024

andreyvelich Nov 7, 2024

andreyvelich Nov 7, 2024

andreyvelich Nov 7, 2024

andreyvelich Nov 7, 2024 •

edited

Loading

		// JobExporter is the Job name for the exporter.
		JobExporter string = "exporter"

KEP-2170: Adding validation webhook for v2 trainjob #2307

Are you sure you want to change the base?

KEP-2170: Adding validation webhook for v2 trainjob #2307

Conversation

akshaychitneni commented Oct 24, 2024 • edited Loading

google-oss-prow bot commented Oct 24, 2024

akshaychitneni commented Oct 24, 2024

coveralls commented Oct 25, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11709371378

Details

💛 - Coveralls

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshaychitneni Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

akshaychitneni commented Oct 24, 2024 •

edited

Loading

coveralls commented Oct 25, 2024 •

edited

Loading

akshaychitneni Nov 6, 2024 •

edited

Loading

andreyvelich Nov 7, 2024 •

edited

Loading