feat(sdk): Add version description optionally to Py SDK. Part of #6256 #6472
Conversation
|
Hi @casassg. 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 Once the patch is verified, the new status will be reflected by the 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. |
|
@Bobgy should we bump up the kfp-server-api dep here to make sure it doesnt break when description is released? or should I add a better warning if it fails? |
Hi @casassg, sorry for late reply. I think a better warning is more appropriate, because users' kfp-server-api should always be the same version as their KFP backend. Forcing users who upgraded their SDKs to also upgrade the kfp-server-api version is not a good practice. cc @chensun @capri-xiyue what are your thoughts? |
I think warning makes more sense. |
casassg
force-pushed
the
gcasassaez/fix-kdp-sdk-description
branch
from
cf0dd67
to
f38ecb3
Compare
|
Use try/except to do backwards compatible support. I could use if/else statement based on server version, but it may make code more complex. Happy to implement though! |
sdk/python/kfp/_client.py
Outdated
| pipeline_package_path, **kwargs) | ||
| except kfp_server_api.exceptions.ApiTypeError as e: | ||
| # ToDo: Remove this once we drop support for kfp_server_api < 1.7 | ||
| if 'description' in e.message: |
There was a problem hiding this comment.
nit: I wonder if there could be a more specific and reliable check.
There was a problem hiding this comment.
I added a bit more of specific. But it's hard since the arguments accepted are just a local variable of the function generated by OpenAPI spec. Open to alternatives
|
/ok-to-test |
|
/lgtm |
sdk/python/kfp/_client.py
Outdated
| except kfp_server_api.exceptions.ApiTypeError as e: | ||
| # ToDo: Remove this once we drop support for kfp_server_api < 1.7 | ||
| if 'description' in e.message: | ||
| logging.warning( |
There was a problem hiding this comment.
I am not sure of what others think, but my understanding is to just throw this error message.
Because it's better the user upgrades KFP server api when they see this error.
There was a problem hiding this comment.
Yes, on a second thought, error might be better.
There was a problem hiding this comment.
Adding a NotImplementedError?
|
(sorry folks, went on PTO to visit back home for 2 weeks, will try to get to this early next week. if someone is available before then please don't hesitate to take it from me!) |
|
Also, note that until 19b3de4 gets released, we don't have a public version that supports this. This is due to me forgetting about the Assumed in wording it's the 1.7.1 release, but lmk if that's not the case. |
|
/lgtm @Bobgy do you want to take a look as well? |
|
also, is there a release for KFP upcoming for KF 1.4? |
|
rebumping this up @Bobgy |
|
/approve |
|
[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 |
Description of your changes: