Update the naming conventions for Kubeflow Trainer #2415

andreyvelich · 2025-02-04T23:00:00Z

Part of: #2170
This PR updates the naming conventions for Kubeflow Trainer.
Also, it changes CRDs to v1alpha1.

Please give me your thoughts on the naming if that looks good.

Kubeflow Trainer Images:

docker.io/kubeflow/trainer-controller-manager:<SHA>
docker.io/kubeflow/model-initializer:<SHA>
docker.io/kubeflow/dataset-initializer:<SHA>

Kubeflow Trainer k8s resources:

$ k get deploy -n kubeflow-system

NAME                                  READY   UP-TO-DATE   AVAILABLE   AGE
jobset-controller-manager             1/1     1            1           63m
kubeflow-trainer-controller-manager   1/1     1            1           63m

$ k get svc -n kubeflow-system

NAME                                        TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)            AGE
jobset-controller-manager-metrics-service   ClusterIP   10.96.40.9      <none>        8443/TCP           64m
jobset-webhook-service                      ClusterIP   10.96.47.150    <none>        443/TCP            64m
kubeflow-trainer-controller-manager         ClusterIP   10.96.229.157   <none>        8080/TCP,443/TCP   64m

$ k get sa -n kubeflow-system 

NAME                                  SECRETS   AGE
jobset-controller-manager             0         65m
kubeflow-trainer-controller-manager   0         65m

$ k get ValidatingWebhookConfiguration
NAME                                      WEBHOOKS   AGE
jobset-validating-webhook-configuration   2          65m
validator.trainer.kubeflow.org            3          65m

TODOs:

Update manifests
Update GitHub actions
Update SDK Trainer client.

/assign @tenzen-y @kubeflow/wg-training-leads @Electronic-Waste @saileshd1402 @astefanutti @franciscojavierarceo @saileshd1402 @seanlaii @kannon92

review-notebook-app · 2025-02-04T23:00:07Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

andreyvelich · 2025-02-05T02:53:32Z

examples/pytorch/image-classification/mnist.ipynb

-    "    if local_rank == 0:\n",
-    "        # Only download the dataset from local rank 0\n",
-    "        dataset = datasets.FashionMNIST(\n",
-    "            \"./data\",\n",
-    "            train=True,\n",
-    "            download=True,\n",
-    "            transform=transforms.Compose([transforms.ToTensor()]),\n",
-    "        )\n",
-    "        dist.barrier()\n",
-    "    else:\n",
-    "        # Wait for local rank 0 to complete downloading the dataset and load it\n",
-    "        dist.barrier()\n",
-    "        dataset = datasets.FashionMNIST(\n",
-    "            \"./data\",\n",
-    "            train=True,\n",
-    "            download=False,\n",
-    "            transform=transforms.Compose([transforms.ToTensor()]),\n",
-    "        )\n",


I removed the dataset logic from this example for now. Since this Notebook demonstrates single proc per Node we should be good.

I think, we can find more elegant way to download data only on LOCAL_RANK=0 process.

WDYT @astefanutti ?

Good idea, I agree we can revisit this and find a better approach.

andreyvelich · 2025-02-05T02:54:53Z

cc @akshaychitneni @shravan-achar

Electronic-Waste · 2025-02-05T15:37:04Z

@andreyvelich We may need to rebase this PR since #2413 changed the API Group.

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

andreyvelich · 2025-02-05T15:45:53Z

It should be good now.
Please help with the review @Electronic-Waste @kubeflow/wg-training-leads @astefanutti @franciscojavierarceo

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

astefanutti · 2025-02-05T16:12:07Z

@andreyvelich Looks good to me.

Not strictly related to the scope of this PR but I see you've fixed the webhooks paths already :)

andreyvelich · 2025-02-05T16:51:23Z

@tenzen-y @Electronic-Waste @astefanutti Any thoughts what I forgot to fix to resolve problem with the Webhook ?

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

andreyvelich · 2025-02-05T17:01:43Z

It looks like Webhook paths, should have the correct address.

astefanutti · 2025-02-05T17:04:58Z

It looks correct to me, according to what controller-runtime generates: https://github.com/kubernetes-sigs/controller-runtime/blob/bbc9711d9b2db31ce828714e7d3c875c00b4c1f1/pkg/builder/webhook.go#L287

tenzen-y

Thanks, just some minor comments

tenzen-y · 2025-02-05T19:59:28Z

Makefile

@@ -102,7 +102,7 @@ endif
 # Instructions to run tests.
 .PHONY: test
 test: ## Run Go unit test.
-	go test ./pkg/apis/trainer/v2alpha1/... ./pkg/controller.v2/... ./pkg/runtime.v2/... ./pkg/webhooks.v2/... ./pkg/util.v2/... -coverprofile cover.out
+	go test ./pkg/apis/trainer/v1alpha1/... ./pkg/controller/... ./pkg/runtime/... ./pkg/webhooks/... ./pkg/util/... -coverprofile cover.out


Suggested change

