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

feat(sdk): Implement Registry client #7597

Merged
merged 35 commits into from
May 10, 2022
Merged

feat(sdk): Implement Registry client #7597

merged 35 commits into from
May 10, 2022

Conversation

chongyouquan
Copy link
Contributor

@chongyouquan chongyouquan commented Apr 22, 2022 •
edited
Loading

Description of your changes:
Initial PR for registry client.
Does not yet include support for:

  • config files
  • protos (current returned format is JSON)

Checklist:

@chongyouquan chongyouquan requested review from IronPan and removed request for connor-mccarthy April 22, 2022 20:59
Arhell
Arhell reviewed Apr 24, 2022
View reviewed changes
Copy link
Member

@Arhell Arhell left a comment

Choose a reason for hiding this comment

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

/assign @chensun

@chongyouquan chongyouquan requested a review from hilcj April 25, 2022 20:10
@chongyouquan
Copy link
Contributor Author

/retest

connor-mccarthy
connor-mccarthy requested changes Apr 25, 2022
View reviewed changes
Copy link
Member

@connor-mccarthy connor-mccarthy left a comment

Choose a reason for hiding this comment

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

Hi You Quan,

Thanks for this! I have not yet reviewed/tested the functionality. In addition to the line-level notes, here are some high-level comments:

  1. Can you please apply the code style guidelines in the sdk README?

  2. Can you ensure tests are passing?

  3. Can you please use the PR title naming convention described here? For this PR, the title should be feat(sdk): ….

  4. Do you have a test environment for this where we can smoke test these features?

Thanks again!
Connor

sdk/python/kfp/registry/client.py Outdated Show resolved Hide resolved
sdk/python/kfp/registry/__init__.py Outdated Show resolved Hide resolved
sdk/python/kfp/registry/client.py Outdated Show resolved Hide resolved
sdk/python/kfp/registry/client.py Outdated Show resolved Hide resolved
sdk/python/kfp/registry/client.py Outdated Show resolved Hide resolved
sdk/python/kfp/registry/client.py Outdated Show resolved Hide resolved
sdk/python/kfp/registry/client.py Outdated Show resolved Hide resolved
sdk/python/kfp/registry/client.py Outdated Show resolved Hide resolved
sdk/python/kfp/registry/client_test.py Outdated Show resolved Hide resolved
sdk/python/kfp/registry/client_test.py Outdated Show resolved Hide resolved
@google-oss-prow google-oss-prow bot added size/XL and removed size/L labels Apr 25, 2022
@chongyouquan chongyouquan changed the title feat(sdk/python/kfp): Implement Registry client feat(sdk): Implement Registry client Apr 25, 2022
@chongyouquan chongyouquan requested a review from IronPan May 3, 2022 23:01
@chongyouquan
Copy link
Contributor Author

Thanks for addressing the feedback You Quan! We’re working on improving the discoverability of the contribution guidelines, so thanks for working through the feedback.

Overall, this looks great. Just a few more nitpick comments (apologies for missing some of these on the first pass).

It may also make sense to have someone with a better sense of the intended functionality review, but from my review of the Colab notebook, it seems to work great.

A few high-level things:

  1. Can you be sure to run mypy over the code? mypy sdk/python/kfp/registry/
  2. Can you be sure to run docformatter over the code? docformatter sdk/python/kfp/registry/*.py
  3. Can you rename the files to registry_client.py and registry_client_test.py? Just to be consistent with the name of the class.

Thanks for the comments!

  1. Done.
  2. Done.
  3. Done.

@chongyouquan
Copy link
Contributor Author

/retest

connor-mccarthy
connor-mccarthy reviewed May 4, 2022
View reviewed changes
Copy link
Member

@connor-mccarthy connor-mccarthy left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks, @chongyouquan!

sdk/python/kfp/registry/registry_client.py Outdated Show resolved Hide resolved
@google-oss-prow google-oss-prow bot added the lgtm label May 4, 2022
@chensun
Copy link
Member

chensun commented May 4, 2022

/lgtm

Great work, thanks!

@chongyouquan Can you please add a release note entry here: https://github.com/kubeflow/pipelines/blob/master/sdk/RELEASE.md#major-features-and-improvements

@google-oss-prow google-oss-prow bot removed the lgtm label May 5, 2022
@chongyouquan
Copy link
Contributor Author

/lgtm

Great work, thanks!

@chongyouquan Can you please add a release note entry here: https://github.com/kubeflow/pipelines/blob/master/sdk/RELEASE.md#major-features-and-improvements

Done.

connor-mccarthy
connor-mccarthy reviewed May 5, 2022
View reviewed changes
sdk/RELEASE.md Outdated Show resolved Hide resolved
@connor-mccarthy
Copy link
Member

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label May 5, 2022
@connor-mccarthy
Copy link
Member

Thanks, @chongyouquan!

@google-oss-prow google-oss-prow bot removed the lgtm label May 9, 2022
@chensun
Copy link
Member

chensun commented May 10, 2022

/lgtm
/approve

Thanks!

@google-oss-prow google-oss-prow bot added the lgtm label May 10, 2022
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

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 25e4c58 into kubeflow:master May 10, 2022
jagadeeshi2i pushed a commit to jagadeeshi2i/pipelines that referenced this pull request May 11, 2022
@chongyouquan
feat(sdk): Implement Registry client (kubeflow#7597)
8e64900 
* Implement registry client

* Update registry client code

* Add test skeleton

* Add some tests

* Update code

* add tests

* update tests

* update tests

* Rename Client -> RegistryClient

* Update wrt comments

* add type annotations

* fix renaming in __init__.py

* remove unused imports

* extract host variable in test

* format using yapf

* remove locals and use arg keywords

* remove json conversion

* fix header

* write bytes when downloading file

* fix create_tag; fix tests

* fix request_body for update_tag and create_tag using json.dumps

* simply return json for delete_tag

* rename files

* format files

* update return types and format double quotes

* add comments and format files

* add todos

* update credentials and change open to use context

* format using yapf

* move request into context

* Update comments

* Update release notes

* Update release notes
abaland pushed a commit to abaland/pipelines that referenced this pull request May 29, 2022
@chongyouquan @abaland
feat(sdk): Implement Registry client (kubeflow#7597)
a0049d8 
* Implement registry client

* Update registry client code

* Add test skeleton

* Add some tests

* Update code

* add tests

* update tests

* update tests

* Rename Client -> RegistryClient

* Update wrt comments

* add type annotations

* fix renaming in __init__.py

* remove unused imports

* extract host variable in test

* format using yapf

* remove locals and use arg keywords

* remove json conversion

* fix header

* write bytes when downloading file

* fix create_tag; fix tests

* fix request_body for update_tag and create_tag using json.dumps

* simply return json for delete_tag

* rename files

* format files

* update return types and format double quotes

* add comments and format files

* add todos

* update credentials and change open to use context

* format using yapf

* move request into context

* Update comments

* Update release notes

* Update release notes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@connor-mccarthy connor-mccarthy connor-mccarthy requested changes

@Arhell Arhell Arhell left review comments

@chensun chensun Awaiting requested review from chensun

@hilcj hilcj Awaiting requested review from hilcj

@IronPan IronPan Awaiting requested review from IronPan

Assignees

@chensun chensun

@connor-mccarthy connor-mccarthy

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

Successfully merging this pull request may close these issues.
5 participants
@chongyouquan @chensun @connor-mccarthy @IronPan @Arhell