kubeflow / training-operator

Distributed ML Training and Fine-Tuning on Kubernetes
https://www.kubeflow.org/docs/components/training
Apache License 2.0
1.6k stars 697 forks source link

Deprecate MPIJob v1 #1906

Open alculquicondor opened 1 year ago

alculquicondor commented 1 year ago

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 commented 1 year ago

@kubeflow/wg-training-leads

terrytangyuan commented 1 year ago

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?

tenzen-y commented 1 year ago

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.

terrytangyuan commented 1 year ago

Sounds good

terrytangyuan commented 1 year ago

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.

tenzen-y commented 1 year ago

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 commented 1 year ago

Friendly ping @johnugeorge :)

johnugeorge commented 1 year ago

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?

tenzen-y commented 11 months ago

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:

MPI Operator Side:

terrytangyuan commented 11 months ago

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.

eero-t commented 11 months ago

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
tenzen-y commented 11 months ago

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.

johnugeorge commented 11 months ago

+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?

tenzen-y commented 11 months ago

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.

andreyvelich commented 11 months ago

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

tenzen-y commented 11 months ago

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

Yes, that's right.

itayvallach commented 11 months ago

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?

tenzen-y commented 11 months ago

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

github-actions[bot] commented 8 months ago

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.

tenzen-y commented 8 months ago

/remove-lifecycle frozen

tenzen-y commented 8 months ago

/remove-lifecycle stale

tenzen-y commented 6 months ago

/retitle Deprecate MPIJob v1

vsoch commented 3 months ago

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

github-actions[bot] commented 3 weeks ago

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.

vsoch commented 3 weeks ago

We probably don't want to close this.