[KubeRay] Support NumOfHosts when calculating PodSet assignments #3384

andrewsykim · 2024-10-31T03:11:41Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

NumOfHosts is a new field we added to KubeRay RayCluster to support multi-host training / inference with accelerators like TPUs. Kueue needs to be updated to check NumOfHosts and appropriately adjust podset assignments if NumOfHosts is greater than 1

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Account for NumOfHosts when calculating PodSet assignments for RayJob and RayCluster

netlify · 2024-10-31T03:11:59Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`e98217b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6723b5dfbdef940008b7db8f

mbobrovskyi · 2024-10-31T03:34:58Z

pkg/util/testingjobs/raycluster/wrappers.go

@@ -150,6 +150,11 @@ func (j *ClusterWrapper) WithWorkerPriorityClassName(value string) *ClusterWrapp
 	return j
 }

+func (j *ClusterWrapper) WithNumOfHosts(value int32) *ClusterWrapper {
+	j.Spec.WorkerGroupSpecs[0].NumOfHosts = value


Why "0" index?

It's just for testing and following the assumption in other places that the generated object only has 1 worker group

I don't like that we hardcoding this index. Maybe like this?

func (j *ClusterWrapper) WithNumOfHosts(groupName string, value int32) *ClusterWrapper { for index, group := range j.Spec.WorkerGroupSpecs { if group.GroupName == groupName { j.Spec.WorkerGroupSpecs[index].NumOfHosts = value } } return j }

mbobrovskyi · 2024-10-31T03:41:11Z

pkg/util/testingjobs/rayjob/wrappers.go

+func (j *JobWrapper) WithNumOfHosts(value int32) *JobWrapper {
+	j.Spec.RayClusterSpec.WorkerGroupSpecs[0].NumOfHosts = value
+	return j
+}
+


Maybe it's better to add it directly to rayv1.WorkerGroupSpec on the test case?

Ah, good catch

mbobrovskyi · 2024-10-31T03:48:02Z

/test pull-kueue-test-e2e-main-1-30 pull-kueue-test-e2e-main-1-31

Due to #3368.

mbobrovskyi · 2024-10-31T15:34:58Z

pkg/controller/jobs/raycluster/raycluster_controller.go

+			count = *wgs.Replicas
+		}
+		if wgs.NumOfHosts > 1 {
+			count = count * wgs.NumOfHosts


Suggested change

count = count * wgs.NumOfHosts

count *= wgs.NumOfHosts

mbobrovskyi · 2024-10-31T15:47:14Z

pkg/controller/jobs/rayjob/rayjob_controller.go

+			count = *wgs.Replicas
+		}
+		if wgs.NumOfHosts > 1 {
+			count = count * wgs.NumOfHosts


Suggested change

count = count * wgs.NumOfHosts

count *= wgs.NumOfHosts

mbobrovskyi · 2024-10-31T15:47:52Z

pkg/controller/jobs/rayjob/rayjob_controller_test.go

+					},
+					{
+						Name:     "group1",
+						Count:    4,


Suggested change

Count: 4,

Count: 1,

mbobrovskyi · 2024-10-31T15:48:20Z

pkg/controller/jobs/rayjob/rayjob_controller_test.go

+					},
+					{
+						Name:     "group2",
+						Count:    3,


Suggested change

Count: 3,

Count: 12,

andrewsykim · 2024-10-31T16:48:17Z

Thanks @mbobrovskyi, addressed your comments

mbobrovskyi · 2024-10-31T16:49:32Z

Thanks @mbobrovskyi, addressed your comments

Please check this one #3384 (comment).

andrewsykim · 2024-10-31T16:53:00Z

Please check this one #3384 (comment).

Thanks, I incorporated your suggestion

andrewsykim · 2024-10-31T16:55:57Z

Also FYI @tenzen-y @ryanaoleary @kevin85421

mbobrovskyi · 2024-10-31T16:56:47Z

/lgtm

Thanks!

k8s-ci-robot · 2024-10-31T16:56:54Z

LGTM label has been added.

Git tree hash: e09b5279525c032596f5ea5b99c7fcad5f1de22f

mbobrovskyi · 2024-10-31T16:57:17Z

/cc @mimowo @tenzen-y

tenzen-y

I would like to leave the final decision to @mimowo since there is no bandwidth.
My primary question is if this numOfHosts concept has a similar concept for the JobSet replicatedJobs[*].replicas.

Because the JobSet replicas have some limitations for TAS

mimowo · 2024-10-31T19:53:58Z

/approve
/cherry-pick release-0.8

k8s-infra-cherrypick-robot · 2024-10-31T19:54:01Z

@mimowo: once the present PR merges, I will cherry-pick it on top of release-0.8 in a new PR and assign it to you.

In response to this:

/approve
/cherry-pick release-0.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2024-10-31T19:54:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, mimowo

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:

~~pkg/controller/OWNERS~~ [mimowo]
~~pkg/util/OWNERS~~ [mimowo]

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

mbobrovskyi · 2024-10-31T20:10:09Z

/retest

Due to #3406.

k8s-infra-cherrypick-robot · 2024-10-31T20:22:04Z

@mimowo: #3384 failed to apply on top of branch "release-0.8":

Applying: TAS: Support RayCluster. (#3386)
Using index info to reconstruct a base tree...
M	pkg/controller/jobs/raycluster/raycluster_controller.go
M	pkg/controller/jobs/raycluster/raycluster_controller_test.go
M	pkg/controller/jobs/rayjob/rayjob_controller.go
M	pkg/controller/jobs/rayjob/rayjob_controller_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/controller/jobs/rayjob/rayjob_controller_test.go
Auto-merging pkg/controller/jobs/rayjob/rayjob_controller.go
CONFLICT (content): Merge conflict in pkg/controller/jobs/rayjob/rayjob_controller.go
Auto-merging pkg/controller/jobs/raycluster/raycluster_controller_test.go
Auto-merging pkg/controller/jobs/raycluster/raycluster_controller.go
CONFLICT (content): Merge conflict in pkg/controller/jobs/raycluster/raycluster_controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 TAS: Support RayCluster. (#3386)

In response to this:

/approve
/cherry-pick release-0.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

andrewsykim · 2024-11-01T04:00:55Z

The merge conflict is really odd.. also no idea why Github is considering this PR and TAS support for RayCluster as one commit:

andrewsykim · 2024-11-01T04:04:41Z

ugh. this is my bad, I must have messed up the commit when I ammended it

andrewsykim · 2024-11-01T04:09:38Z

manually opened the cherry-pick PR with the conflicts resolved #3408

tenzen-y · 2024-11-01T07:57:43Z

ugh. this is my bad, I must have messed up the commit when I ammended it

I think that It's not bad since users can obtain proper updated information from Release Note :)

tenzen-y · 2024-11-04T13:46:29Z

/release-note-edit

Account for NumOfHosts when calculating PodSet assignments for RayJob and RayCluster

andrewsykim · 2024-11-05T00:05:37Z

@tenzen-y looks like the v0.8.2 tag never included this change https://github.com/kubernetes-sigs/kueue/commits/v0.8.2

Despite it being included in release-0.8: https://github.com/kubernetes-sigs/kueue/commits/release-0.8

tenzen-y · 2024-11-05T00:17:11Z

@tenzen-y looks like the v0.8.2 tag never included this change https://github.com/kubernetes-sigs/kueue/commits/v0.8.2

Oh, it looks like my bad. Let us release v0.8.3 for the fix.
Thank you for reporting this!

Co-authored-by: Mykhailo Bobrovskyi <[email protected]>

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Oct 31, 2024

k8s-ci-robot requested review from denkensk and mbobrovskyi October 31, 2024 03:11

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 31, 2024

andrewsykim changed the title ~~[KubeRay] Support NumOfHosts when calculating PodSet assignments for KubeRay jobs~~ [KubeRay] Support NumOfHosts when calculating PodSet assignments Oct 31, 2024

mbobrovskyi reviewed Oct 31, 2024

View reviewed changes

mbobrovskyi mentioned this pull request Oct 31, 2024

Flaky E2E Test: Pod groups when Single CQ should admit group that fits #3368

Closed

andrewsykim mentioned this pull request Oct 31, 2024

Release v0.8.2 #3371

Closed

34 tasks

andrewsykim force-pushed the kuberay-num-of-hosts branch 2 times, most recently from 8a12ceb to eebd0b4 Compare October 31, 2024 15:12

mbobrovskyi reviewed Oct 31, 2024

View reviewed changes

andrewsykim force-pushed the kuberay-num-of-hosts branch from eebd0b4 to d14ebd4 Compare October 31, 2024 16:19

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

andrewsykim force-pushed the kuberay-num-of-hosts branch 3 times, most recently from 9d46183 to 86ac93e Compare October 31, 2024 16:23

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

TAS: Support RayCluster. (kubernetes-sigs#3386)

e98217b

andrewsykim force-pushed the kuberay-num-of-hosts branch from 86ac93e to e98217b Compare October 31, 2024 16:52

k8s-ci-robot assigned mbobrovskyi Oct 31, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2024

k8s-ci-robot requested review from mimowo and tenzen-y October 31, 2024 16:57

tenzen-y reviewed Oct 31, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2024

k8s-ci-robot merged commit 6047afe into kubernetes-sigs:main Oct 31, 2024
16 checks passed

k8s-ci-robot added this to the v0.9 milestone Oct 31, 2024

andrewsykim mentioned this pull request Nov 1, 2024

[release-0.8][KubeRay] Support NumOfHosts when calculating PodSet assignments (#3386) #3408

Merged

tenzen-y mentioned this pull request Nov 5, 2024

Release v0.8.3 #3441

Closed

36 tasks

mimowo mentioned this pull request Nov 5, 2024

Release v0.9.0 #3302

Closed

34 tasks

kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024

TAS: Support RayCluster. (kubernetes-sigs#3386) (kubernetes-sigs#3384)

44ed4b9

Co-authored-by: Mykhailo Bobrovskyi <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KubeRay] Support NumOfHosts when calculating PodSet assignments #3384

[KubeRay] Support NumOfHosts when calculating PodSet assignments #3384

andrewsykim commented Oct 31, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented Oct 31, 2024 •

edited

Loading

mbobrovskyi Oct 31, 2024

andrewsykim Oct 31, 2024

mbobrovskyi Oct 31, 2024 •

edited

Loading

mbobrovskyi Oct 31, 2024 •

edited

Loading

andrewsykim Oct 31, 2024

mbobrovskyi commented Oct 31, 2024

mbobrovskyi Oct 31, 2024

mbobrovskyi Oct 31, 2024

mbobrovskyi Oct 31, 2024

mbobrovskyi Oct 31, 2024

andrewsykim commented Oct 31, 2024

mbobrovskyi commented Oct 31, 2024

andrewsykim commented Oct 31, 2024

andrewsykim commented Oct 31, 2024

mbobrovskyi commented Oct 31, 2024

k8s-ci-robot commented Oct 31, 2024

mbobrovskyi commented Oct 31, 2024

tenzen-y left a comment

mimowo commented Oct 31, 2024

k8s-infra-cherrypick-robot commented Oct 31, 2024

k8s-ci-robot commented Oct 31, 2024

mbobrovskyi commented Oct 31, 2024

k8s-infra-cherrypick-robot commented Oct 31, 2024

andrewsykim commented Nov 1, 2024

andrewsykim commented Nov 1, 2024

andrewsykim commented Nov 1, 2024

tenzen-y commented Nov 1, 2024

tenzen-y commented Nov 4, 2024

andrewsykim commented Nov 5, 2024

tenzen-y commented Nov 5, 2024

[KubeRay] Support NumOfHosts when calculating PodSet assignments #3384

[KubeRay] Support NumOfHosts when calculating PodSet assignments #3384

Conversation

andrewsykim commented Oct 31, 2024 • 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 Oct 31, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

mbobrovskyi Oct 31, 2024

Choose a reason for hiding this comment

andrewsykim Oct 31, 2024

Choose a reason for hiding this comment

mbobrovskyi Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

mbobrovskyi Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

andrewsykim Oct 31, 2024

Choose a reason for hiding this comment

mbobrovskyi commented Oct 31, 2024

mbobrovskyi Oct 31, 2024

Choose a reason for hiding this comment

mbobrovskyi Oct 31, 2024

Choose a reason for hiding this comment

mbobrovskyi Oct 31, 2024

Choose a reason for hiding this comment

mbobrovskyi Oct 31, 2024

Choose a reason for hiding this comment

andrewsykim commented Oct 31, 2024

mbobrovskyi commented Oct 31, 2024

andrewsykim commented Oct 31, 2024

andrewsykim commented Oct 31, 2024

mbobrovskyi commented Oct 31, 2024

k8s-ci-robot commented Oct 31, 2024

mbobrovskyi commented Oct 31, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

mimowo commented Oct 31, 2024

k8s-infra-cherrypick-robot commented Oct 31, 2024

k8s-ci-robot commented Oct 31, 2024

mbobrovskyi commented Oct 31, 2024

k8s-infra-cherrypick-robot commented Oct 31, 2024

andrewsykim commented Nov 1, 2024

andrewsykim commented Nov 1, 2024

andrewsykim commented Nov 1, 2024

tenzen-y commented Nov 1, 2024

tenzen-y commented Nov 4, 2024

andrewsykim commented Nov 5, 2024

tenzen-y commented Nov 5, 2024

andrewsykim commented Oct 31, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented Oct 31, 2024 •

edited

Loading

mbobrovskyi Oct 31, 2024 •

edited

Loading

mbobrovskyi Oct 31, 2024 •

edited

Loading