test(runtime): add UT for jobset runtime valid function. #2562

Harshal292004 · 2025-03-23T10:16:52Z

What this PR does / why we need it:
Add fine grained UT for jobset runtime Validate function.

Which issue(s) this PR fixes
Fixes #2556

Checklist:

Docs included if any changes are user facing

Harshal292004 · 2025-03-23T10:18:07Z

@tenzen-y Could you please look into this PR fixing #2556 Jobset UTs

IRONICBo · 2025-03-25T08:42:45Z

/ok-to-test
/rerun-all

IRONICBo

Other LGTM, Thanks for your contribution.

cc @tenzen-y

IRONICBo · 2025-03-25T08:46:43Z

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

+			if err != nil {
+				t.Fatalf("Failed to initialize JobSet plugin: %v", err)
+			}
+			warnings, errs := p.(framework.CustomValidationPlugin).Validate(tc.runtimeJobTemplate, nil, tc.oldObj, tc.newObj)


Upstream has update the Validate functions, you can rebase your commit to main and remove fist params here.

IRONICBo · 2025-03-25T08:49:15Z

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

+			},
+		},
+		"model intializer job exists but container missing ": {
+			runtimeJobTemplate: func() client.Object {


Ditto, you can remove it because we don't need runtimeJobTemplate now.

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

Line 82 in 4e6199c

func (j *JobSet) Validate(info *runtime.Info, _, newObj *trainer.TrainJob) (admission.Warnings, field.ErrorList) {

IRONICBo · 2025-03-25T08:49:20Z

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

+		},
+		// assert that we get a error  here
+		"dataset initializer job exists but container missing": {
+			runtimeJobTemplate: func() client.Object {


Ditto, you can remove it.

IRONICBo · 2025-03-25T08:49:44Z

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

+			},
+		},
+		"no model intializer job": {
+			runtimeJobTemplate: utiltesting.MakeJobSetWrapper(metav1.NamespaceDefault, "test"),


Ditto, you can remove it.

IRONICBo · 2025-03-25T08:49:54Z

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

+				Obj(),
+		},
+		"no dataset intializer job": {
+			runtimeJobTemplate: utiltesting.MakeJobSetWrapper(metav1.NamespaceDefault, "test"),


Ditto, you can remove it.

IRONICBo · 2025-03-25T08:50:48Z

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

+				Obj(),
+		},
+		"no initializer job": {
+			runtimeJobTemplate: utiltesting.MakeJobSetWrapper(metav1.NamespaceDefault, "test"),


Ditto, you can remove it.

IRONICBo · 2025-03-25T08:50:50Z

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

+		wantWarnings       admission.Warnings
+	}{
+		"no jobset runtime template": {
+			runtimeJobTemplate: &utiltesting.JobSetWrapper{},


Ditto, you can remove it.

Harshal292004 · 2025-03-25T15:33:38Z

@IRONICBo I updated the entire test . Can you have a look

Harshal292004 · 2025-03-26T02:27:13Z

@tenzen-y I have removed the test case where runtimeInfo is nil it's causing panic when accessing ReplicatedJobs from jobSetSpec. Other test cases are passing successfully

Harshal292004 · 2025-03-26T02:27:47Z

cc @IRONICBo @Electronic-Waste @jinchihe

IRONICBo · 2025-03-26T03:22:25Z

/rerun-all

coveralls · 2025-03-26T03:27:11Z

Pull Request Test Coverage Report for Build 14470201388

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.9%) to 67.356%

Totals
Change from base Build 14412786058:	0.9%
Covered Lines:	1758
Relevant Lines:	2610

💛 - Coveralls

Harshal292004 · 2025-03-26T04:07:51Z

/rerun-all

Harshal292004 · 2025-03-26T04:08:38Z

@IRONICBo Can you please rerun the workflows there was a gci error in the last run

IRONICBo · 2025-03-26T08:30:54Z

/rerun-all

IRONICBo · 2025-03-26T08:31:05Z

@IRONICBo Can you please rerun the workflows there was a gci error in the last run

OK

IRONICBo · 2025-03-27T02:33:05Z

Thank you!
/lgtm

Harshal292004 · 2025-03-27T03:19:16Z

Hi @IRONICBo ,

I recently raised two PRs for Kubeflow Trainer, both of which passed all checks. One of them was reviewed by @tenzen-y , but it hasn’t been merged yet as no approvals or suggestions were provided by the reviewers.

Could you clarify if there’s a specific process for PRs to get merged? Should I wait for approval, or is there something I should do to get more reviewer attention?

IRONICBo · 2025-03-27T06:44:19Z

Hi @IRONICBo ,

I recently raised two PRs for Kubeflow Trainer, both of which passed all checks. One of them was reviewed by @tenzen-y , but it hasn’t been merged yet as no approvals or suggestions were provided by the reviewers.

Could you clarify if there’s a specific process for PRs to get merged? Should I wait for approval, or is there something I should do to get more reviewer attention?

I am not sure, and you may need to wait for other reviewers to process it, other reviews may be busy with other threads.

Harshal292004 · 2025-03-31T05:33:24Z

@tenzen-y Could you please review this PR ?

tenzen-y

Thank you
Basically, lgtm

I added a few nit comments.

tenzen-y · 2025-03-28T17:08:10Z

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

@@ -325,3 +329,211 @@ func TestJobSet(t *testing.T) {
 		})
 	}
 }
+
+func TestJobSetValidate(t *testing.T) {


Suggested change

func TestJobSetValidate(t *testing.T) {

func TestValidate(t *testing.T) {

In Go, we use Test prefix as a Test function name.

tenzen-y · 2025-04-15T12:19:53Z

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

+func TestJobSetValidate(t *testing.T) {
+	cases := map[string]struct {
+		info         *runtime.Info
+		_            *trainer.TrainJob


Suggested change

_ *trainer.TrainJob

This is unused field.

tenzen-y · 2025-04-15T12:22:15Z

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

+			if err != nil {
+				t.Fatalf("Failed to initialize JobSet plugin: %v", err)
+			}
+


Suggested change

tenzen-y · 2025-04-15T12:25:06Z

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

+				Initializer(&trainer.Initializer{Dataset: nil}).
+				Obj(),
+		},
+		"must have dataset initializer  job when trainJob is configured with input datasetConfig": {


Suggested change

"must have dataset initializer job when trainJob is configured with input datasetConfig": {

"must have dataset initializer job when trainJob is configured with input datasetConfig": {

Signed-off-by: Harshal292004 <malaniharshal95@gmail.com>

Harshal292004 · 2025-04-15T13:07:34Z

@tenzen-y Updated everything as suggested

Harshal292004 · 2025-04-15T13:09:22Z

@tenzen-y, I was wondering if there are any significant opportunities for contribution within Kubeflow at the moment? @Electronic-Waste is currently focused on KEP 2401, so there isn’t much for me to do in that area.

tenzen-y · 2025-04-15T13:24:00Z

@tenzen-y, I was wondering if there are any significant opportunities for contribution within Kubeflow at the moment? @Electronic-Waste is currently focused on KEP 2401, so there isn’t much for me to do in that area.

Every contribution is valuable. If you have an interest in any feature development, please contact the issue creator.

tenzen-y

Thank you
/lgtm
/approve

google-oss-prow · 2025-04-15T13:26:44Z

[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

google-oss-prow bot requested review from Electronic-Waste and jinchihe March 23, 2025 10:16

google-oss-prow bot added the size/L label Mar 23, 2025

IRONICBo suggested changes Mar 25, 2025

View reviewed changes

Harshal292004 force-pushed the jobset-validation branch from 7262c2e to 7714e17 Compare March 25, 2025 15:32

Harshal292004 requested a review from IRONICBo March 25, 2025 15:33

google-oss-prow bot assigned IRONICBo Mar 27, 2025

google-oss-prow bot added the lgtm label Mar 27, 2025

tenzen-y reviewed Apr 15, 2025

View reviewed changes

tenzen-y mentioned this pull request Apr 3, 2025

Implement UTs for Plugin CustomValidations #2556

Closed

3 tasks

Harshal292004 added 5 commits April 15, 2025 18:24

test(runtime): add UT for jobset runtime valid function.

4536c70

Signed-off-by: Harshal292004 <malaniharshal95@gmail.com>

Update the test cases

c4af1eb

Signed-off-by: Harshal292004 <malaniharshal95@gmail.com>

update the test cases

801df94

Signed-off-by: Harshal292004 <malaniharshal95@gmail.com>

format imports

dc4c53a

Signed-off-by: Harshal292004 <malaniharshal95@gmail.com>

Updated as suggested

ea3bcb5

Signed-off-by: Harshal292004 <malaniharshal95@gmail.com>

Harshal292004 force-pushed the jobset-validation branch from b1d6fc8 to ea3bcb5 Compare April 15, 2025 13:07

google-oss-prow bot removed the lgtm label Apr 15, 2025

tenzen-y reviewed Apr 15, 2025

View reviewed changes

google-oss-prow bot assigned tenzen-y Apr 15, 2025

google-oss-prow bot added the lgtm label Apr 15, 2025

google-oss-prow bot added the approved label Apr 15, 2025

google-oss-prow bot merged commit b9c0ee2 into kubeflow:master Apr 15, 2025
18 checks passed

google-oss-prow bot added this to the v2.0 milestone Apr 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(runtime): add UT for jobset runtime valid function. #2562

test(runtime): add UT for jobset runtime valid function. #2562

Harshal292004 commented Mar 23, 2025

Harshal292004 commented Mar 23, 2025

IRONICBo commented Mar 25, 2025 •

edited

Loading

IRONICBo left a comment •

edited

Loading

IRONICBo Mar 25, 2025

IRONICBo Mar 25, 2025

IRONICBo Mar 25, 2025

IRONICBo Mar 25, 2025 •

edited

Loading

IRONICBo Mar 25, 2025

IRONICBo Mar 25, 2025

IRONICBo Mar 25, 2025

Harshal292004 commented Mar 25, 2025

Harshal292004 commented Mar 26, 2025

Harshal292004 commented Mar 26, 2025

IRONICBo commented Mar 26, 2025

coveralls commented Mar 26, 2025 •

edited

Loading

Harshal292004 commented Mar 26, 2025

Harshal292004 commented Mar 26, 2025

IRONICBo commented Mar 26, 2025

IRONICBo commented Mar 26, 2025

IRONICBo commented Mar 27, 2025

Harshal292004 commented Mar 27, 2025

IRONICBo commented Mar 27, 2025 •

edited

Loading

Harshal292004 commented Mar 31, 2025

tenzen-y left a comment

tenzen-y Mar 28, 2025

tenzen-y Apr 15, 2025

tenzen-y Apr 15, 2025

tenzen-y Apr 15, 2025

Harshal292004 commented Apr 15, 2025

Harshal292004 commented Apr 15, 2025

tenzen-y commented Apr 15, 2025

tenzen-y left a comment

google-oss-prow bot commented Apr 15, 2025

	func TestJobSetValidate(t *testing.T) {
	func TestValidate(t *testing.T) {

	"must have dataset initializer job when trainJob is configured with input datasetConfig": {
	"must have dataset initializer job when trainJob is configured with input datasetConfig": {

test(runtime): add UT for jobset runtime valid function. #2562

test(runtime): add UT for jobset runtime valid function. #2562

Conversation

Harshal292004 commented Mar 23, 2025

Harshal292004 commented Mar 23, 2025

IRONICBo commented Mar 25, 2025 • edited Loading

IRONICBo left a comment • 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

IRONICBo Mar 25, 2025 • 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

Harshal292004 commented Mar 25, 2025

Harshal292004 commented Mar 26, 2025

Harshal292004 commented Mar 26, 2025

IRONICBo commented Mar 26, 2025

coveralls commented Mar 26, 2025 • edited Loading

Pull Request Test Coverage Report for Build 14470201388

Details

💛 - Coveralls

Harshal292004 commented Mar 26, 2025

Harshal292004 commented Mar 26, 2025

IRONICBo commented Mar 26, 2025

IRONICBo commented Mar 26, 2025

IRONICBo commented Mar 27, 2025

Harshal292004 commented Mar 27, 2025

IRONICBo commented Mar 27, 2025 • edited Loading

Harshal292004 commented Mar 31, 2025

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

Choose a reason for hiding this comment

Harshal292004 commented Apr 15, 2025

Harshal292004 commented Apr 15, 2025

tenzen-y commented Apr 15, 2025

tenzen-y left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Apr 15, 2025

IRONICBo commented Mar 25, 2025 •

edited

Loading

IRONICBo left a comment •

edited

Loading

IRONICBo Mar 25, 2025 •

edited

Loading

coveralls commented Mar 26, 2025 •

edited

Loading

IRONICBo commented Mar 27, 2025 •

edited

Loading