Support kubeflow.org/xgboostjob #1114

tenzen-y · 2023-09-12T19:42:41Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Support kubeflow.org/xgboostjob.

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/xgboostjob

k8s-ci-robot · 2023-09-12T19:42:47Z

[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

netlify · 2023-09-12T19:42:47Z

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	`e4fceeb`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/650190d48e18ef0008f89e97
😎 Deploy Preview	https://deploy-preview-1114--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

tenzen-y · 2023-09-12T19:43:07Z

/hold for review

mimowo

LGTM, have you tested this on a running cluster?
/assign @alculquicondor

test/integration/controller/jobs/xgboostjob/xgboostjob_controller_test.go

mimowo · 2023-09-13T09:28:26Z

test/integration/controller/jobs/xgboostjob/xgboostjob_controller_test.go

+			return len(createdWorkload.Status.Conditions) == 2
+		}, util.ConsistentDuration, util.Interval).Should(gomega.BeTrue())
+
+		ginkgo.By("checking the job gets suspended when parallelism changes and the added node selectors are removed")


non-blocking nit: we could make the integration test shorter as the implementation is pretty well commonized. I'm thinking of this scenario as a potential overkill, but I don't have a strong view as some integration tests are needed as a scenarios like this is probably <1s anyway.

That's a good point.

we could make the integration test shorter as the implementation is pretty well commonized. I'm thinking of this scenario as a potential overkill

Which does that indicate all integrations or all kubeflow jobs?

We have pretty well commonized code at both levels - between integrations, and kubeflow integrations in particular. As a result each kubeflow integration implements only around 10 functions, and all are one-liners.

That's a good point.

we could make the integration test shorter as the implementation is pretty well commonized. I'm thinking of this scenario as a potential overkill

Which does that indicate all integrations or all kubeflow jobs?

I would focus only on kubeflow jobs here.

It makes sense. I think moving this case to a unit test might be better since this scenario might be useful to check expected behavior although this is overkill as integration tests.
WDYT?

sgtm, still I would kieep some basic integration tests per integratoin, can be follow up

Let's keep tracking by creating an issue.

sgtm, would you like to create one?

I apologize that I forgot to push Submit new Issue button 😞
Created: #1119

tenzen-y · 2023-09-13T09:55:53Z

LGTM, have you tested this on a running cluster? /assign @alculquicondor

Yes, I verified this feature on a real cluster using the following XGBoostJob:

apiVersion: kubeflow.org/v1
kind: XGBoostJob
metadata:
  name: xgboost-dist-iris-test-train
  namespace: default
  labels:
    kueue.x-k8s.io/queue-name: user-queue
spec:
  xgbReplicaSpecs:
    Master:
      replicas: 1
      restartPolicy: Never
      template:
        spec:
          containers:
          - name: xgboost
            image: docker.io/merlintang/xgboost-dist-iris:1.1
            resources:
              requests:
                cpu: 0.5
                memory: 256Mi
            ports:
            - containerPort: 9991
              name: xgboostjob-port
            imagePullPolicy: Always
            args:
              - --job_type=Train
              - --xgboost_parameter=objective:multi:softprob,num_class:3
              - --n_estimators=10
              - --learning_rate=0.1
              - --model_path=/tmp/xgboost-model
              - --model_storage_type=local
    Worker:
      replicas: 2
      restartPolicy: ExitCode
      template:
        spec:
          containers:
          - name: xgboost
            image: docker.io/merlintang/xgboost-dist-iris:1.1
            resources:
              requests:
                cpu: 0.5
                memory: 256Mi
            ports:
            - containerPort: 9991
              name: xgboostjob-port
            imagePullPolicy: Always
            args:
              - --job_type=Train
              - --xgboost_parameter="objective:multi:softprob,num_class:3"
              - --n_estimators=10
              - --learning_rate=0.1

I will create a PR for the document once #1109 (comment) is resolved.

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

tenzen-y · 2023-09-13T10:13:58Z

/remove-kind documentation
/kind feature

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

tenzen-y · 2023-09-13T10:46:30Z

/retest
Due to #1090

