fix(sdk): Add in filter to list_pipeline_versions SDK method #7223
Conversation
There was a problem hiding this comment.
Thanks @alexlatchford for the fix!
Other than the nitpicks I left in comments, can you please add a release note for this PR?
sdk/python/kfp/_client.py
Outdated
| @@ -1401,7 +1401,8 @@ def list_pipeline_versions( | |||
| pipeline_id: str, | |||
| page_token: str = '', | |||
| page_size: int = 10, | |||
| sort_by: str = '' | |||
| sort_by: str = '', | |||
| filter: str = None | |||
There was a problem hiding this comment.
nit: if the default value is None, then it should be :Optional[str] = None
BTW, were you able to test this with both default None and a real value?
There was a problem hiding this comment.
To be honest I'm just copying the pattern established in this earlier PR that introduced the filter argument 😅 I can certainly confirm it works with the filter set as we're running this within the Zillow-specific fork of the kfp-sdk we maintain 😄
I've fixed this type hint (and updated the type hints for all the list_* client method arguments too), I'm happy to test out with no filter if needs be. Let me know if you want me to try it out or if you're happy without :)
There was a problem hiding this comment.
Yes, this is more of a style thing, it doesn't affect runtime behavior. Our code may have many other styling issues, and we want to be more compliance to the style for newly added code at least.
https://google.github.io/styleguide/pyguide.html#3195-nonetype
| @@ -1410,6 +1411,8 @@ def list_pipeline_versions( | |||
| page_token: Token for starting of the page. | |||
| page_size: Size of the page. | |||
| sort_by: One of 'field_name', 'field_name desc'. For example, 'name desc'. | |||
| filter: A url-encoded, JSON-serialized Filter protocol buffer | |||
There was a problem hiding this comment.
Is it possible to give an example of what a filter value could be?
There was a problem hiding this comment.
I've added an example to all the affected areas. Looks like the URL encoding actually happens under the hood as a part of the REST client in kfp-server-api so technically you don't need to provide a URL encoded filter, I can update the docstring if that works?
Also in the comment I'm referring to a private dictionary interface which holds the mapping between op name and index, not ideal but happy to make that interface public if you want? We could expose it via from kfp import FILTER_OPERATIONS or something like that?
There was a problem hiding this comment.
I think that's good enough for now. Thanks!
164c901 to
e6aaffe
Compare
sdk/python/kfp/_client.py
Outdated
| @@ -1401,7 +1401,8 @@ def list_pipeline_versions( | |||
| pipeline_id: str, | |||
| page_token: str = '', | |||
| page_size: int = 10, | |||
| sort_by: str = '' | |||
| sort_by: str = '', | |||
| filter: str = None | |||
There was a problem hiding this comment.
To be honest I'm just copying the pattern established in this earlier PR that introduced the filter argument 😅 I can certainly confirm it works with the filter set as we're running this within the Zillow-specific fork of the kfp-sdk we maintain 😄
I've fixed this type hint (and updated the type hints for all the list_* client method arguments too), I'm happy to test out with no filter if needs be. Let me know if you want me to try it out or if you're happy without :)
| @@ -1410,6 +1411,8 @@ def list_pipeline_versions( | |||
| page_token: Token for starting of the page. | |||
| page_size: Size of the page. | |||
| sort_by: One of 'field_name', 'field_name desc'. For example, 'name desc'. | |||
| filter: A url-encoded, JSON-serialized Filter protocol buffer | |||
There was a problem hiding this comment.
I've added an example to all the affected areas. Looks like the URL encoding actually happens under the hood as a part of the REST client in kfp-server-api so technically you don't need to provide a URL encoded filter, I can update the docstring if that works?
Also in the comment I'm referring to a private dictionary interface which holds the mapping between op name and index, not ideal but happy to make that interface public if you want? We could expose it via from kfp import FILTER_OPERATIONS or something like that?
|
@alexlatchford: The following test failed, say
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. |
|
/lgtm Thanks! |
|
[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 |
The test failure is not related to this change. I'll ignore it and force merge the change. |
…w#7223) * AIP-5661: Add in filter to list_pipeline_versions SDK method * AIP-5661: Update type hints + docstrings to conform to standard. Include message in release notes
Description of your changes:
Currently when using
kfp pipeline list-versions --filter "..." $pipeline_idit breaks with:Looks like adding the
filterwas just missed when it was added into the SDK :)