-
Notifications
You must be signed in to change notification settings - Fork 702
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
add XGBoostJob api #1286
Conversation
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. |
Can we stop using go mod vendor mode. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
I think it's a great idea. Let me give a try. Please take a look at the updated commits, where I removed |
3e771de
to
c153e32
Compare
@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. |
Thanks. Great. It looks good to me |
/lgtm |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Jeffwan 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 |
part of kubeflow/common#138 #1298 |
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