tenzen-y · 2023-09-13T10:56:05Z

#1090 happened again :(

/test pull-kueue-test-e2e-main-1-24

mimowo · 2023-09-14T07:01:50Z

/lgtm
/assign @trasc
in case @trasc would like to give a review pass as he reviewed the previous kubeflow integrations

k8s-ci-robot · 2023-09-14T07:01:56Z

LGTM label has been added.

Git tree hash: 915ed626e7bc079fe2a4b79283648146bf2fa7ce

trasc

/lgtm

tenzen-y · 2023-09-15T08:00:11Z

Let's merge this.
/hold cancel

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/documentation Categorizes issue or PR as related to documentation. labels Sep 12, 2023

k8s-ci-robot requested review from ahg-g and ArangoGutierrez September 12, 2023 19:42

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 12, 2023

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

mimowo reviewed Sep 13, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor Sep 13, 2023

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

Support kubeflow.org/xgboostjob

e97f95d

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

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/documentation Categorizes issue or PR as related to documentation. labels Sep 13, 2023

Remove duplicated constants in the integration test

e4fceeb

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

tenzen-y force-pushed the add-support-xgboostjob branch from 556de27 to e4fceeb Compare September 13, 2023 10:37

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

tenzen-y mentioned this pull request Sep 13, 2023

Remove duplicated constants in the integration tests #1116

Merged

k8s-ci-robot assigned trasc and mimowo Sep 14, 2023

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

This was referenced Sep 14, 2023

Commonize KubeflowJob Tests #1119

Closed

[DOC] Add tasks for XGBoostJob integration to site #1120

Merged

trasc reviewed Sep 15, 2023

View reviewed changes

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

tenzen-y mentioned this pull request Sep 15, 2023

Support kubeflow operator #297

Closed

2 tasks

k8s-ci-robot merged commit 731938d into kubernetes-sigs:main Sep 15, 2023
15 checks passed

k8s-ci-robot added this to the v0.5 milestone Sep 15, 2023

tenzen-y deleted the add-support-xgboostjob branch September 15, 2023 08:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support kubeflow.org/xgboostjob #1114

Support kubeflow.org/xgboostjob #1114

tenzen-y commented Sep 12, 2023 •

edited

k8s-ci-robot commented Sep 12, 2023

netlify bot commented Sep 12, 2023 •

edited

tenzen-y commented Sep 12, 2023

mimowo left a comment

mimowo Sep 13, 2023

tenzen-y Sep 13, 2023

mimowo Sep 13, 2023 •

edited

mimowo Sep 13, 2023

tenzen-y Sep 13, 2023

mimowo Sep 13, 2023

tenzen-y Sep 13, 2023

mimowo Sep 14, 2023

tenzen-y Sep 14, 2023

tenzen-y commented Sep 13, 2023

tenzen-y commented Sep 13, 2023

tenzen-y commented Sep 13, 2023

tenzen-y commented Sep 13, 2023

mimowo commented Sep 14, 2023

k8s-ci-robot commented Sep 14, 2023

trasc left a comment

tenzen-y commented Sep 15, 2023

Support kubeflow.org/xgboostjob #1114

Support kubeflow.org/xgboostjob #1114

Conversation

tenzen-y commented Sep 12, 2023 • edited

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?

k8s-ci-robot commented Sep 12, 2023

netlify bot commented Sep 12, 2023 • edited

✅ Deploy Preview for kubernetes-sigs-kueue ready!

tenzen-y commented Sep 12, 2023

mimowo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo Sep 13, 2023 • edited

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

tenzen-y commented Sep 13, 2023

tenzen-y commented Sep 13, 2023

tenzen-y commented Sep 13, 2023

tenzen-y commented Sep 13, 2023

mimowo commented Sep 14, 2023

k8s-ci-robot commented Sep 14, 2023

trasc left a comment

Choose a reason for hiding this comment

tenzen-y commented Sep 15, 2023

tenzen-y commented Sep 12, 2023 •

edited

netlify bot commented Sep 12, 2023 •

edited

mimowo Sep 13, 2023 •

edited