Add initial Kueue integration #313

tedhtchang · 2024-10-09T00:15:35Z

This PR is to optionally enable Kueue for LMEvalJob by implementing the Kueue's GenericJob interface. It still needs some other functionalities so I want to broke it down a little bit:

1. Kueue requires LMEvalJob to have Spec.Suspend. We need to handle these 3 suspend cases in the LMEvalJob reconciler:

Spec.Suspend(=true) upon LMEvalJob creation. This means the job is created but pods are running.
Spec.Suspend(=true) while the LMEvalJob is running. This means delete the pods owned by the job.
Spec.Suspend(=false) while the LMEvalJob is in suspended state. This means re-create the pods owned by the job.
See PR#317

2. Get the PodSpec from the LMEValJob. Currently PodSpec is not stored in the LMEvalJob object.

Kueue needs to admit the job as a kueue.x-k8s.io/v1beta1/Workload. So the Workload can represent the resources (cpu/memory) requirements for the job.
We need the PodSpec from the LMEvalJob object to create a PodSet for Workload. See PR#323

3. Node Affinity

When we want to run job on particular node. We can create a spec.nodeLabels in ResourceFlavor which can be referenced in a ClusterQueue.

apiVersion: kueue.x-k8s.io/v1beta1
kind: ResourceFlavor
metadata:
  name: "default-flavor"
spec:
  nodeLabels:
    kubernetes.io/hostname: kueue-worker

Implement these runWithPodInfo and restoreWithPodInfo.
Extract nodeSelector object from from the PodSetInfo object and then overwrite the LMevalJob PodSpec spec affinity. @yhwang created another PR to introduce Pod.Spec.NodeAffinity into LMevalJob.

openshift-ci · 2024-10-09T00:15:47Z

Hi @tedhtchang. Thanks for your PR.

I'm waiting for a trustyai-explainability member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

ruivieira · 2024-10-23T16:39:17Z

Hi @tedhtchang this is looking great, thanks!

Just a question, we cannot guarantee that Kueue is installed on the cluster. Would enabling Kueue support for LM-Eval (and not having Kueue installed) be a problem?

tedhtchang · 2024-10-23T17:08:43Z

@ruivieira @yhwang Can you guys change the PR to merge to main branch Or I can close & open a new PR?(done)
Thanks @yhwang

tedhtchang · 2024-10-23T18:26:42Z

Hi @tedhtchang this is looking great, thanks!

Just a question, we cannot guarantee that Kueue is installed on the cluster. Would enabling Kueue support for LM-Eval (and not having Kueue installed) be a problem?

@ruivieira We can start the operator without the job_mgr_controller if Kueue is not enabled. It shouldn't be a problem.

tedhtchang · 2024-10-24T09:37:48Z

/cc @ruivieira @yhwang Use the controllers/job_mgr/README.md to setup env and test this PR.

github-actions · 2024-10-24T10:06:43Z

PR image build and manifest generation completed successfully!

📦 PR image: quay.io/trustyai/trustyai-service-operator-ci:605323ebc1f1eb567a11e60671a3a09a1767b0df

📦 LMES driver image: quay.io/trustyai/ta-lmes-driver:605323ebc1f1eb567a11e60671a3a09a1767b0df

📦 LMES job image: quay.io/trustyai/ta-lmes-job:605323ebc1f1eb567a11e60671a3a09a1767b0df

🗂️ CI manifests

devFlags:
  manifests:
    - contextDir: config
      sourcePath: ''
      uri: https://api.github.com/repos/trustyai-explainability/trustyai-service-operator-ci/tarball/operator-605323ebc1f1eb567a11e60671a3a09a1767b0df

yhwang · 2024-10-24T17:00:17Z

cmd/main.go

