feat(SDK)!: SDK client v2beta1 API integration. Fixes #8706 #9112
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Fixes #8706. Test failures are expected because we haven't released |
chensun
changed the title
| @@ -495,16 +491,16 @@ def get_pipeline_id(self, name: str) -> Optional[str]: | |||
| """ | |||
| pipeline_filter = json.dumps({ | |||
| 'predicates': [{ | |||
| 'op': _FILTER_OPERATIONS['EQUALS'], | |||
| 'key': 'name', | |||
| 'operation': _FILTER_OPERATIONS['EQUALS'], | |||
There was a problem hiding this comment.
Would the key change from 'op' to 'operation' be a breaking change?
There was a problem hiding this comment.
Unfortunately yes, this is a breaking change. I guess when we made the API changes, we missed that SDK users could pass in this as a string.
Technically we could avoid breaking SDK users by loading the JSON back to a dict and do a string replacement on behalf of the user, but I feel a bit hesitate to introduce such a hack that alters user provided value. Hopefully this breakage would be easy to discover and easy to fix. WDYT?
There was a problem hiding this comment.
Agreed that this is a very minor breaking change. I'm okay with not fixing. It will be easy to add that hack if it turns out to be a pain point for users. Plus, we don't promise backward compatibility from the main namespace to KFP SDK v2 anyway and the client is associate most with the main namespace in KFP v1.
|
|
||
| :: | ||
|
|
||
| json.dumps({ | ||
| "predicates": [{ | ||
| "op": _FILTER_OPERATIONS["EQUALS"], | ||
| "key": "name", | ||
| "operation": "EQUALS", |
There was a problem hiding this comment.
Curious: why remove _FILTER_OPERATIONS here?
There was a problem hiding this comment.
This is in the docstring part, and _FILTER_OPERATIONS isn't exposed, the example is not really runnable--it needs to be at least kfp.client._FILTER_PERATIONS[...] to be runnable.
Also the API works with either the enum name or the enum value, if users has to know the key EQUALS, there's really no point for them to convert the key to its enum value here.
|
/retest |
The test was expected to pass with the latest backend release, but somehow the test cluster was not updated following kubeflow/testing#1026 |
| output_format, | ||
| ) | ||
|
|
||
|
|
||
| @recurring_run.command() | ||
| @click.argument('job-id') | ||
| @click.argument('recurring-run-id') |
There was a problem hiding this comment.
Chose to break the CLI interface because it's painful to support both old and new parameters in the code path, and I expect CLI users usually use it in a more interactive way, the cli helper makes it easier to discover the parameter changes. Thoughts? @connor-mccarthy
There was a problem hiding this comment.
SGTM. What do you think about adding a section to the website docs beneath this one? https://github.com/kubeflow/website/blob/2ca1e2cf634cdd5311f68d06d25674aaf084a87b/content/en/docs/components/pipelines/v2/migration.md#cli-output-change
Probably would only need a sentence or two and I don't think we need to enumerate every break.
Could also be good to mention the new canonical name "recurring run".
There was a problem hiding this comment.
SG. Will follow up on the documentation changes.
chensun
changed the title
chensun
changed the title
|
@chensun: 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. |
…ubeflow#9112) * SDK client v2beta1 API integration. * fix unit tests * create alias for use of methods and fields referencing `job`.` * fix cli * fix pipeline list-versions * more cli fixes * client fix
Description of your changes:
Migrate SDK client to use v2beta1 API.
Manually tested all public method with most commonly used arguments.
Checklist: