-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(sdk): Fix deprecated client to work with kfp-server-api 2.0.1 #11109
base: master
Are you sure you want to change the base?
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 |
Hi @therc. 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. |
be08abc
to
8e7d7a1
Compare
/ok-to-test |
This was tested with
|
/rerun-all |
8e7d7a1
to
46dbfa1
Compare
/retest |
46dbfa1
to
e48185f
Compare
/test all |
The deprecated client is currently broken, as it refers to APIs that don't exist anymore. This change makes things less broken, but there might be more fixes needed. Basically: 1. Job -> RecurringRun 2. adapt code from current client that warns for FOO_job() and forwards to FOO_recurring_run() 3. adapt some code from create_recurring_run() 4. version the APIs: "Api" -> "V2beta1" Signed-off-by: Rudi C <rudi@ctrl-labs.com>
e48185f
to
35ece08
Compare
I fixed the YAPF errors, but can't rerun the tests. I guess the failure and/or my new commit made the PR lose its trusted status. |
that's odd. It's still labeled "ok-to-test", but now it says "10 workflows awaiting approval". @hbelmiro, have you ever seen that before? |
/rerun-all |
@gregsheremeta yes: #10981 |
Had to trigger test-run-all-gcpc-modules again, by hand, but it's all green now. Blocked on approvals. How often are SDKs released? sdk/CONTRIBUTING.md doesn't mention anything about what would trigger a version bump. We'd be happy if it included @gregsheremeta's #10913 as well! |
cc @chensun |
Ping? |
I looked it over and it looks fine, but I have 0 experience with v1 or the deprecated stuff ... so ideally @chensun can look at and ack this. I'm inclined to tag it lgtm since it's a deprecated file and the tests pass, but I'll wait for Chen.
PRs would be great. We have several example PRs of adding these things back if that would help guide you. Alternatively, please open issues for the things you need that are still missing. |
/reopen |
@hbelmiro: Reopened this PR. In response to this:
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. |
@therc the CI is working now. Can you please rebase? |
Description of your changes:
"There are two ways of doing things at Google: the deprecated one, and the one that doesn't work yet" (psrc)
The deprecated client is currently broken, as it refers to APIs that don't exist anymore. This change makes things less broken, but there might be more fixes needed.
We had started by trying to upgrade to the pipelines V2 API, but it's still missing several Kubernetes-specific features that we need (affinities, host mounts, shared memory, etc.). Some have PRs in progress, some not even that.
We then tried using the kfp.deprecated code bundled in V2, because staying on kfp 1.8.5 is preventing us from upgrading several of our Conda dependencies: protobuf, grpc, python-kubernetes, etc. Those, in turn, block other upgrades.
Unfortunately, kfp.deprecated is suffering from bit rot, so some surgery is needed.
Basically:
Checklist: