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

add XGBoostJob api #1286

Merged
merged 1 commit into from
Jul 5, 2021
Merged

add XGBoostJob api #1286

merged 1 commit into from
Jul 5, 2021

Conversation

zw0610
Copy link
Member

@zw0610 zw0610 commented Jul 3, 2021

As Jiaxin explained below, we are working to unify multiple training operators into a single operator, which is able to use a universal design pattern, resource and code base to offer existing features to users.

@Jeffwan

@Jeffwan
Copy link
Member

Jeffwan commented Jul 4, 2021

For people who are not familiar with the change, we plan to implement universal operator in a separate branch in this project. The major reason is to keep original star we accumulated.

@Jeffwan
Copy link
Member

Jeffwan commented Jul 4, 2021

Can we stop using go mod vendor mode. vendor changes are very big and it distract attention from major code changes.
(This PR is so large due to massive vendor files and I can not even open it)

Copy link
Member

@Jeffwan Jeffwan left a comment

Choose a reason for hiding this comment

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

Can you help refine go.mod?

if you plan to use 1.19, then choose controller-runtime 0.7.x (kubebuilder v3.0.0)
If you plan to use 1.18, then choose controller-runtime 0.6.x

@@ -0,0 +1,17 @@
apiVersion: rbac.authorization.k8s.io/v1
Copy link
Member

Choose a reason for hiding this comment

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

minor: We can remove these unused configs later

annotations:
controller-gen.kubebuilder.io/version: v0.4.1
creationTimestamp: null
name: tfjobs.kubeflow.org
Copy link
Member

Choose a reason for hiding this comment

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

I didn't find other tfjobs related files. Is this added by mistake?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's auto-generated when I execute make generate & make manifests after adding XGBoost API. Because these two command in Makefile apply code generation for all files in pkg/apis/..., the TFJob, whose api files are already there since we didn't remove it, will also be added to the code-generation phase.

Copy link
Member

Choose a reason for hiding this comment

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

Got you. We don't necessary need to check it in. This is minor and thanks for explaination

@zw0610
Copy link
Member Author

zw0610 commented Jul 4, 2021

Can we stop using go mod vendor mode. vendor changes are very big and it distract attention from major code changes.
(This PR is so large due to massive vendor files and I can not even open it)

I think it's a great idea. Let me give a try.

Please take a look at the updated commits, where I removed vendor.

@zw0610 zw0610 force-pushed the apis branch 2 times, most recently from 3e771de to c153e32 Compare July 4, 2021 12:26
@Jeffwan
Copy link
Member

Jeffwan commented Jul 4, 2021

@zw0610 Sorry I didn't clarify vendor stuff clearly. I mean in our PR, let's try not to bring these changes even it's not synced. this makes sure the changes are clean and conflicts are easy to resolve when we merge codes back to master.

We can remove vendor later after all codes are in master.

@Jeffwan
Copy link
Member

Jeffwan commented Jul 5, 2021

Thanks. Great. It looks good to me

@Jeffwan
Copy link
Member

Jeffwan commented Jul 5, 2021

/lgtm

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Jeffwan
To complete the pull request process, please assign johnugeorge after the PR has been reviewed.
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:

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

@Jeffwan Jeffwan merged commit b24f343 into kubeflow:all-in-one-operator Jul 5, 2021
Jeffwan pushed a commit to Jeffwan/tf-operator that referenced this pull request Jul 5, 2021
Jeffwan pushed a commit that referenced this pull request Jul 5, 2021
@Jeffwan
Copy link
Member

Jeffwan commented Jul 11, 2021

part of kubeflow/common#138 #1298

Jeffwan pushed a commit that referenced this pull request Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants