feat: Upload namespaced pipeline definitions. Part of #4197
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:
- Fix backend authentication for upload requests in order to properly authenticate users to upload pipeline definitions in specific namespaces
- Update the swagger definitions accordingly for the upload APIs and generate the code for the relevant clients
- Extend the corresponding Python wrappers with a namespace field
Refs kubeflow/pipelines#4197
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.
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.
/ok-to-test
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.
@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!
I heard that @Linchin is also reviewing API stuff
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 Unfortunately no since the meeting is at 2 am in the night my time :(
[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.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
/retest
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 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.
@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.
Wondering if there is any blockers or updates for this PR?
@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 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
- [Host] Start recording
- [StefanoFioravanzo] Please review https://github.com/kubeflow/pipelines/pull/8196 such that we can continue with the UI implementation
- [Connor McCarthy] on [External] Support for Platform-specific Pipeline Runtime Features
@juliusvonkohout Thanks a lot for mentioning this in the meeting agenda. Hope I manage to attend the meeting!
@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 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
- merge #8511
- release
kfp-server-api - 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: 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
- merge #8511
- release
kfp-server-api- 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.