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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

therc
Copy link

@therc therc commented Aug 16, 2024

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:

  1. Job -> RecurringRun
  2. adapt code from the 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 we consume: "Api" -> "V2beta1"

Checklist:

Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign chensun for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@gregsheremeta
Copy link
Contributor

/ok-to-test

@therc
Copy link
Author

therc commented Aug 16, 2024

This was tested with

kfp                       2.8.0              pyhd8ed1ab_0    conda-forge
kfp-pipeline-spec         0.3.0              pyhd8ed1ab_0    conda-forge
kfp-server-api            2.0.1              pyhd8ed1ab_0    conda-forge

@hbelmiro
Copy link
Contributor

/rerun-all

@therc
Copy link
Author

therc commented Aug 19, 2024

/retest

@therc
Copy link
Author

therc commented Aug 19, 2024

/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 <[email protected]>
@therc
Copy link
Author

therc commented Aug 19, 2024

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.

@gregsheremeta
Copy link
Contributor

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?

@hbelmiro
Copy link
Contributor

/rerun-all

@hbelmiro
Copy link
Contributor

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?

@gregsheremeta yes: #10981

@therc
Copy link
Author

therc commented Aug 21, 2024

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!

@hbelmiro
Copy link
Contributor

cc @chensun

@therc
Copy link
Author

therc commented Sep 16, 2024

Ping?

@gregsheremeta
Copy link
Contributor

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.

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants