feat: Upload namespaced pipeline definitions. Part of #4197 #8196
Conversation
|
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 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. |
|
/ok-to-test |
pangiann
force-pushed
the
feature-pangiann-upload-namespaced-pipelines
branch
from
3911ab4 to
89e5090
Compare
|
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 ? |
|
@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 |
@StefanoFioravanzo |
|
@juliusvonkohout Unfortunately no since the meeting is at 2 am in the night my time :( |
elikatsis
force-pushed
the
feature-pangiann-upload-namespaced-pipelines
branch
from
89e5090 to
c0f8d38
Compare
|
[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 |
Autogenerate Go and Python clients for the updated gRPC-Gateway which got reverted by f604406
Fix the way KFP API server authenticates pipeline upload requests. We leverage 'isAuthenticated()` function which requires proper initialization of the context object to include user identity.
Extend swagger defintion of the pipeline upload API with a namespace parameter in order to support uploading namespaced pipelines.
Autogenerate the updated Go and Python clients after extending the swagger definitions of upload pipeline APIs with a namespace field.
Add a namespace field in the relevant Python wrappers for uploading pipeline definitions.
elikatsis
force-pushed
the
feature-pangiann-upload-namespaced-pipelines
branch
from
c0f8d38 to
17b0140
Compare
|
/retest |
|
All tests are green, this PR should be good to go! 🚀 |
|
@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 I have added the following to the agenda for you 2022-12-07
|
|
@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 #8511 with the backend changes. I've also prepared the SDK PR but we need to
I'll open the SDK PR as a draft soon (#8512) until we've completed the above items. I'm closing this one. /close |
|
@elikatsis: Closed 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. |
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 f604406 reverted the change of cf1b873 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:
Refs #4197