Support kubeflow.org/pytorchjob #995

tenzen-y · 2023-07-18T06:47:37Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Support kubeflow.org/pytorchjob.

Which issue(s) this PR fixes:

Part-of #297

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Support kubeflow.org/pytorchjob

netlify · 2023-07-18T06:47:44Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`775ca92`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/64db11c388c0cb00082002b3

tenzen-y · 2023-07-18T16:02:37Z

Blocked on kubeflow/trainer#1859.

tenzen-y · 2023-07-18T16:08:44Z

/hold

tenzen-y · 2023-07-18T16:14:49Z

/assign @trasc

trasc

The generic approach around pkg/controller/jobs/kubeflow/kubeflowjob makes the implementation a bit hard to follow. Are we planing to support other similar kubeflow jobs in the near future?

trasc · 2023-07-19T05:25:21Z

config/components/manager/controller_manager_config.yaml

@@ -32,3 +32,4 @@ integrations:
  - "kubeflow.org/mpijob"
  - "ray.io/rayjob"
  - "jobset.x-k8s.io/jobset"
+  - "kubeflow.org/pytorchjob"


The same should be done in charts/kueue/values.yaml

kueue/charts/kueue/values.yaml

Lines 84 to 88 in f215a43

integrations:

frameworks:

- "batch/job"

- "kubeflow.org/mpijob"

- "ray.io/rayjob"

trasc · 2023-07-19T05:32:03Z

config/components/rbac/role.yaml

+- apiGroups:
+  - kubeflow.org
+  resources:
+  - pytorchjobs
+  verbs:
+  - get
+  - list
+  - patch
+  - update
+  - watch
+- apiGroups:
+  - kubeflow.org
+  resources:
+  - pytorchjobs/status
+  verbs:
+  - get
+  - update


two extra role files should be created config/components/rbac/pytorchjob_editor_role.yaml and config/components/rbac/pytorchjob_viewer_role.yaml (check the content of config/components/rbac/jobset_XXX_role.yaml ) and referenced in config/components/rbac/kustomization.yaml

It's a good point. Thanks.

trasc · 2023-07-19T05:43:24Z

go.mod

@@ -83,3 +85,5 @@ require (
 	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
 	sigs.k8s.io/yaml v1.3.0 // indirect
 )
+
+replace github.com/kubeflow/training-operator v1.6.0 => github.com/tenzen-y/training-operator v1.3.0-rc.1.0.20230717233919-1ed3e8e55322


we should drop the replace and either:

a. Hold this PR until kubeflow/trainer#1859 is merged and a new kubeflow/training-operator is available containing it.
b. Hold this PR until kubeflow/trainer#1859 is merged, use a non-release version of github.com/kubeflow/training-operator v1.7.0-xxx-xxxx and create a Followup pr to switch to a release version when available.

Yes, I will do b.

trasc · 2023-07-19T06:11:47Z

pkg/controller/jobs/kubeflow/jobs/pytorchjob/pytorchjob_controller.go

+type PyTorchJob struct {
+	*kftraining.PyTorchJob
+	*kubeflowjob.KubeflowJob
+}


If you make Object() part of the KFJobControl you could remove the kftraining.PyTorchJob member, and potentially change this to :

Suggested change

type PyTorchJob struct {

*kftraining.PyTorchJob

*kubeflowjob.KubeflowJob

}

type PyTorchJob kubeflowjob.KubeflowJob

It makes sense.

For Composition, we need to use an embedded struct. So I will modify this in the following:

type PyTorchJob struct { *kubeflowjob.KubeflowJob }

I think it should work the same way, it is used like this to "make" the generic job out of all integrations we currently have.

@trasc Does that mean we should use the type alias instead of the embedded struct?

If doing so, it means that we remove kubeflowjob.KubeflowJob, and then in all kubeflow integrations (TFJob, PaddleJob, ...), we declare the same processes such as RunWithPodSetsInfo, RestorePodSetsInfo and so on.

thinking about it , you could do the same for GetGVK() and might be able to make KubeflowJob generic and

var NewReconciler = jobframework.NewGenericReconciler(func() jobframework.GenericJob { - pytorchJobObj := &kftraining.PyTorchJob{} - return &PyTorchJob{PyTorchJob: pytorchJobObj, KubeflowJob: kubeflowjob.NewKubeflowJob((*JobControl)(pytorchJobObj))} + return &kftraining.PyTorchJob{} }, nil) ... -type PyTorchJob struct { - *kftraining.PyTorchJob - *kubeflowjob.KubeflowJob -} + type PyTorchJob KubeflowJob[JobControl]

If Object is part of KFJobControl, then you can change

func (j *KubeflowJob) Object() client.Object {

return nil

return j.KFJobControl.Object()
}
Then you don't need the *kftraining.PyTorchJob memeber.

Then you can simplify the struct, and also make it a type "redefinition" type PyTorchJob kubeflowjob.KubeflowJob.

Thanks for the clarifications.
If doing so, I think type PyTorchJob kubeflowjob.KubeflowJob doesn't implement the GenericJob interface, right?

Although type JobControl kftraining.PyTorchJob implement the GenericJob.

Tooo many interfaces :)... ,

To keep it simple I think kubeflowjob.KubeflowJob should have all the common code for kubeflow(kftraining)) Jobs implementing GenericJob. and use KFJobControl to get it's job (eg. kftraining.PyTorchJob) specific bits.

To avoid confusion, KFJobControl should be a named member, don't embed.

type KubeflowJob struct { c KFJobControl }

The reconciler setup could look like:

var NewReconciler = jobframework.NewGenericReconciler(func() jobframework.GenericJob { return &kubeflowjob.KubeflowJob{c: &JobControl{}} }, nil) type JobControl kftraining.PyTorchJob var _ kubeflowjob.KFJobControl = (*JobControl)(nil) ....

Uhm. Actually, I adopted the same pattern you suggested before submitting this PR. Then, I switched implementation to now one since this one is more reusable.

However, I will roll back my implementation to your suggested one since if @trasc, who has much understanding of the kueue, is confused by this implementation, then almost everyone will be confused.

pkg/controller/jobs/kubeflow/jobs/pytorchjob/pytorchjob_controller.go

trasc · 2023-07-19T06:26:22Z

pkg/controller/jobs/kubeflow/jobs/jobs.go

Missing header

also I think we could just import
"sigs.k8s.io/kueue/pkg/controller/jobs/kubeflow/jobs/pytorchjob"
in
pkg/controller/jobs/jobs.go

In the future, we will add more framework support. So, I would like to put the linking for kubeflow here.

// Reference the job framework integration packages to ensure linking. import ( _ "sigs.k8s.io/kueue/pkg/controller/jobs/kubeflow/jobs/pytorchjob" _ "sigs.k8s.io/kueue/pkg/controller/jobs/kubeflow/jobs/tfjob" _ "sigs.k8s.io/kueue/pkg/controller/jobs/kubeflow/jobs/paddlejob" _ "sigs.k8s.io/kueue/pkg/controller/jobs/kubeflow/jobs/xgboostjob" _ "sigs.k8s.io/kueue/pkg/controller/jobs/kubeflow/jobs/mxjob" )

Missing header

Done.

pkg/controller/jobs/kubeflow/kubeflowjob/interface.go

pkg/controller/jobs/kubeflow/kubeflowjob/kubeflowjob.go

tenzen-y · 2023-07-19T06:49:25Z

The generic approach around pkg/controller/jobs/kubeflow/kubeflowjob makes the implementation a bit hard to follow. Are we planing to support other similar kubeflow jobs in the near future?

@trasc Yes. We will support TFJob, MXJob, XGboostJob, and PaddleJob.
cc: @alculquicondor

kerthcet · 2023-07-19T06:53:52Z

/cc

tenzen-y · 2023-08-09T07:31:25Z

@trasc This PR is ready for review. PTAL.

trasc

/lgtm

tenzen-y · 2023-08-11T11:02:16Z

/assign @alculquicondor

Makefile

pkg/controller/jobs/kubeflow/kubeflowjob/kubeflowjob_controller.go

pkg/controller/jobs/kubeflow/jobs/pytorchjob/pytorchjob_controller.go

k8s-ci-robot · 2023-08-14T17:58:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

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:

~~OWNERS~~ [tenzen-y]

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

tenzen-y · 2023-08-14T19:01:50Z

The kueue-controller-manager seems suddenly can not access the kube-apiserver:
https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kueue/995/pull-kueue-test-e2e-main-1-24/1691153180097777664/artifacts/run-test-e2e-1.24.15/kueue-controller-manager.log
/retest

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y · 2023-08-15T05:49:25Z

Squashed.

trasc · 2023-08-16T06:04:11Z

/lgtm

alculquicondor · 2023-08-16T12:16:10Z

/release-note-edit

Support kubeflow.org/pytorchjob

/hold cancel

k8s-ci-robot requested review from alculquicondor and ArangoGutierrez July 18, 2023 06:47

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 18, 2023

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 18, 2023

tenzen-y force-pushed the support-kubeflow-pytorchjob branch 7 times, most recently from 801b061 to bd3d770 Compare July 18, 2023 12:50

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 18, 2023

tenzen-y force-pushed the support-kubeflow-pytorchjob branch 2 times, most recently from 61b2e25 to 92a16ca Compare July 18, 2023 16:08

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2023

tenzen-y changed the title ~~WIP: Support kubeflow.org/pytorchjob~~ Support kubeflow.org/pytorchjob Jul 18, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 18, 2023

k8s-ci-robot assigned trasc Jul 18, 2023

trasc reviewed Jul 19, 2023

View reviewed changes

k8s-ci-robot requested a review from kerthcet July 19, 2023 06:53

tenzen-y force-pushed the support-kubeflow-pytorchjob branch from 3589969 to fc23a88 Compare August 9, 2023 07:02

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2023

tenzen-y force-pushed the support-kubeflow-pytorchjob branch from 66222ab to 8fe23aa Compare August 9, 2023 07:12

tenzen-y changed the title ~~WIP: Support kubeflow.org/pytorchjob~~ Support kubeflow.org/pytorchjob Aug 9, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 9, 2023

tenzen-y force-pushed the support-kubeflow-pytorchjob branch from 8fe23aa to b384a36 Compare August 9, 2023 07:14

tenzen-y mentioned this pull request Aug 9, 2023

Add docs for the PyTorchJob integration #1048

Merged

trasc reviewed Aug 11, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2023

k8s-ci-robot assigned alculquicondor Aug 11, 2023

alculquicondor reviewed Aug 14, 2023

View reviewed changes

tenzen-y force-pushed the support-kubeflow-pytorchjob branch from b384a36 to 37201a3 Compare August 14, 2023 17:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2023

tenzen-y force-pushed the support-kubeflow-pytorchjob branch from 86c5b32 to 5fe074b Compare August 14, 2023 18:21

Support kubeflow.org/pytorchjob

775ca92

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y force-pushed the support-kubeflow-pytorchjob branch from 5fe074b to 775ca92 Compare August 15, 2023 05:48

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 16, 2023

k8s-ci-robot merged commit 04a1421 into kubernetes-sigs:main Aug 16, 2023

k8s-ci-robot added this to the v0.5 milestone Aug 16, 2023

tenzen-y deleted the support-kubeflow-pytorchjob branch August 16, 2023 12:31

tenzen-y mentioned this pull request Aug 16, 2023

Support kubeflow operator #297

Closed

2 tasks

	integrations:
	frameworks:
	- "batch/job"
	- "kubeflow.org/mpijob"
	- "ray.io/rayjob"

Support kubeflow.org/pytorchjob #995

Support kubeflow.org/pytorchjob #995

Conversation

tenzen-y commented Jul 18, 2023 • edited by k8s-ci-robot Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

netlify bot commented Jul 18, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

tenzen-y commented Jul 18, 2023

tenzen-y commented Jul 18, 2023

tenzen-y commented Jul 18, 2023

trasc 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

trasc Jul 19, 2023 • 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

trasc Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trasc Jul 20, 2023 • 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

Choose a reason for hiding this comment

tenzen-y commented Jul 19, 2023

kerthcet commented Jul 19, 2023

tenzen-y commented Aug 9, 2023

trasc left a comment

Choose a reason for hiding this comment

tenzen-y commented Aug 11, 2023

k8s-ci-robot commented Aug 14, 2023

tenzen-y commented Aug 14, 2023

tenzen-y commented Aug 15, 2023

trasc commented Aug 16, 2023

alculquicondor commented Aug 16, 2023

tenzen-y commented Jul 18, 2023 •

edited by k8s-ci-robot

Loading

netlify bot commented Jul 18, 2023 •

edited

Loading

trasc Jul 19, 2023 •

edited

Loading

trasc Jul 20, 2023 •

edited

Loading

trasc Jul 20, 2023 •

edited

Loading