pipelines icon indicating copy to clipboard operation
pipelines copied to clipboard

fix(sdk): Fix deprecated client to work with kfp-server-api 2.0.1

Open therc opened this issue 1 year ago • 13 comments

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:

therc avatar Aug 16 '24 20:08 therc

[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

google-oss-prow[bot] avatar Aug 16 '24 20:08 google-oss-prow[bot]

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.

google-oss-prow[bot] avatar Aug 16 '24 20:08 google-oss-prow[bot]

/ok-to-test

gregsheremeta avatar Aug 16 '24 20:08 gregsheremeta

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

therc avatar Aug 16 '24 20:08 therc

/rerun-all

hbelmiro avatar Aug 16 '24 21:08 hbelmiro

/retest

therc avatar Aug 19 '24 19:08 therc

/test all

therc avatar Aug 19 '24 19:08 therc

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.

therc avatar Aug 19 '24 21:08 therc

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 avatar Aug 19 '24 21:08 gregsheremeta

/rerun-all

hbelmiro avatar Aug 19 '24 22:08 hbelmiro

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: https://github.com/kubeflow/pipelines/issues/10981

hbelmiro avatar Aug 19 '24 22:08 hbelmiro

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!

therc avatar Aug 21 '24 18:08 therc

cc @chensun

hbelmiro avatar Aug 22 '24 12:08 hbelmiro

Ping?

therc avatar Sep 16 '24 19:09 therc

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.

gregsheremeta avatar Sep 19 '24 12:09 gregsheremeta

/reopen

hbelmiro avatar Dec 11 '24 11:12 hbelmiro

@hbelmiro: Reopened this PR.

In response to this:

/reopen

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.

google-oss-prow[bot] avatar Dec 11 '24 11:12 google-oss-prow[bot]

@therc the CI is working now. Can you please rebase?

hbelmiro avatar Dec 11 '24 11:12 hbelmiro

I imagine that we should remove the kfp.deprecated namespace if we have decided to not fix it, because it literally does not work without this PR.

thesuperzapper avatar Mar 04 '25 13:03 thesuperzapper

Sorry, I should have closed this PR. The deprecated namespace was removed without much explanation in November in https://github.com/kubeflow/pipelines/pull/11366

therc avatar Mar 04 '25 14:03 therc