go test ./pkg/apis/trainer/v1alpha1/... ./pkg/controller/... ./pkg/runtime/... ./pkg/webhooks/... ./pkg/util/... -coverprofile cover.out

go test $(shell go list ./... | grep -v '/test/') -coverprofile cover.out

Currently, all Go files should be tested. When I set up this, we have 2 version codes (v1 and v2). So, I specify the dedicated directory.

tenzen-y · 2025-02-05T20:01:36Z

cmd/trainer-controller-manager/main.go

-	runtime "github.com/kubeflow/training-operator/pkg/runtime.v2"
-	runtimecore "github.com/kubeflow/training-operator/pkg/runtime.v2/core"
-	webhooksv2 "github.com/kubeflow/training-operator/pkg/webhooks.v2"
+	kubeflowv1 "github.com/kubeflow/trainer/pkg/apis/trainer/v1alpha1"


Suggested change

kubeflowv1 "github.com/kubeflow/trainer/pkg/apis/trainer/v1alpha1"

kubeflow "github.com/kubeflow/trainer/pkg/apis/trainer/v1alpha1"

Now, we can just say "kubeflow".

@tenzen-y Can we keep the kubeflowv1 for now since in the future, we might want to introduce another APIs: v1alpha2 and we should distinguish it in the controller ?

In that case, we may need kubeflowv1alpha1 to distinguish versions like v1alpha2 from v1alpha1.

Would you mind do it in the follow-up PR, given the number of changes this PR already has ?

Yeah，I would like to help this:)

I would recommend using kubeflow since we should make the import name shorter as much as possible.
Typically, we use kubeflow or trainer for the latest internal API version.
After we graduate v1alpha1 to v1beta1, and then introduce new API with v1alpha1, we call the new alpha API as kubeflowv1alpha1 or trainerv1alpha1.

We can imagine the situation where Kube impl uses core as import name, but, external projects use corev1.

https://go.dev/blog/package-names

Let's use trainer to be more explicit here.

tenzen-y · 2025-02-05T20:02:26Z

cmd/trainer-controller-manager/main.go

-	runtimecore "github.com/kubeflow/training-operator/pkg/runtime.v2/core"
-	webhooksv2 "github.com/kubeflow/training-operator/pkg/webhooks.v2"
+	kubeflowv1 "github.com/kubeflow/trainer/pkg/apis/trainer/v1alpha1"
+	kubeflowcontroller "github.com/kubeflow/trainer/pkg/controller"


Suggested change

kubeflowcontroller "github.com/kubeflow/trainer/pkg/controller"

"github.com/kubeflow/trainer/pkg/controller"

Have you seen any conflicts with "controller"?

Yeah, it conflicts with this pkg: https://github.com/kubeflow/training-operator/blob/91bd7ef89a76bda71f1df324485b46b905a2c2b3/cmd/trainer-controller-manager/main.go#L32

In that case, we typically use ctrl "sigs.k8s.io/controller-runtime/pkg/controller".
In go, the package name should be shorter as much as possible. If the shorter name is popular, we can use the shorter name like controller -> ctrl , format -> fmt.

https://go.dev/blog/package-names

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

Electronic-Waste

Basically LGTM. Just a few comments for you @andreyvelich

Electronic-Waste · 2025-02-06T11:57:53Z

cmd/trainer-controller-manager/main.go

+	"github.com/kubeflow/trainer/pkg/runtime"
+	runtimecore "github.com/kubeflow/trainer/pkg/runtime/core"
+	"github.com/kubeflow/trainer/pkg/util/cert"
+	webhooks "github.com/kubeflow/trainer/pkg/webhooks"


Do we also want to distinguish different versions of webhooks just like APIs?

#2415 (comment)

Electronic-Waste · 2025-02-06T11:59:58Z

cmd/trainer-controller-manager/main.go

-	runtimecore "github.com/kubeflow/training-operator/pkg/runtime.v2/core"
-	webhooksv2 "github.com/kubeflow/training-operator/pkg/webhooks.v2"
+	kubeflowv1 "github.com/kubeflow/trainer/pkg/apis/trainer/v1alpha1"
+	kubeflowcontroller "github.com/kubeflow/trainer/pkg/controller"


Same question for the controller. Do we want to distinguish different versions of controllers?

#2415 (comment)

Electronic-Waste · 2025-02-06T12:10:59Z

docs/release/changelog.py

-    + "For example: [v1.7.0](https://github.com/kubeflow/training-operator/releases/tag/v1.7.0)",
+    + "For example: [v1.7.0](https://github.com/kubeflow/trainer/releases/tag/v1.7.0)",


Have we enabled url redirection? If so, we may not need to change this url since v1.7.0 is the release of training-operator.

I think, once we rename the repo GitHub automatically add the redirects it.

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

andreyvelich · 2025-02-06T13:04:58Z

@tenzen-y @Electronic-Waste I renamed kubeflowv1 to trainer.

tenzen-y · 2025-02-06T13:15:35Z

Great to see, thanks.
/lgtm
/approve

google-oss-prow · 2025-02-06T13:15:42Z

[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 added the do-not-merge/work-in-progress label Feb 4, 2025

google-oss-prow bot requested review from jinchihe and kuizhiqing February 4, 2025 23:00

google-oss-prow bot added the size/XXL label Feb 4, 2025

andreyvelich commented Feb 5, 2025

View reviewed changes

andreyvelich force-pushed the rename-to-trainer branch 2 times, most recently from 786f92e to fbc6cb9 Compare February 5, 2025 12:55

andreyvelich changed the title ~~[WIP] Update the naming conventions for Kubeflow Trainer~~ Update the naming conventions for Kubeflow Trainer Feb 5, 2025

google-oss-prow bot removed the do-not-merge/work-in-progress label Feb 5, 2025

Update the naming conventions for Kubeflow Trainer

0cf4ff6

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

andreyvelich force-pushed the rename-to-trainer branch from fbc6cb9 to 0cf4ff6 Compare February 5, 2025 15:45

Fix webhooks

1e38960

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

Fix paths for webhooks

91bd7ef

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

tenzen-y reviewed Feb 5, 2025

View reviewed changes

Update go test cmd

f6dd14b

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

Electronic-Waste reviewed Feb 6, 2025

View reviewed changes

Rename kubeflowv1 to trainer pkg

d15a9c5

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

google-oss-prow bot assigned tenzen-y Feb 6, 2025

google-oss-prow bot added the lgtm label Feb 6, 2025

google-oss-prow bot added the approved label Feb 6, 2025

google-oss-prow bot merged commit 3060332 into kubeflow:master Feb 6, 2025
14 checks passed

andreyvelich deleted the rename-to-trainer branch February 6, 2025 13:56

andreyvelich mentioned this pull request Feb 6, 2025

Fix the group name, keep it consistent with the CRD definition #2421

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the naming conventions for Kubeflow Trainer #2415

Update the naming conventions for Kubeflow Trainer #2415

andreyvelich commented Feb 4, 2025 •

edited

Loading

review-notebook-app bot commented Feb 4, 2025

andreyvelich Feb 5, 2025

astefanutti Feb 5, 2025

andreyvelich commented Feb 5, 2025

Electronic-Waste commented Feb 5, 2025

andreyvelich commented Feb 5, 2025

astefanutti commented Feb 5, 2025

andreyvelich commented Feb 5, 2025

andreyvelich commented Feb 5, 2025

astefanutti commented Feb 5, 2025

tenzen-y left a comment

tenzen-y Feb 5, 2025

tenzen-y Feb 5, 2025

andreyvelich Feb 6, 2025

Electronic-Waste Feb 6, 2025

andreyvelich Feb 6, 2025

Electronic-Waste Feb 6, 2025

tenzen-y Feb 6, 2025 •

edited

Loading

andreyvelich Feb 6, 2025

tenzen-y Feb 5, 2025

andreyvelich Feb 6, 2025

tenzen-y Feb 6, 2025

Electronic-Waste left a comment

Electronic-Waste Feb 6, 2025

tenzen-y Feb 6, 2025

Electronic-Waste Feb 6, 2025

tenzen-y Feb 6, 2025

Electronic-Waste Feb 6, 2025

andreyvelich Feb 6, 2025

andreyvelich commented Feb 6, 2025

tenzen-y commented Feb 6, 2025

google-oss-prow bot commented Feb 6, 2025

	go test ./pkg/apis/trainer/v1alpha1/... ./pkg/controller/... ./pkg/runtime/... ./pkg/webhooks/... ./pkg/util/... -coverprofile cover.out
	go test $(shell go list ./... \| grep -v '/test/') -coverprofile cover.out

	kubeflowv1 "github.com/kubeflow/trainer/pkg/apis/trainer/v1alpha1"
	kubeflow "github.com/kubeflow/trainer/pkg/apis/trainer/v1alpha1"

	kubeflowcontroller "github.com/kubeflow/trainer/pkg/controller"
	"github.com/kubeflow/trainer/pkg/controller"

		+ "For example: [v1.7.0](https://github.com/kubeflow/training-operator/releases/tag/v1.7.0)",
		+ "For example: [v1.7.0](https://github.com/kubeflow/trainer/releases/tag/v1.7.0)",

Update the naming conventions for Kubeflow Trainer #2415

Update the naming conventions for Kubeflow Trainer #2415

Conversation

andreyvelich commented Feb 4, 2025 • edited Loading

review-notebook-app bot commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Feb 5, 2025

Electronic-Waste commented Feb 5, 2025

andreyvelich commented Feb 5, 2025

astefanutti commented Feb 5, 2025

andreyvelich commented Feb 5, 2025

andreyvelich commented Feb 5, 2025

astefanutti commented Feb 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y Feb 6, 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

Choose a reason for hiding this comment

Electronic-Waste 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

andreyvelich commented Feb 6, 2025

tenzen-y commented Feb 6, 2025

google-oss-prow bot commented Feb 6, 2025

andreyvelich commented Feb 4, 2025 •

edited

Loading

tenzen-y Feb 6, 2025 •

edited

Loading