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 clientset for MPIJob, PytorchJob, MXJob, and XGBoostJob #1610

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

tenzen-y
Copy link
Member

@tenzen-y tenzen-y commented Jun 11, 2022

What this PR does / why we need it:
As discussed in here, I moved Kubernetes Custom Resource API codes to the pkg/apis/kubeflow.org/v1 directory to generate Go clientset for MPIJob, PytorchJob, MXJob, and XGBoostJob.

So, I made the following changes:

  1. Move pkg/apis/[mpi|mxnet|pytorch|tensorflow|xgboost]/[v1|validation]/*.go to pkg/apis/kubeflow.org/v1/*.go.
  2. Reduce duplication of codes for defaulting and validation tests.
  3. Add a framework name prefix (ex. MPIJobDefaultPortName) to variable names that conflict between all frameworks.
  4. Regenerate Go clientset, Python SDK, API documentation, and e2e test codes for all frameworks.
  5. Update code to generate swagger file.
  6. Update Readme.

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #1499, #1501, #1586, #1592

Checklist:

  • Docs included if any changes are user facing

@tenzen-y tenzen-y changed the title [WIP] Add clientset for mpijob, pytorchjob, mxjob, and xgboostjob [WIP] Add clientset for MPIJob, PytorchJob, MXJob, and XGBoostJob Jun 11, 2022
@tenzen-y tenzen-y force-pushed the add-clientset branch 3 times, most recently from baac7cb to 230baee Compare June 11, 2022 10:15
@coveralls
Copy link

coveralls commented Jun 11, 2022

Pull Request Test Coverage Report for Build 2482116121

  • 345 of 2153 (16.02%) changed or added relevant lines in 36 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+4.5%) to 41.518%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/apis/kubeflow.org/v1/defaulting_utils.go 34 35 97.14%
pkg/controller.v1/pytorch/initcontainer.go 11 12 91.67%
pkg/controller.v1/tensorflow/util.go 12 13 92.31%
pkg/apis/kubeflow.org/v1/mxnet_defaults.go 30 33 90.91%
pkg/apis/kubeflow.org/v1/tensorflow_defaults.go 36 39 92.31%
pkg/apis/kubeflow.org/v1/xgboost_defaults.go 29 32 90.63%
pkg/controller.v1/pytorch/elastic.go 6 9 66.67%
pkg/apis/kubeflow.org/v1/mpi_defaults.go 0 4 0.0%
pkg/controller.v1/tensorflow/tfjob_controller.go 40 44 90.91%
pkg/controller.v1/register_controller.go 0 5 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/controller.v1/mpi/mpijob_controller.go 2 77.8%
Totals Coverage Status
Change from base Build 2474657252: 4.5%
Covered Lines: 2298
Relevant Lines: 5535

💛 - Coveralls

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@tenzen-y tenzen-y changed the title [WIP] Add clientset for MPIJob, PytorchJob, MXJob, and XGBoostJob Add clientset for MPIJob, PytorchJob, MXJob, and XGBoostJob Jun 11, 2022
@tenzen-y
Copy link
Member Author

@kubeflow/wg-training-leads Please take a look.

@tenzen-y
Copy link
Member Author

/cc @zw0610

@tenzen-y
Copy link
Member Author

I have grouped Go dependencies.

@tenzen-y
Copy link
Member Author

I have updated links to API documentation in README.md.

@tenzen-y
Copy link
Member Author

/cc @johnugeorge @gaocegege

@zw0610
Copy link
Member

zw0610 commented Jun 12, 2022

Thank you very much for your contribution! After the test passed, could you squash multiple commits into one?

@tenzen-y
Copy link
Member Author

Thank you very much for your contribution! After the test passed, could you squash multiple commits into one?

Thanks for your comment! @zw0610
Sure, I will squash all commits into one.

@tenzen-y tenzen-y force-pushed the add-clientset branch 2 times, most recently from b2bf97a to 766813b Compare June 12, 2022 03:11
@tenzen-y
Copy link
Member Author

@zw0610 I have squashed all commits and passed all tests.

@johnugeorge
Copy link
Member

/assign @zw0610
/assign @gaocegege

@johnugeorge
Copy link
Member

@zw0610 @gaocegege Can you please review this so that we can get it in this release?

@zw0610
Copy link
Member

zw0610 commented Jun 13, 2022

LGTM
I went through the non-client parts. @gaocegege Could you also take a look at the clientset & python-client generation?

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

I think it is a breaking change for developers since the API packages are changed from gtihub.com/kubeflow/training-operator/pkg/apis/tensorflow/v1 to gtihub.com/kubeflow/training-operator/pkg/apis/kubeflow.org/v1

We should notice users in some degree

The PR itself LGTM

Thanks for your contribution! 🎉 👍

@tenzen-y
Copy link
Member Author

@zw0610 @gaocegege Thanks for your review of this large PR!

I think it is a breaking change for developers since the API packages are changed from gtihub.com/kubeflow/training-operator/pkg/apis/tensorflow/v1 to gtihub.com/kubeflow/training-operator/pkg/apis/kubeflow.org/v1

We should notice users in some degree

I agree with you.

@zw0610
Copy link
Member

zw0610 commented Jun 14, 2022

this fix is long-overdue. Thank you again for this tremendous work.
/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jun 14, 2022
@gaocegege
Copy link
Member

@johnugeorge Could we merge it?

@johnugeorge
Copy link
Member

Thanks everyone
/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge, tenzen-y

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-prow google-oss-prow bot merged commit bf3787f into kubeflow:master Jun 14, 2022
@tenzen-y tenzen-y deleted the add-clientset branch June 14, 2022 04:32
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.

[Feature] Create a Informer/ClientSet for PyTorch Jobs
5 participants