feature: add support for mpijob in katib #1183

YuxiJin-tobeyjin · 2020-05-11T13:30:37Z

What this PR does / why we need it:
Add support for mpijob in katib

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1181

Release note:

None

cc @gaocegege @johnugeorge @andreyvelich

googlebot · 2020-05-11T13:30:43Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

kubeflow-bot · 2020-05-11T13:30:44Z

This change is

k8s-ci-robot · 2020-05-11T13:30:48Z

Hi @YuxiJin-tobeyjin. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2020-05-11T13:30:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign johnugeorge
You can assign the PR to them by writing /assign @johnugeorge in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2020-05-11T14:12:32Z

@YuxiJin-tobeyjin: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

YuxiJin-tobeyjin · 2020-05-11T14:15:37Z

@googlebot I signed it!

googlebot · 2020-05-11T14:15:40Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

gaocegege

/assign @andreyvelich @johnugeorge

andreyvelich

@YuxiJin-tobeyjin Thank you for doing this!
Can you add e2e test for MPI Job, please?

andreyvelich · 2020-05-11T14:54:00Z

pkg/controller.v1alpha3/consts/const.go


 	// built-in JobRoles
 	JobRole        = "job-role"
 	JobRoleTF      = "tf-job-role"
 	JobRolePyTorch = "pytorch-job-role"
+	JobRoleMpi     = "mpi_role_type"


Job roles in MPI are named with "_" not with "-" ?

Yes, refer to
https://github.com/kubeflow/mpi-operator/blob/master/pkg/controllers/v1alpha2/mpi_job_controller.go#L81

@terrytangyuan Hi, will we change the label in the future, maybe v1beta1 or v1?

Agree with @gaocegege. I think MPI should follow the same patterns as TF Job and Pytorch Job.

Thanks. I changed it in kubeflow/mpi-operator#252 for v1 candidate of MPI Operator. Perhaps this PR can add support for v1 candidate directly? The API should be relatively stable even though there isn't official release yet.

Agree with @gaocegege .
@YuxiJin-tobeyjin can you change this PR to support v1 MPI Operator version?

Ok, but I need to make some tests on latest mpi master branch to ensure changes work as expected, I will do it ASAP.

Thanks for your contribution! 🎉 👍

@terrytangyuan I've tried mpi-operator master branch on our cluster with kubernetes 1.14 and encountered some problems.

First, when creating new mpijob crd ,it throws up

unknown field "additionalPrinterColumns" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1beta1.CustomResourceValidation

Walk around then, the crd has been created ok, but the mpi-operator is created failed

the server could not find the requested resource (get mpijobs.kubeflow.org)

Maybe problem is here ,still use KubeflowV1alpha2 to get v1 crd?

https://github.com/kubeflow/mpi-operator/blob/acddf3028ce922e24ac3e735a2928ff4487be28f/cmd/mpi-operator.v1/app/server.go#L288

Walk around then, but mpi-operator throws

Failed to list *v1beta1.PodGroup: podgroups.scheduling.volcano.sh

So , now use mpi-operator v1 we must use volcano as the batch scheduler? Since kube-batch latest release do not support v1beta1.PodGroup. Actually we've already used the kube-batch latest release for some times...

Thanks! I fixed the CRD validation issue in kubeflow/mpi-operator#257 (comment). Yes Volcano has better support and maintenance. You may want to update your MPI Operator's deployment yaml which includes permissions for Volcano resources: https://github.com/kubeflow/mpi-operator/blob/master/deploy/v1/mpi-operator.yaml

andreyvelich · 2020-05-11T14:55:51Z

pkg/metricscollector/v1alpha3/common/const.go

@@ -30,7 +30,7 @@ const (

 	TrainingCompleted = "completed"

-	DefaultFilter = `([\w|-]+)\s*=\s*((-?\d+)(\.\d+)?)`
+	DefaultFilter = `([\w|-]+)\s*[:=]\s*((-?\d+)(\.\d+)?)`


Why do you want to change DefaultFilter ?

In most cases ,the print logs are like “filtername : *%“ or “filtername = *%“, so I prefer the default filter to support both “:” and "=", would it be better? @andreyvelich WDYT

I'm happy with this modification.

If we change this, I think we should modify documentation about default filter for Metrics Collector. For example here: https://www.kubeflow.org/docs/components/hyperparameter-tuning/experiment/#metrics-collector.

Do you think specifying filter with filter format (:) will be not handy for Katib user?
E.g, like we did for DARTS example: https://github.com/kubeflow/katib/blob/master/examples/v1alpha3/nas/darts-example-gpu.yaml#L19.
Your thoughts @gaocegege @johnugeorge ?

I think we can keep it to enhance the default filter. But we need to open an issue to update the doc.

terrytangyuan · 2020-05-12T12:48:38Z

pkg/webhook/v1alpha3/pod/inject_webhook_test.go

+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: "batch/v1",
+							Kind:       "Job",


Not sure if it's related here but just FYI we removed Job and Statefulset for v1 candidate here so now MPI Operator only creates pods: kubeflow/mpi-operator#203

(⊙o⊙)… we developed some features based on worker's sts property...

@terrytangyuan Is there any plan when to release a new mpi-operator version?

No concrete timeline yet but it should be relatively stable now as we are getting close to graduation.

carmark · 2020-09-23T08:25:40Z

@andreyvelich The proposal was merged, could we move this PR forward or submit a new one?

YuxiJin-tobeyjin · 2020-09-23T08:36:36Z

Close cause #1273 will give a better solution.

andreyvelich · 2020-10-03T00:50:56Z

@carmark Sure, I have already created PR to support MPI operator in Katib: #1342.

feature: add support for mpijob in katib

c47ef29

k8s-ci-robot added needs-ok-to-test size/M labels May 11, 2020

k8s-ci-robot requested review from Akado2009 and garganubhav May 11, 2020 13:30

gaocegege reviewed May 11, 2020

View reviewed changes

k8s-ci-robot assigned andreyvelich and johnugeorge May 11, 2020

andreyvelich reviewed May 11, 2020

View reviewed changes

terrytangyuan mentioned this pull request May 12, 2020

Consistent label conventions with other operators kubeflow/mpi-operator#252

Merged

terrytangyuan reviewed May 12, 2020

View reviewed changes

terrytangyuan mentioned this pull request May 14, 2020

Fix issue in v1 controller's CRD existence check kubeflow/mpi-operator#257

Merged

andreyvelich mentioned this pull request Jul 21, 2020

Proposal: Support custom CRD in Trial Job #1273

Merged

YuxiJin-tobeyjin closed this Sep 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add support for mpijob in katib #1183

feature: add support for mpijob in katib #1183

YuxiJin-tobeyjin commented May 11, 2020

googlebot commented May 11, 2020

kubeflow-bot commented May 11, 2020

k8s-ci-robot commented May 11, 2020

k8s-ci-robot commented May 11, 2020

k8s-ci-robot commented May 11, 2020

YuxiJin-tobeyjin commented May 11, 2020

googlebot commented May 11, 2020

gaocegege left a comment

andreyvelich left a comment

andreyvelich May 11, 2020

YuxiJin-tobeyjin May 12, 2020

gaocegege May 12, 2020

andreyvelich May 12, 2020

terrytangyuan May 12, 2020

andreyvelich May 13, 2020

YuxiJin-tobeyjin May 14, 2020

gaocegege May 14, 2020

YuxiJin-tobeyjin May 14, 2020 •

edited

Loading

terrytangyuan May 14, 2020

andreyvelich May 11, 2020

YuxiJin-tobeyjin May 12, 2020

sperlingxx May 12, 2020

andreyvelich May 12, 2020

gaocegege May 13, 2020

terrytangyuan May 12, 2020 •

edited

Loading

YuxiJin-tobeyjin May 13, 2020

YuxiJin-tobeyjin May 13, 2020

terrytangyuan May 13, 2020 •

edited

Loading

carmark commented Sep 23, 2020

YuxiJin-tobeyjin commented Sep 23, 2020

andreyvelich commented Oct 3, 2020 •

edited

Loading

feature: add support for mpijob in katib #1183

feature: add support for mpijob in katib #1183

Conversation

YuxiJin-tobeyjin commented May 11, 2020

googlebot commented May 11, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

kubeflow-bot commented May 11, 2020

k8s-ci-robot commented May 11, 2020

k8s-ci-robot commented May 11, 2020

k8s-ci-robot commented May 11, 2020

YuxiJin-tobeyjin commented May 11, 2020

googlebot commented May 11, 2020

gaocegege left a comment

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuxiJin-tobeyjin May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrytangyuan May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrytangyuan May 13, 2020 • edited Loading

Choose a reason for hiding this comment

carmark commented Sep 23, 2020

YuxiJin-tobeyjin commented Sep 23, 2020

andreyvelich commented Oct 3, 2020 • edited Loading

YuxiJin-tobeyjin May 14, 2020 •

edited

Loading

terrytangyuan May 12, 2020 •

edited

Loading

terrytangyuan May 13, 2020 •

edited

Loading

andreyvelich commented Oct 3, 2020 •

edited

Loading