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 pytorch mpi mxnet xgboost clientset and update doc #1586

Closed
wants to merge 0 commits into from

Conversation

41tair
Copy link

@41tair 41tair commented May 5, 2022

What this PR does / why we need it:
add pytorch mpi mxnet xgboost clientset and fix the confict

Which issue(s) this PR fixes
Fixes #1501

@aws-kf-ci-bot
Copy link
Contributor

Hi @41tair. 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.

@@ -16,5 +16,5 @@
// +k8s:openapi-gen=true

// Package v1 is the v1 version of the API.
// +groupName=kubeflow.org
// +groupName=tensorflow.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.

It may be a block change /cc @kubeflow/wg-training-leads @zw0610

Copy link
Member

Choose a reason for hiding this comment

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

Is it auto-generated or manually? I believe we should keep the group name, even in the documentation.

Copy link
Author

Choose a reason for hiding this comment

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

Is it auto-generated or manually? I believe we should keep the group name, even in the documentation.

This is generated manually, and the code-generator does not generate clientset.This is a workaround.More info in #1501

@gaocegege
Copy link
Member

/ok-to-test

@41tair
Copy link
Author

41tair commented May 11, 2022

is there any update? Change the api structure like

#1501
Like other projects, we change the hierarchy to /pkg/apis//<ml_framework_x>_type.go where they all span in same directory meaning they are in same subgroup, then when generating clientset informer, they will be in the same package.

or use this pr modification?

@zw0610
Copy link
Member

zw0610 commented May 12, 2022

is there any update? Change the api structure like

#1501
Like other projects, we change the hierarchy to /pkg/apis//<ml_framework_x>_type.go where they all span in same directory meaning they are in same subgroup, then when generating clientset informer, they will be in the same package.

or use this pr modification?

I prefer changing the hierarchy to /pkg/apis/v1/<ml_framework_x>_type.go, which is more universal.

go.mod Outdated
@@ -1,4 +1,4 @@
module github.com/kubeflow/training-operator
module github.com/41tair/training-operator
Copy link
Contributor

Choose a reason for hiding this comment

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

this modification should be cancelled

@aws-kf-ci-bot
Copy link
Contributor

@41tair: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
kubeflow-training-operator-presubmit 2c43247 link /test kubeflow-training-operator-presubmit

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@johnugeorge
Copy link
Member

Ref: #1592

@johnugeorge
Copy link
Member

@41tair Can you please update?

@tenzen-y
Copy link
Member

tenzen-y commented Jun 5, 2022

@41tair I would like to include this feature next training-operator release. Do you have any time to work on this PR?
If you have only a little time to work on this, I'd like to create another PR.

Thank you for your contribution!

@41tair
Copy link
Author

41tair commented Jun 6, 2022

@41tair I would like to include this feature next training-operator release. Do you have any time to work on this PR?

If you have only a little time to work on this, I'd like to create another PR.

Thank you for your contribution!

I'd like update this pr before Friday,if it will be before this release.

@tenzen-y
Copy link
Member

tenzen-y commented Jun 6, 2022

@41tair I would like to include this feature next training-operator release. Do you have any time to work on this PR?
If you have only a little time to work on this, I'd like to create another PR.
Thank you for your contribution!

I'd like update this pr before Friday,if it will be before this release.

@41tair Thanks for your response.
We need to merge to the master branch by June 15th to include this feature in the next release.

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 41tair

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

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.

7 participants