@@ -69,7 +72,7 @@ func main() {
 	var probeAddr string
 	var configMap string
 	var enabledServices controllers.EnabledServices
-	flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
+	flag.StringVar(&metricsAddr, "metrics-bind-address", ":9443", "The address and port the metric endpoint binds to.")


Switching the default value here to 9443 may conflict with the webhook port although we don't have a webhook server yet. No cert for running 9443 at this moment. And the metric port is not exposed either.

yhwang · 2024-10-24T17:05:19Z

controllers/job_mgr/README.md

+        kServeServerless: disabled
+        lmes-default-batch-size: "8"
+        lmes-driver-image: quay.io/yhwang/ta-lmes-driver:latest
+        lmes-grpc-port: "8082"


nit: no more gRPC settings. this line and the following line.

yhwang · 2024-10-24T17:07:03Z

controllers/job_mgr/constants.go

@@ -0,0 +1,5 @@
+package job_mgr


missing the license header.

ruivieira · 2024-10-30T09:06:37Z

Hi @tedhtchang , is this PR rebased on main? If not, is it ok to rebase it? Thank you!

tedhtchang · 2024-10-30T19:17:33Z

Hi @tedhtchang , is this PR rebased on main? If not, is it ok to rebase it? Thank you!

@ruivieira You should merge @yhwang's PR first because my PR was based on it. Once you merged it the main, I will rebased the main into my PR. Then you should be able to merged mine.

ruivieira · 2024-11-01T08:50:42Z

@tedhtchang ack, thanks for the heads up. I've approved #340 now.

openshift-ci · 2024-11-05T00:32:44Z

New changes are detected. LGTM label has been removed.

tedhtchang · 2024-11-05T00:40:35Z

@ruivieira @yhwang I rebased and resolved conflicts from main and update-lmevaljob-pod branches.

tedhtchang · 2024-11-07T01:58:37Z

controllers/tas/statuses_test.go

+		k8sClient = fake.NewClientBuilder().WithScheme(scheme.Scheme).WithStatusSubresource(
+			&routev1.Route{},
+			&trustyaiopendatahubiov1alpha1.TrustyAIService{},


Need to register the status subresoruce separately with the updated controller-runtime package.
Thanks @yhwang figured this out.

tedhtchang · 2024-11-07T02:13:43Z

@ruivieira The controller-test failure was fix. Not sure about the smoke test error. Does it always fail ? Nvm, missing service secrets which are generated by Openshift. KinD would not generate those secrets. I try to create these secrets in this PR to pass the smoke test.

ruivieira

@tedhtchang LGTM, thanks for the changes!

I do have a final request: is it possible to regenerate the go.sum?
I'm getting a checksum mismatch for github.com/kserve/[email protected] dependency.

openshift-ci · 2024-11-08T09:04:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yhwang

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

Signed-off-by: ted chang <[email protected]>

tedhtchang · 2024-11-08T17:47:07Z

@ruivieira I saw no difference in go.sum after go mod tidy. I rebase PR anyway.
however @yhwang found mismatched go.sum in the main branch. This is the output:

go mod tidy
go: downloading golang.org/x/mod v0.12.0
go: downloading github.com/grpc-ecosystem/grpc-gateway/v2 v2.15.2

tedhtchang · 2024-11-08T18:01:51Z

/retest

openshift-ci · 2024-11-08T18:35:02Z

@tedhtchang: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/trustyai-service-operator-e2e	`605323e`	link	true	`/test trustyai-service-operator-e2e`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci bot added do-not-merge/work-in-progress needs-ok-to-test labels Oct 9, 2024

tedhtchang force-pushed the optional-kueue-integration branch 3 times, most recently from 24ae784 to ffe34a3 Compare October 9, 2024 23:11

openshift-merge-robot added the needs-rebase label Oct 14, 2024

tedhtchang force-pushed the optional-kueue-integration branch from ffe34a3 to b087db4 Compare October 17, 2024 22:39

openshift-merge-robot removed the needs-rebase label Oct 17, 2024

yhwang added the lm-eval Issues related to LM-Eval label Oct 18, 2024

tedhtchang force-pushed the optional-kueue-integration branch from b087db4 to 80ad816 Compare October 18, 2024 20:44

openshift-merge-robot added the needs-rebase label Oct 19, 2024

tedhtchang changed the base branch from dev/lm-eval to main October 23, 2024 18:07

tedhtchang force-pushed the optional-kueue-integration branch from 80ad816 to 6639b9b Compare October 23, 2024 19:52

openshift-merge-robot removed the needs-rebase label Oct 23, 2024

tedhtchang force-pushed the optional-kueue-integration branch from 6639b9b to 2003c61 Compare October 24, 2024 09:34

tedhtchang marked this pull request as ready for review October 24, 2024 09:36

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 24, 2024

ruivieira added the ok-to-test label Oct 24, 2024

ruivieira requested review from danielezonca, ruivieira and yhwang October 24, 2024 09:57

yhwang reviewed Oct 24, 2024

View reviewed changes

openshift-ci bot assigned yhwang Oct 26, 2024

openshift-ci bot added the lgtm label Oct 26, 2024

openshift-merge-robot added the needs-rebase label Oct 30, 2024

ruivieira approved these changes Nov 1, 2024

View reviewed changes

openshift-ci bot assigned ruivieira Nov 1, 2024

tedhtchang force-pushed the optional-kueue-integration branch from c94e081 to 78cbad0 Compare November 5, 2024 00:32

openshift-ci bot removed the lgtm label Nov 5, 2024

openshift-merge-robot removed the needs-rebase label Nov 5, 2024

openshift-merge-robot added the needs-rebase label Nov 5, 2024

tedhtchang force-pushed the optional-kueue-integration branch from 78cbad0 to f80c1b8 Compare November 5, 2024 18:20

openshift-merge-robot removed the needs-rebase label Nov 5, 2024

tedhtchang force-pushed the optional-kueue-integration branch from f80c1b8 to 353cf02 Compare November 7, 2024 01:49

tedhtchang commented Nov 7, 2024

View reviewed changes

ruivieira requested changes Nov 8, 2024

View reviewed changes

tedhtchang added 4 commits November 8, 2024 09:27

Add initial Kueue integration

b1100b2

Signed-off-by: ted chang <[email protected]>

implement genericjob interface

cb5e11f

Signed-off-by: ted chang <[email protected]>

Address comments

fc44ba3

Signed-off-by: ted chang <[email protected]>

Rebase and resolve conflict with main and update-lmevaljob-pod branches

605323e

Signed-off-by: ted chang <[email protected]>

tedhtchang force-pushed the optional-kueue-integration branch from 353cf02 to 605323e Compare November 8, 2024 17:38

ruivieira merged commit 02de274 into trustyai-explainability:main Nov 11, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial Kueue integration #313

Add initial Kueue integration #313

tedhtchang commented Oct 9, 2024 •

edited

Loading

openshift-ci bot commented Oct 9, 2024

ruivieira commented Oct 23, 2024

tedhtchang commented Oct 23, 2024 •

edited

Loading

tedhtchang commented Oct 23, 2024

tedhtchang commented Oct 24, 2024

github-actions bot commented Oct 24, 2024 •

edited

Loading

yhwang Oct 24, 2024 •

edited

Loading

yhwang Oct 24, 2024 •

edited

Loading

yhwang Oct 24, 2024

ruivieira commented Oct 30, 2024

tedhtchang commented Oct 30, 2024 •

edited

Loading

ruivieira commented Nov 1, 2024

openshift-ci bot commented Nov 5, 2024

tedhtchang commented Nov 5, 2024

tedhtchang Nov 7, 2024

tedhtchang commented Nov 7, 2024 •

edited

Loading

ruivieira left a comment

openshift-ci bot commented Nov 8, 2024

tedhtchang commented Nov 8, 2024

tedhtchang commented Nov 8, 2024

openshift-ci bot commented Nov 8, 2024

Add initial Kueue integration #313

Add initial Kueue integration #313

Conversation

tedhtchang commented Oct 9, 2024 • edited Loading

1. Kueue requires LMEvalJob to have Spec.Suspend. We need to handle these 3 suspend cases in the LMEvalJob reconciler:

2. Get the PodSpec from the LMEValJob. Currently PodSpec is not stored in the LMEvalJob object.

3. Node Affinity

openshift-ci bot commented Oct 9, 2024

ruivieira commented Oct 23, 2024

tedhtchang commented Oct 23, 2024 • edited Loading

tedhtchang commented Oct 23, 2024

tedhtchang commented Oct 24, 2024

github-actions bot commented Oct 24, 2024 • edited Loading

yhwang Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

yhwang Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

yhwang Oct 24, 2024

Choose a reason for hiding this comment

ruivieira commented Oct 30, 2024

tedhtchang commented Oct 30, 2024 • edited Loading

ruivieira commented Nov 1, 2024

openshift-ci bot commented Nov 5, 2024

tedhtchang commented Nov 5, 2024

tedhtchang Nov 7, 2024

Choose a reason for hiding this comment

tedhtchang commented Nov 7, 2024 • edited Loading

ruivieira left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 8, 2024

tedhtchang commented Nov 8, 2024

tedhtchang commented Nov 8, 2024

openshift-ci bot commented Nov 8, 2024

tedhtchang commented Oct 9, 2024 •

edited

Loading

tedhtchang commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024 •

edited

Loading

yhwang Oct 24, 2024 •

edited

Loading

yhwang Oct 24, 2024 •

edited

Loading

tedhtchang commented Oct 30, 2024 •

edited

Loading

tedhtchang commented Nov 7, 2024 •

edited

Loading