training-operator icon indicating copy to clipboard operation
training-operator copied to clipboard

Deprecate MPIJob v1

Open alculquicondor opened this issue 2 years ago • 23 comments

Ideally, we should migrate the v2 implementations to the training operator, then remove the v1 implementation from the training-operator to reduce the maintenance costs. However, we can not take the way immediately because there are many issues in the training operator (e.g. inconsistent job conditions, not using headless svc, and so on). So, I think it would be better to mark the v1 implementation as deprecated, then stop adding the new features to the v1 implementation and only provide bug fixes. So we suggest using the mpi-operator to users if they would like to the new features.

Originally posted by @tenzen-y in https://github.com/kubeflow/training-operator/issues/1768#issuecomment-1709480672

alculquicondor avatar Sep 07 '23 12:09 alculquicondor

@kubeflow/wg-training-leads

alculquicondor avatar Sep 07 '23 12:09 alculquicondor

migrate the v2 implementations to the training operator

Are you suggesting moving the entire codebase to training-operator? Or use mpi-operator as a library?

terrytangyuan avatar Sep 08 '23 00:09 terrytangyuan

migrate the v2 implementations to the training operator

Are you suggesting moving the entire codebase to training-operator? Or use mpi-operator as a library?

Use mpi-operator as a library. I think a separate binary for mpi-operator would be worth it since mpi-operator doesn't focus on ML Training.

tenzen-y avatar Sep 08 '23 06:09 tenzen-y

Sounds good

terrytangyuan avatar Sep 08 '23 11:09 terrytangyuan

there are many issues in the training operator (e.g. inconsistent job conditions, not using headless svc, and so on)

Can you expand on this? This would be helpful for estimating work and allocating sufficient resources.

terrytangyuan avatar Sep 08 '23 17:09 terrytangyuan

there are many issues in the training operator (e.g. inconsistent job conditions, not using headless svc, and so on)

Can you expand on this? This would be helpful for estimating work and allocating sufficient resources.

Sure. Actually, there are already issues:

Headless SVC issue: https://github.com/kubeflow/training-operator/issues/1030 Inconsistent job conditions: https://github.com/kubeflow/training-operator/issues/1703

tenzen-y avatar Sep 08 '23 19:09 tenzen-y

Friendly ping @johnugeorge :)

tenzen-y avatar Sep 14 '23 17:09 tenzen-y

Sorry for late reply.

Agree. I am good with deprecating v1 in favor for v2. We need to take it up sometime. Can you explain more on your idea of creating a library? You mean, reconcile logic to be used from MPI operator repo within training-operator? Is it easy in managing manifests etc?

We will target all pre-reqs(#1030 #1703) for next training operator 1.8 release and then followed by mpi v2 support in training operator if we have time. What do you think?

johnugeorge avatar Oct 09 '23 11:10 johnugeorge

Sorry for late reply.

Agree. I am good with deprecating v1 in favor for v2. We need to take it up sometime. Can you explain more on your idea of creating a library? You mean, reconcile logic to be used from MPI operator repo within training-operator? Is it easy in managing manifests etc?

We will target all pre-reqs(#1030 #1703) for next training operator 1.8 release and then followed by mpi v2 support in training operator if we have time. What do you think?

I have discussed this with @johnugeorge offline. We leave the individual mpi-operator, and the training-operator uses mpi-operatror as a library. It means that users can deploy MPIJob v2 as either part of the training operator or the mpi-operator.

We have tasks to realize this migration and deprecation:

Training Operator Side:

  • [ ] Resolve Headless SVC issue (https://github.com/kubeflow/training-operator/issues/1030)
  • [ ] Resolve Inconsistent job conditions (https://github.com/kubeflow/training-operator/issues/1703)
  • [ ] Import the mpi-operator as a library to the training-operator and provide MPIJob v2
  • [ ] Notice to users that MPIJob v1 is no longer maintenance

MPI Operator Side:

  • [ ] Refactor MPI Operator (kubeflow/mpi-operator) so that the training-operator can use the mpi-operator as a library.

tenzen-y avatar Nov 15 '23 18:11 tenzen-y

We leave the individual mpi-operator, and the training-operator uses mpi-operatror as a library. It means that users can deploy MPIJob v2 as either part of the training operator or the mpi-operator.

@tenzen-y Thanks! This approach looks good.

terrytangyuan avatar Nov 15 '23 18:11 terrytangyuan

Sounds great!

I assume that would fix also https://github.com/kubeflow/training-operator/issues/1807, maybe also some other MPIJob tickets: https://github.com/kubeflow/training-operator/issues?q=is%3Aissue+is%3Aopen+mpijob.

But more important could be whether there will be regressions compared to current v1 features though.

Would training-operator MPIJob tests be updated to v2:

$ find training-operator/ -name '*test.go' | grep -i mpi
training-operator/pkg/apis/kubeflow.org/v1/mpi_validation_test.go
training-operator/pkg/apis/kubeflow.org/v1/mpi_defaults_test.go
training-operator/pkg/controller.v1/mpi/suite_test.go
training-operator/pkg/controller.v1/mpi/mpijob_controller_test.go

And/or mpi-operator tests brought to training-operator?

$ find mpi-operator/ -name '*test.go'
mpi-operator/test/integration/main_test.go
mpi-operator/test/integration/mpi_job_controller_test.go
mpi-operator/test/e2e/e2e_suite_test.go
mpi-operator/test/e2e/mpi_job_test.go
mpi-operator/pkg/controller/mpi_job_controller_test.go
mpi-operator/pkg/controller/podgroup_test.go
mpi-operator/pkg/apis/kubeflow/v2beta1/default_test.go
mpi-operator/pkg/apis/kubeflow/validation/validation_test.go

eero-t avatar Nov 15 '23 18:11 eero-t

I assume that would fix also https://github.com/kubeflow/training-operator/issues/1807, maybe also some other MPIJob tickets: https://github.com/kubeflow/training-operator/issues?q=is%3Aissue+is%3Aopen+mpijob.

Yes, that's right.

Would training-operator MPIJob tests be updated to v2

Yes, we should have proper tests.

And/or mpi-operator tests brought to training-operator?

No, I think that we wouldn't have tests for MPI-Operator library in this repo. However, I think we should implement unit and e2e tests alongside the training-operator.

tenzen-y avatar Nov 15 '23 18:11 tenzen-y

+1 One point that is yet to finalize, is the mpi-operator v2 manifests location. How do users install mpi operator with training operator? How does Kubeflow manifests sync mpi operator manifests during any release?

johnugeorge avatar Nov 16 '23 08:11 johnugeorge

is the mpi-operator v2 manifests location.

I think that we can use kustomize remote ref in the following:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - kubeflow.org_tfjobs.yaml
  - kubeflow.org_mxjobs.yaml
  - kubeflow.org_pytorchjobs.yaml
  - kubeflow.org_xgboostjobs.yaml
  - github.com/kubeflow/mpi-operator/manifesist/crd.yaml?ref=v0.4.0
  - kubeflow.org_paddlejobs.yaml

And then, I think that we can have pre-built all-in-one manifests in this repository for the users without internet access. It means save manifests (deploy.yaml) built with kustomize build github.com/kubeflow/training-operator/manifests/overlays/standalone > deploy.yaml.

How do users install mpi operator with training operator?

If users want to install both operators, users need to disable the MPIJob on the training-operator side as in the past.

tenzen-y avatar Nov 16 '23 08:11 tenzen-y

@tenzen-y Does it mean that we are going to maintain separate releases for MPI Operator and Training Operator ?

andreyvelich avatar Nov 16 '23 12:11 andreyvelich

@tenzen-y Does it mean that we are going to maintain separate releases for MPI Operator and Training Operator ?

Yes, that's right.

tenzen-y avatar Nov 16 '23 21:11 tenzen-y

migrate the v2 implementations to the training operator

Are you suggesting moving the entire codebase to training-operator? Or use mpi-operator as a library?

Use mpi-operator as a library. I think a separate binary for mpi-operator would be worth it since mpi-operator doesn't focus on ML Training.

@tenzen-y Can you explain why mpi-operator doesn't focus on ML Training?

itayvallach avatar Nov 17 '23 11:11 itayvallach

migrate the v2 implementations to the training operator

Are you suggesting moving the entire codebase to training-operator? Or use mpi-operator as a library?

Use mpi-operator as a library. I think a separate binary for mpi-operator would be worth it since mpi-operator doesn't focus on ML Training.

@tenzen-y Can you explain why mpi-operator doesn't focus on ML Training?

MPIJob isn't used only for machine learning. MPIJob is used in generic HPC use cases like simulations. So, I think that we shouldn't focus only on ML use cases.

Any thoughts? > @terrytangyuan @alculquicondor

tenzen-y avatar Nov 17 '23 13:11 tenzen-y

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] avatar Feb 15 '24 15:02 github-actions[bot]

/remove-lifecycle frozen

tenzen-y avatar Feb 15 '24 15:02 tenzen-y

/remove-lifecycle stale

tenzen-y avatar Feb 15 '24 15:02 tenzen-y

/retitle Deprecate MPIJob v1

tenzen-y avatar Apr 26 '24 17:04 tenzen-y

MPIJob isn't used only for machine learning. MPIJob is used in generic HPC use cases like simulations. So, I think that we shouldn't focus only on ML use cases.

+1

vsoch avatar Jul 17 '24 03:07 vsoch