Skip to content
This repository has been archived by the owner on Feb 1, 2022. It is now read-only.

Migrate v1 implementation to latest kubeflow/common #85

Merged
merged 2 commits into from
May 19, 2020

Conversation

Jeffwan
Copy link
Member

@Jeffwan Jeffwan commented May 19, 2020

Address #83 and part of #44

As you can see in the PR,

  1. We don't need to implement Create/Delete Pod/Service. As this is pretty generic, kubeflow/common already provides implementation
  2. Batch scheduler has been changed from kube-batch to volcano. By default, it's disabled in XGBoost operator. This is just reversed for users who need it.
  3. expectation.go starts to use expectation package from kubeflow/common.
  4. package changes from
  • "github.com/kubeflow/common/job_controller/api/v1"
  • "github.com/kubeflow/common/job_controller"
    to
  • "github.com/kubeflow/common/pkg/apis/common/v1"
  • "github.com/kubeflow/common/pkg/controller.v1/common"

We can easily get performance improvement, bug fixed via common version upgrade.

Kubeflow/common will be more and more stable, the changes will be few lines in the future.

@kubeflow-bot
Copy link

This change is Reviewable

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks

/lgtm
/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: terrytangyuan

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

The pull request process is described here

Needs approval from an approver in each of these files:

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 k8s-ci-robot merged commit 34d49cc into kubeflow:master May 19, 2020
@merlintang
Copy link
Contributor

Why do we require to use volcano ? in most of the cases, we do not have kubebatch or volcano. @Jeffwan

WorkQueue: &FakeWorkQueue{},
Recorder: r.recorder,
KubeClientSet: kubeClientSet,
VolcanoClientSet: volcanoClientSet,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for common, volcano is the default scheduler?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@merlintang Yes. Most of the kube-batch contributors come to volcano project now. This is a kind of placeholder and make sure it's common compatible. In the previous implementation, we import kube-batch as well but not use it.

xgboost itself can still be installed and used separately. This is transparent to users and user don't need to use it. I should add more comments on the code.

One improvement we can do here is to hide these details to different operators. I will add one issue kubeflow/common#96

@Jeffwan Jeffwan deleted the migrate_v1_to_common branch May 19, 2020 17:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants