KEP-2170: Implement Initializer builders in the JobSet plugin #2316

andreyvelich · 2024-11-01T01:03:47Z

I implemented builder for dataset and model initializers.

/assign @kubeflow/wg-training-leads @varshaprasad96 @akshaychitneni @deepanker13 @helenxie-bit @Electronic-Waste @saileshd1402 @kannon92 @kuizhiqing @shravan-achar

Signed-off-by: Andrey Velichkevich <[email protected]>

andreyvelich · 2024-11-01T01:05:03Z

pkg/apis/kubeflow.org/v2alpha1/trainjob_types.go

-	SecretRef *corev1.SecretReference `json:"secretRef,omitempty"`
+	// Reference to the secret with credentials to download dataset.
+	// Secret must be created in the TrainJob's namespace.
+	SecretRef *corev1.LocalObjectReference `json:"secretRef,omitempty"`


@tenzen-y I updated the API to use *corev1.LocalObjectReference type for the SecretRef.
I think, we should use this since we don't want to support cross-namespace reference for this secret.

andreyvelich · 2024-11-01T01:07:59Z

pkg/constants/constants.go

+	// TODO (andreyvelich): Add validation to check that initializer ReplicatedJob has the following volumes.
+	VolumeMountModelInitializer = corev1.VolumeMount{
+		Name:      VolumeNameInitializer,
+		MountPath: "/workspace/model",
+	}
+
+	// VolumeMountModelInitializer is the volume mount for the dataset initializer container.
+	VolumeMountDatasetInitializer = corev1.VolumeMount{
+		Name:      VolumeNameInitializer,
+		MountPath: "/workspace/dataset",
+	}
+
+	// VolumeInitializer is the volume for the initializer ReplicatedJob.
+	// TODO (andreyvelich): We should make VolumeSource configurable.
+	VolumeInitializer = corev1.Volume{


@akshaychitneni @tenzen-y I think, we should validate that runtime has the correct configuration for the volume if it has the Initializer ReplicatedJob.
I added these consts in this PR, so we can re-use it in the Validation PR.
Otherwise, users will get many errors while using our custom Storage Initializers.

coveralls · 2024-11-01T01:08:39Z

Pull Request Test Coverage Report for Build 11636022716

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 11618065644:	0.0%
Covered Lines:	77
Relevant Lines:	77

💛 - Coveralls

Signed-off-by: Andrey Velichkevich <[email protected]>

tenzen-y

Thank you.

tenzen-y · 2024-11-01T16:15:27Z

Makefile

@@ -47,7 +47,7 @@ manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and Cust
 		output:rbac:artifacts:config=manifests/v2/base/rbac \
 		output:webhook:artifacts:config=manifests/v2/base/webhook

-generate: controller-gen ## Generate apidoc, sdk and code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.
+generate: controller-gen manifests ## Generate apidoc, sdk and code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.


Could you remove this?

training-operator/.github/workflows/test-go.yaml

Lines 32 to 35 in 7c5ea70

- name: Check manifests

run: |

make manifests && git add manifests &&

git diff --cached --exit-code || (echo 'Please run "make manifests" to generate manifests' && exit 1);

@tenzen-y Oh, you want to separate it.
Don't we want to include manifests command into generate script ?

I wanted to say removing "Check manifests" from workflow because we will perform manifests verification with make generate since this PR.

Oh, you are right!

pkg/constants/constants.go

tenzen-y · 2024-11-01T16:37:35Z

pkg/constants/constants.go

+	// This is the temporary container that we use in the initializer ReplicatedJob.
+	// TODO (andreyvelich): Once JobSet supports execution policy, we can remove it.
+	// TODO (andreyvelich): Add validation to check that initializer ReplicatedJob has this container.
+	ContainerBusyBox corev1.Container = corev1.Container{


It seems that this is only for the testing wrappers.
In that case, we should not expose this as a global constants. Instead of this, could you move this to util.v2/testing/constants.go?

As I mentioned here, I think we should use these consts for Webhook validation: #2316 (comment)

I see. In that case, could we move these default values into the JobSet plugins since these are semantically tied into the JobSet, then we can reuse these in the JobSet CustomValidationPlugin and ComponentBuilderPlugin?

Sure, that makes sense!

tenzen-y · 2024-11-01T16:38:02Z

pkg/constants/constants.go

+
+	// VolumeMountModelInitializer is the volume mount for the model initializer container.
+	// TODO (andreyvelich): Add validation to check that initializer ReplicatedJob has the following volumes.
+	VolumeMountModelInitializer = corev1.VolumeMount{


tenzen-y · 2024-11-01T16:38:59Z

pkg/constants/constants.go

+	}
+
+	// VolumeMountModelInitializer is the volume mount for the dataset initializer container.
+	VolumeMountDatasetInitializer = corev1.VolumeMount{


tenzen-y · 2024-11-01T16:43:09Z

pkg/runtime.v2/framework/plugins/jobset/builder.go

@@ -51,6 +52,90 @@ func NewBuilder(objectKey client.ObjectKey, jobSetTemplateSpec kubeflowv2.JobSet
 	}
 }

+// mergeInitializerEnvs merges the TrainJob and Runtime Pod envs.
+func mergeInitializerEnvs(storageUri *string, trainJobEnvs, containerEnv []corev1.EnvVar) []corev1.EnvVar {


Suggested change

func mergeInitializerEnvs(storageUri *string, trainJobEnvs, containerEnv []corev1.EnvVar) []corev1.EnvVar {

func mergeInitializerEnvs(storageUri string, trainJobEnvs, containerEnv []corev1.EnvVar) []corev1.EnvVar {

It seems that this pointer is less valuable.

Actually, I use this pointer here to verify if StorageUri is set:

training-operator/pkg/runtime.v2/framework/plugins/jobset/builder.go

Line 60 in e9f017c

if storageUri != nil {

That also allow us to not use ptr.Deref() here: #2316 (comment)

Does it make sense ?

Ah, I was just missed the line. That makes sense. Thanks.

pkg/runtime.v2/framework/plugins/jobset/builder.go

tenzen-y · 2024-11-01T16:45:01Z

pkg/runtime.v2/framework/plugins/jobset/builder.go

+// mergeInitializerEnvs merges the TrainJob and Runtime Pod envs.
+func mergeInitializerEnvs(storageUri *string, trainJobEnvs, containerEnv []corev1.EnvVar) []corev1.EnvVar {
+	envNames := sets.New[string]()
+	envs := []corev1.EnvVar{}


Suggested change

envs := []corev1.EnvVar{}

var envs []corev1.EnvVar

Avoiding unneeded malloc would be better.

pkg/runtime.v2/framework/plugins/jobset/builder.go

Co-authored-by: Yuki Iwai <[email protected]> Signed-off-by: Andrey Velichkevich <[email protected]>

Signed-off-by: Andrey Velichkevich <[email protected]>

tenzen-y

Thank you for this contribution!
/lgtm
/approve

/hold for CI

google-oss-prow · 2024-11-01T20:41:51Z

[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 · 2024-11-01T21:06:22Z

/hold cancel

andreyvelich added 3 commits November 1, 2024 00:08

KEP-2170: Implement Initializer builder in the JobSet plugin

8e670ef

Signed-off-by: Andrey Velichkevich <[email protected]>

Update the SDK models

ba9e215

Signed-off-by: Andrey Velichkevich <[email protected]>

Remove Info from Initializer builder

58d563d

Signed-off-by: Andrey Velichkevich <[email protected]>

google-oss-prow bot requested review from jinchihe and kuizhiqing November 1, 2024 01:03

google-oss-prow bot added the size/XL label Nov 1, 2024

andreyvelich commented Nov 1, 2024

View reviewed changes

Update manifests

e9f017c

Signed-off-by: Andrey Velichkevich <[email protected]>

tenzen-y reviewed Nov 1, 2024

View reviewed changes

andreyvelich and others added 4 commits November 1, 2024 17:20

Update pkg/constants/constants.go

1e14800

Co-authored-by: Yuki Iwai <[email protected]> Signed-off-by: Andrey Velichkevich <[email protected]>

Use var for envs

b9f9e8a

Signed-off-by: Andrey Velichkevich <[email protected]>

Remove check manifests from GitHub actions

c35b8d1

Signed-off-by: Andrey Velichkevich <[email protected]>

Move consts to JobSet plugin

cd0bc23

Signed-off-by: Andrey Velichkevich <[email protected]>

tenzen-y reviewed Nov 1, 2024

View reviewed changes

google-oss-prow bot added the do-not-merge/hold label Nov 1, 2024

google-oss-prow bot assigned tenzen-y Nov 1, 2024

google-oss-prow bot added the lgtm label Nov 1, 2024

google-oss-prow bot added the approved label Nov 1, 2024

google-oss-prow bot removed the do-not-merge/hold label Nov 1, 2024

google-oss-prow bot merged commit 2e1e125 into kubeflow:master Nov 1, 2024
43 checks passed

andreyvelich deleted the issue-2290-init-builder branch November 2, 2024 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2170: Implement Initializer builders in the JobSet plugin #2316

KEP-2170: Implement Initializer builders in the JobSet plugin #2316

andreyvelich commented Nov 1, 2024

andreyvelich Nov 1, 2024

andreyvelich Nov 1, 2024

coveralls commented Nov 1, 2024 •

edited

Loading

tenzen-y left a comment

tenzen-y Nov 1, 2024

andreyvelich Nov 1, 2024

tenzen-y Nov 1, 2024

andreyvelich Nov 1, 2024

tenzen-y Nov 1, 2024

andreyvelich Nov 1, 2024

tenzen-y Nov 1, 2024

andreyvelich Nov 1, 2024

tenzen-y Nov 1, 2024

tenzen-y Nov 1, 2024

tenzen-y Nov 1, 2024

andreyvelich Nov 1, 2024 •

edited

Loading

tenzen-y Nov 1, 2024

tenzen-y Nov 1, 2024 •

edited

Loading

tenzen-y left a comment

google-oss-prow bot commented Nov 1, 2024

tenzen-y commented Nov 1, 2024

	- name: Check manifests
	run: \|
	make manifests && git add manifests &&
	git diff --cached --exit-code \|\| (echo 'Please run "make manifests" to generate manifests' && exit 1);

	func mergeInitializerEnvs(storageUri *string, trainJobEnvs, containerEnv []corev1.EnvVar) []corev1.EnvVar {
	func mergeInitializerEnvs(storageUri string, trainJobEnvs, containerEnv []corev1.EnvVar) []corev1.EnvVar {

KEP-2170: Implement Initializer builders in the JobSet plugin #2316

KEP-2170: Implement Initializer builders in the JobSet plugin #2316

Conversation

andreyvelich commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 1, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11636022716

Details

💛 - Coveralls

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

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

andreyvelich Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Nov 1, 2024

tenzen-y commented Nov 1, 2024

coveralls commented Nov 1, 2024 •

edited

Loading

andreyvelich Nov 1, 2024 •

edited

Loading

tenzen-y Nov 1, 2024 •

edited

Loading