pipelines icon indicating copy to clipboard operation
pipelines copied to clipboard

feat: Upload namespaced pipeline definitions. Part of #4197

Open pangiann opened this issue 3 years ago • 7 comments

This PR is part of the overarching idea to support namespaced pipeline definitions. The whole idea is thoroughly analyzed and exposed in this design doc: https://docs.google.com/document/d/1fM4y2L1IVqVj-iiNjYFRRktdCh7FQXgU2XpaYLaqt-A/edit?resourcekey=0-kd5loyP7w3PBD0ug6ECmLQ#

You will notice that in the first commit we autogenerate the Go and Python clients. We did this because commit https://github.com/kubeflow/pipelines/commit/f6044062388855e4e0fdf58d9de848631f5094cf reverted the change of cf1b873a9aec8f2eb733ff2fa8707a0de2fcf438 for some reason. We didn't find any rationale for this somewhere, it looks like it was not pushed with a PR.

Objective of this PR is to:

  1. Fix backend authentication for upload requests in order to properly authenticate users to upload pipeline definitions in specific namespaces
  2. Update the swagger definitions accordingly for the upload APIs and generate the code for the relevant clients
  3. Extend the corresponding Python wrappers with a namespace field

Refs kubeflow/pipelines#4197

pangiann avatar Aug 25 '22 13:08 pangiann

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Aug 25 '22 13:08 google-cla[bot]

Hi @pangiann. 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 25 '22 13:08 google-oss-prow[bot]

/ok-to-test

elikatsis avatar Aug 29 '22 07:08 elikatsis

Will you present this at the next KFP meeting Wednesday, 14. September⋅19:00 bis 19:40 CEST https://www.google.com/url?q=http://bit.ly/kfp-meeting-notes&sa=D&source=calendar&usd=2&usg=AOvVaw3fFVzrVbaK0EXNBM5blCu1 ? This is most likely the best way to get it reviewed and merged.

juliusvonkohout avatar Sep 02 '22 08:09 juliusvonkohout

@jlyaoyuli @zijianjoy @chensun Hi! Could someone take a look at this PR? We are waiting for this one to get merged so we can submit the frontend PR to implement the UX described here https://docs.google.com/document/d/1fM4y2L1IVqVj-iiNjYFRRktdCh7FQXgU2XpaYLaqt-A/edit?resourcekey=0-kd5loyP7w3PBD0ug6ECmLQ#

Let me know if you need any help!

StefanoFioravanzo avatar Sep 21 '22 06:09 StefanoFioravanzo

I heard that @Linchin is also reviewing API stuff

juliusvonkohout avatar Sep 21 '22 07:09 juliusvonkohout

Will you present this at the next KFP meeting Wednesday, 14. September⋅19:00 bis 19:40 CEST https://www.google.com/url?q=http://bit.ly/kfp-meeting-notes&sa=D&source=calendar&usd=2&usg=AOvVaw3fFVzrVbaK0EXNBM5blCu1 ? This is most likely the best way to get it reviewed and merged.

@StefanoFioravanzo Since there was no feedback in this PR here or the design document (since may 16) i would like to know whether you can make it to the next KFP meeting in around 36 hours.

juliusvonkohout avatar Sep 27 '22 11:09 juliusvonkohout

@juliusvonkohout Unfortunately no since the meeting is at 2 am in the night my time :(

StefanoFioravanzo avatar Sep 27 '22 14:09 StefanoFioravanzo

[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 by writing /assign @chensun in a comment. 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 Sep 29 '22 07:09 google-oss-prow[bot]

/retest

elikatsis avatar Oct 13 '22 11:10 elikatsis

All tests are green, this PR should be good to go! :rocket: Pinging again the reviewers that we have already mentioned above as I don't know who is going to take this: @jlyaoyuli @zijianjoy @chensun @Linchin

cc @juliusvonkohout @StefanoFioravanzo

elikatsis avatar Oct 13 '22 15:10 elikatsis

@elikatsis if this is not revieved and merged in the next 10 days you need to join the next nightly KFP meeting in around 10 days to get more direct attention.

juliusvonkohout avatar Oct 14 '22 09:10 juliusvonkohout

@elikatsis if this is not revieved and merged in the next 10 days you need to join the next nightly KFP meeting in around 10 days to get more direct attention.

It's very difficult for us to attend that meeting as it's in the middle of the night. @zijianjoy confirmed that the Google folks have approved the design and approach, so I hope they will get back to us soon and merge this cc @jlyaoyuli @chensun @Linchin

Note that we have a few PRs coming to update the UI (as proposed in the doc), so the sooner we unblock this the higher the chances we get this done in time for the next KFP release.

StefanoFioravanzo avatar Oct 19 '22 15:10 StefanoFioravanzo

Wondering if there is any blockers or updates for this PR?

maganaluis avatar Nov 01 '22 18:11 maganaluis

@zijianjoy @jlyaoyuli @chensun @Linchin Hi! We would love to merge this PR, as it has been sitting here for quite some time. I know there are some conflicts, but if you tell us you can get some time to merge, we will resolve the conflicts within the day.

StefanoFioravanzo avatar Nov 14 '22 18:11 StefanoFioravanzo

@StefanoFioravanzo There are two alternating slots for the pipelines meeting. One is perfectly suitable for Europe. The next one is 7th December 19.00-19:40 German time, so if you are based in Europe that should be suitable for you.

Notes & Agenda: http://bit.ly/kfp-meeting-notes Join at: https://meet.google.com/jkr-dupp-wwm

I have added the following to the agenda for you

2022-12-07

juliusvonkohout avatar Nov 24 '22 14:11 juliusvonkohout

@juliusvonkohout Thanks a lot for mentioning this in the meeting agenda. Hope I manage to attend the meeting!

StefanoFioravanzo avatar Nov 25 '22 08:11 StefanoFioravanzo

@StefanoFioravanzo At a quick glance, the backend changes and sdk changes look good to me. To move forward, please rebase and resolve merge conflicts. Also, I'd recommend separate the sdk and backend changes into separate PRs. Thanks!

chensun avatar Nov 28 '22 19:11 chensun

@chensun Thank you for your reply!

I created https://github.com/kubeflow/pipelines/pull/8511 with the backend changes. I've also prepared the SDK PR but we need to

  1. merge #8511
  2. release kfp-server-api
  3. use (2) in the SDK requirements

I'll open the SDK PR as a draft soon (https://github.com/kubeflow/pipelines/pull/8512) until we've completed the above items. I'm closing this one.

/close

elikatsis avatar Nov 30 '22 16:11 elikatsis

@elikatsis: Closed this PR.

In response to this:

@chensun Thank you for your reply!

I created https://github.com/kubeflow/pipelines/pull/8511 with the backend changes. I've also prepared the SDK PR but we need to

  1. merge #8511
  2. release kfp-server-api
  3. use (2) in the SDK requirements

I'll open the SDK PR as a draft soon until we've completed the above items. I'm closing this one.

/close

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 Nov 30 '22 16:11 google-oss-prow[bot]