Unify code structure of training job api #1300

Jeffwan · 2021-07-12T17:15:12Z

This PR is part of #1299 kubeflow/common#138

It resolves "1. Eliminate difference between operators and make them have same coding structure (including apis, docs, openapi specs)"

We use this opportunity to unify the apis code structure. kubebuilder and code-generator will be used together to achieve the goal

kubebuilder leverage controller-runtime to generate codes. However, it won't give us clientset, openapis, etc. Ideally, we would like to generate clientset,informer,lister for any tools that want to interact with training jobs. In this case, we still need code-generator.

Kubebuilder only gives us following objects.

├── pytorch
│   ├── v1
│   │   ├── groupversion_info.go
│   │   ├── pytorchjob_types.go
│   │   ├── zz_generated.deepcopy.go

Kubebuilder leverages webhook to mutate object and give default values. We definitely don't want to bring extra components to the training operators. Instead, we still leverage defaults.go and manually assign default values to job objects.
Keep openapi for sdk generation in the future

├── pytorch
│   ├── v1
│   │   ├── constants.go                      -> owner: developer. DefaultPort, DefaultContainerName, etc
│   │   ├── defaults.go                         -> owner: developer SetJobDefaults
│   │   ├── doc.go                                -> owner: developer  global markers to generate code.  remove deepcopy marker 
│   │   ├── groupversion_info.go        -> generated by kubebudiler
│   │   ├── openapi_generated.go     -> generated by openapi-gen
│   │   ├── pytorchjob_types.go.       -> originally generated by kubebudiler
│   │   ├── register.go                        -> owner: developer.  used by clientset
│   │   ├── zz_generated.deepcopy.go -> generate by kubebuilder
│   │   └── zz_generated.defaults.go    -> generated by defaulter-gen

- Clean up register.go and use reference in groupversion_info.go (generated by kubebuilder) instead - Add doc.go and register.go for framework missing them - Remove global tag “+k8s:deepcopy-gen=package,register” and this should be taken care of by controller-gen. Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>

1. Update codegen scripts to generate defaulters and openapi spec for all frameworks 2. Create defaults.go for framework which miss it. Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>

Jeffwan · 2021-07-12T17:17:22Z

/cc @kubeflow/wg-training-leads @zw0610 Please have a review on this change

google-oss-robot · 2021-07-12T17:17:26Z

@Jeffwan: GitHub didn't allow me to request PR reviews from the following users: a, review, on, change, Please, have, this.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @kubeflow/wg-training-leads @zw0610 Please have a review on this change

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/test-infra repository.

hack/update-codegen.sh

pkg/controller.v1/mxnet/mxjob_controller.go

Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>

pkg/apis/mxnet/v1/defaults.go

pkg/apis/xgboost/v1/defaults.go

zw0610

looking good.

pkg/apis/xgboost/v1/defaults.go

Jeffwan · 2021-07-13T04:32:35Z

@gaocegege @zw0610 Please have another check. Rerun goimports -w pkg/.

terrytangyuan

LGTM. Consider adding goimports check to the CI?

Jeffwan · 2021-07-13T16:11:12Z

LGTM. Consider adding goimports check to the CI?

Yeah, that's a good idea. I will add it in a separate PR. there're some more cleanups on the CI tools.

johnugeorge · 2021-07-13T20:13:27Z

Great @Jeffwan
LGTM

andreyvelich · 2021-07-13T20:23:37Z

pkg/controller.v1/mxnet/mxjob_controller.go

@@ -123,7 +123,7 @@ type MXJobReconciler struct {

 func (r *MXJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	_ = log.FromContext(ctx)
-	logger := r.Log.WithValues(mxjobv1.Singular, req.NamespacedName)
+	logger := r.Log.WithValues("mxjobs", req.NamespacedName)


@Jeffwan Do we want to keep Singular and Plural in the constants also ?

Yeah, nice catch. I've moved them to constants.go now

Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>

Jeffwan · 2021-07-14T05:11:37Z

pkg/apis/pytorch/v1/doc.go

@@ -12,7 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-// +k8s:deepcopy-gen=package,register


We don't need deepcopy-gen to generate file anymore since we have controller-gen from kubebuilder. They do the same work

Jeffwan · 2021-07-14T19:08:03Z

Seems it looks good to most reviewers. Can someone leave a /lgtm ?

/cc @kubeflow/wg-training-leads

terrytangyuan

/lgtm

Jeffwan · 2021-07-14T19:11:41Z

/approve

google-oss-robot · 2021-07-14T19:11:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jeffwan, terrytangyuan, zw0610

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~~ [Jeffwan]

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

* Add docs, register in job apis - Clean up register.go and use reference in groupversion_info.go (generated by kubebuilder) instead - Add doc.go and register.go for framework missing them - Remove global tag “+k8s:deepcopy-gen=package,register” and this should be taken care of by controller-gen. Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com> * Generate defaults and openapi for all frameworks 1. Update codegen scripts to generate defaulters and openapi spec for all frameworks 2. Create defaults.go for framework which miss it. Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com> * Add Kind in constants.go Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com> * Update update-codegen to generate openapi-gen Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com> * Run goimports to format the files * Add Plural and Singular to apis constants.go Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>

Jeffwan added 2 commits July 12, 2021 09:55

Generate defaults and openapi for all frameworks

7ddf016

1. Update codegen scripts to generate defaulters and openapi spec for all frameworks 2. Create defaults.go for framework which miss it. Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>

google-oss-robot requested review from jinchihe and terrytangyuan July 12, 2021 17:15

google-oss-robot added the size/XXL label Jul 12, 2021

Jeffwan changed the title ~~Unify~~ Unify code structure of training job api Jul 12, 2021

google-oss-robot requested review from zw0610 and a team July 12, 2021 17:17

andreyvelich reviewed Jul 12, 2021

View reviewed changes

hack/update-codegen.sh Show resolved Hide resolved

pkg/controller.v1/mxnet/mxjob_controller.go Outdated Show resolved Hide resolved

Jeffwan added 2 commits July 12, 2021 14:13

Add Kind in constants.go

26f2f21

Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>

Update update-codegen to generate openapi-gen

15e4bb5

Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>

gaocegege reviewed Jul 13, 2021

View reviewed changes

pkg/apis/mxnet/v1/defaults.go Outdated Show resolved Hide resolved

pkg/apis/xgboost/v1/defaults.go Outdated Show resolved Hide resolved

zw0610 approved these changes Jul 13, 2021

View reviewed changes

pkg/apis/xgboost/v1/defaults.go Outdated Show resolved Hide resolved

Run goimports to format the files

9fa6efe

terrytangyuan reviewed Jul 13, 2021

View reviewed changes

andreyvelich reviewed Jul 13, 2021

View reviewed changes

Add Plural and Singular to apis constants.go

b1d6843

Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>

Jeffwan commented Jul 14, 2021

View reviewed changes

google-oss-robot requested a review from a team July 14, 2021 19:08

terrytangyuan approved these changes Jul 14, 2021

View reviewed changes

google-oss-robot assigned terrytangyuan Jul 14, 2021

google-oss-robot added the lgtm label Jul 14, 2021

google-oss-robot added the approved label Jul 14, 2021

google-oss-robot merged commit ae7ffd6 into kubeflow:all-in-one-operator Jul 14, 2021

Jeffwan deleted the cleanup_apis branch July 14, 2021 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify code structure of training job api #1300

Unify code structure of training job api #1300

Jeffwan commented Jul 12, 2021 •

edited

Jeffwan commented Jul 12, 2021

google-oss-robot commented Jul 12, 2021

zw0610 left a comment

Jeffwan commented Jul 13, 2021

terrytangyuan left a comment •

edited

Jeffwan commented Jul 13, 2021

johnugeorge commented Jul 13, 2021

andreyvelich Jul 13, 2021

Jeffwan Jul 14, 2021

Jeffwan Jul 14, 2021

Jeffwan commented Jul 14, 2021

terrytangyuan left a comment

Jeffwan commented Jul 14, 2021

google-oss-robot commented Jul 14, 2021

Unify code structure of training job api #1300

Unify code structure of training job api #1300

Conversation

Jeffwan commented Jul 12, 2021 • edited

Jeffwan commented Jul 12, 2021

google-oss-robot commented Jul 12, 2021

zw0610 left a comment

Choose a reason for hiding this comment

Jeffwan commented Jul 13, 2021

terrytangyuan left a comment • edited

Choose a reason for hiding this comment

Jeffwan commented Jul 13, 2021

johnugeorge commented Jul 13, 2021

andreyvelich Jul 13, 2021

Choose a reason for hiding this comment

Jeffwan Jul 14, 2021

Choose a reason for hiding this comment

Jeffwan Jul 14, 2021

Choose a reason for hiding this comment

Jeffwan commented Jul 14, 2021

terrytangyuan left a comment

Choose a reason for hiding this comment

Jeffwan commented Jul 14, 2021

google-oss-robot commented Jul 14, 2021

Jeffwan commented Jul 12, 2021 •

edited

terrytangyuan left a comment •

edited