Merge Kubeflow/common to training operator

[johnugeorge]

Kubeflow/common was initially created to have a common library for all frameworks in separate repos. Since we have already merged all operators into a single training operator, do we really need a separate common repository now?

Motivation:

1. Many changes required for training operator requires changes in common code as well. This adds a huge overhead in maintenance. (Ref: Changing label selector type for HPA common#197, update k8s dependencies to 1.25 common#198, Remove deprecated labels common#200)
2. Maintenance of separate kubeflow/common repo and keeping things in sync with training operator is an overhead (Also considering CI and testing)

Related: #1703

/cc @terrytangyuan @zw0610 @gaocegege @Jeffwan

[gaocegege]

/cc @alculquicondor

[terrytangyuan]

I agree that we can consider merging it now that all operators are in training-operator repo. MPI operator v1 still uses this separately and we should put into some thoughts as well.

[zw0610]

I agree the common repository should be merged and the scripts in the common repository should be further simplified to make the training-operator easier to understand for new developers.

[johnugeorge]

@alculquicondor Can you share your thoughts on this wrt mpi operator dependency?

[terrytangyuan]

From past conversations with @alculquicondor, it seems like there's preference to keep v2 MPI controller separate from training-operator due to some use cases to non-ML users. Is that still the case? Perhaps v2 can be migrated to another project under K8s sigs to make it more generic to all users and v1 will stay in training-operator to be suitable for ML use cases only. I believe it should be fine if we keep the copyright notices "Kubeflow Authors". Once Kubeflow is accepted by CNCF, everything belongs to CNCF anyways so I don't have any legal concerns.

Any thoughts from others? Also cc @ahg-g

[alculquicondor]

FWIIW, the only dependencies that mpi-operator has with common is just constants. So one potential starting point is to move all the logic to training-operator, but keep the constants.

Yes, I have heard the desire of having a lightweight controller for MPI jobs from HPC users.
However, I would not advocate for 2 competing implementations. There should just be one and kubeflow should probably just embed it.

kubernetes-sigs would be a good fit. SIG Apps already had interest in hosting the project. But we would have to wait until the ownership goes to CNCF.

[andreyvelich]

I like this idea.
I remember previously we wanted to keep common repo separate because some users have their own internal Training Operator CRDs based on this library.
@johnugeorge @gaocegege @terrytangyuan @alculquicondor Are we aware of any users that are actively using this repository ?
Maybe we should stay in touch with them.

[terrytangyuan]

My team at Ant Group was using it but it's no longer the case. I don't know any other teams that are relying on this repo.

@alculquicondor I think MPI operator repo can keep those constants itself so that we can remove the dependency on Kubeflow/common at once. WDYT?

[alculquicondor]

Would it be too bad to make mpi-operator depend on the constants from training-operator?

[terrytangyuan]

Would it be too bad to make mpi-operator depend on the constants from training-operator?

I am not sure if that's a good idea as training-operator might be too heavy and there may be conflicts in dependencies. We only reply on very minimal set of constants so they should be easy to maintain.

[johnugeorge]

I agree with @terrytangyuan . I think, It is not worth managing overhead in handling dependencies for mpi-operator to use minimal set of constants.(If that is the concern). I leave it to you @alculquicondor

Can folks give +1 to merge kubeflow/common to kubeflow/training-operator? We can take this up after creating release branch for KF release 1.7 during late Jan

[terrytangyuan]

+1

[alculquicondor]

I guess the common repo would just be archived and we can still depend on it while we don't need new constants (likely to be true for a long time).

+1

[andreyvelich]

+1
cc @kubeflow/wg-training-leads

[gaocegege]

LGTM 👍
/lgtm

[gaocegege]

@johnugeorge @gaocegege @terrytangyuan @alculquicondor Are we aware of any users that are actively using this repository ?

Some enterprise users may use it to write their own operators.

But I think they will lock on a specified version.

[johnugeorge]

Based on the responses, kubeflow/common will be merged into kubeflow/training-operator post 1.7 release branch creation.

[alculquicondor]

Was a release branch opened?

[tenzen-y]

Was a release branch opened?

@alculquicondor Yes, we opened RC.0 branch; https://github.com/kubeflow/training-operator/tree/v1.6-branch.
However, we will freeze to work in the kubeflow/common repository until cutting a final release.

kubeflow/common#209 (comment)

[alculquicondor]

are we still on freeze?

[johnugeorge]

@alculquicondor Final release is not cut yet. I am expecting it to happen in a week

[tenzen-y]

/assign @johnugeorge

[johnugeorge]

Closing this as kubeflow/common is merged into training-operator

[Aunpuncode]

Kubeflow/common was initially created to have a common library for all frameworks in separate repos. Since we have already merged all operators into a single training operator, do we really need a separate common repository now?

Motivation:

1. Many changes required for training operator requires changes in common code as well. This adds a huge overhead in maintenance. (Ref: Changing label selector type for HPA common#197, update k8s dependencies to 1.25 common#198, Remove deprecated labels common#200)
2. Maintenance of separate kubeflow/common repo and keeping things in sync with training operator is an overhead (Also considering CI and testing)

Related: #1703

/cc @terrytangyuan @zw0610 @gaocegege @Jeffwan