Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate MPIJob v1 #1906

Open
alculquicondor opened this issue Sep 7, 2023 · 23 comments
Open

Deprecate MPIJob v1 #1906

alculquicondor opened this issue Sep 7, 2023 · 23 comments

Comments

@alculquicondor
Copy link

alculquicondor commented Sep 7, 2023

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 #1768 (comment)

@alculquicondor
Copy link
Author

@kubeflow/wg-training-leads

@terrytangyuan
Copy link
Member

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
Copy link
Member

tenzen-y commented Sep 8, 2023

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
Copy link
Member

Sounds good

@terrytangyuan
Copy link
Member

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
Copy link
Member

tenzen-y commented Sep 8, 2023

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: #1030
Inconsistent job conditions: #1703

@tenzen-y
Copy link
Member

Friendly ping @johnugeorge :)

@johnugeorge
Copy link
Member

johnugeorge commented Oct 9, 2023

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
Copy link
Member

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:

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

@terrytangyuan
Copy link
Member

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
Copy link

eero-t commented Nov 15, 2023

Sounds great!

I assume that would fix also #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
Copy link
Member

tenzen-y commented Nov 15, 2023

I assume that would fix also #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
Copy link
Member

+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
Copy link
Member

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
Copy link
Member

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

@tenzen-y
Copy link
Member

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

Yes, that's right.

@itayvallach
Copy link

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
Copy link
Member

tenzen-y commented Nov 17, 2023

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

Copy link

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
Copy link
Member

/remove-lifecycle frozen

@tenzen-y
Copy link
Member

/remove-lifecycle stale

@tenzen-y
Copy link
Member

/retitle Deprecate MPIJob v1

@tenzen-y tenzen-y changed the title Deprecate MPI Operator v1 Deprecate MPIJob v1 Apr 26, 2024
@vsoch
Copy link

vsoch commented Jul 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants