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.

Sign up for GitHub

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

Jump to bottom

Add XGBoost controller #1293

Merged
merged 2 commits into from
Jul 7, 2021
Merged

Add XGBoost controller #1293

merged 2 commits into from
Jul 7, 2021

Conversation

Jeffwan
Copy link
Member

@Jeffwan Jeffwan commented Jul 6, 2021 •
edited
Loading

  1. Move major controller logics from kubeflow/xgboost-operator to this repo.
  2. Adapt to kubebuilder 3.0.0 change
  3. Add xgboost train example
  4. Leave some TODOs for code refactor later

Signed-off-by: Jiaxin Shan [email protected]

This is a working version. You can follow steps below

# build codes and apply crd.
make build
kubectl apply -f config/crd/bases/kubeflow.org_xgboostjobs.yaml

# run operator and apply job
./bin/manager --kubeconfig ~/.kube/config
kubectl apply -f config/samples/kubeflow.org_v1_xgboostjob.yaml

@Jeffwan
Copy link
Member Author

Jeffwan commented Jul 6, 2021

/hold

@Jeffwan Jeffwan changed the title Add XGBoost controller [WIP] Add XGBoost controller Jul 6, 2021
@Jeffwan Jeffwan changed the title [WIP] Add XGBoost controller Add XGBoost controller Jul 6, 2021
@Jeffwan
Copy link
Member Author

Jeffwan commented Jul 6, 2021

/cc @zw0610 please have a check

@google-oss-robot
Copy link

@Jeffwan: GitHub didn't allow me to request PR reviews from the following users: please, have, a, check.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @zw0610 please have a check

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.

gaocegege
gaocegege reviewed Jul 6, 2021
View reviewed changes
pkg/controller.v1/xgboost/expectation.go
// satisfiedExpectations returns true if the required adds/dels for the given job have been observed.
// Add/del counts are established by the controller at sync time, and updated as controllees are observed by the controller
// manager.
func (r *XGBoostJobReconciler) satisfiedExpectations(xgbJob *v1.XGBoostJob) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need expectations if we use controller-runtime?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct me if I am wrong. I think they are just helpers and help us save some efforts to determine if we need to resync job. Technically, I think we can definitely remove helpers if rest of the logics are solid.

kubernetes-sigs/controller-runtime#644

terrytangyuan
terrytangyuan approved these changes Jul 6, 2021
View reviewed changes
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.

/lgtm

@Jeffwan Jeffwan mentioned this pull request Jul 7, 2021
Merged
@Jeffwan
Add XGBoost controller
684d36f 
1. Move major controller logics from kubeflow/xgboost-operator to this repo.
2. Adapt to kubebuilder 3.0.0 change
3. Add xgboost train example
4. Leave some TODOs for code refactor later

Signed-off-by: Jiaxin Shan <[email protected]>
@Jeffwan
Copy link
Member Author

Jeffwan commented Jul 7, 2021

/cc @kubeflow/wg-training-leads

@google-oss-robot google-oss-robot requested a review from a team July 7, 2021 00:26
@zw0610
Copy link
Member

zw0610 commented Jul 7, 2021

/lgtm

terrytangyuan
terrytangyuan approved these changes Jul 7, 2021
View reviewed changes
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.

/lgtm

@Jeffwan
Copy link
Member Author

Jeffwan commented Jul 7, 2021

/approve

@Jeffwan
Copy link
Member Author

Jeffwan commented Jul 7, 2021

/hold cancel

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jeffwan, 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

@google-oss-robot google-oss-robot merged commit 20d0e99 into kubeflow:all-in-one-operator Jul 7, 2021
@Jeffwan Jeffwan deleted the add_xgboost_controller branch July 7, 2021 19:13
This was referenced Jul 10, 2021
Open
Closed
@Jeffwan
Copy link
Member Author

Jeffwan commented Jul 11, 2021 •
edited
Loading

part of kubeflow/common#138 #1298

Jeffwan added a commit that referenced this pull request Aug 5, 2021
@Jeffwan
Add XGBoost controller (#1293)
d965361 
* Add XGBoost controller

1. Move major controller logics from kubeflow/xgboost-operator to this repo.
2. Adapt to kubebuilder 3.0.0 change
3. Add xgboost train example
4. Leave some TODOs for code refactor later

Signed-off-by: Jiaxin Shan <[email protected]>

* Move examples from config to example/ folder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaocegege gaocegege gaocegege left review comments

@andreyvelich andreyvelich andreyvelich left review comments

@terrytangyuan terrytangyuan terrytangyuan approved these changes

@jinchihe jinchihe Awaiting requested review from jinchihe

@zw0610 zw0610 Awaiting requested review from zw0610

Assignees

@terrytangyuan terrytangyuan

@zw0610 zw0610

Labels
approved lgtm size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Jeffwan @google-oss-robot @zw0610 @terrytangyuan @gaocegege @andreyvelich