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

feat(backend): preserve querystring in pipeline root (fixes #10318) #10319

Merged
merged 4 commits into from
Dec 19, 2023
Merged

feat(backend): preserve querystring in pipeline root (fixes #10318) #10319

merged 4 commits into from
Dec 19, 2023

Conversation

TobiasGoerke
Copy link
Contributor

The kfp-driver now recognizes query params and assembles bucket names correctly.

See #10318

Copy link

google-cla bot commented Dec 14, 2023

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-oss-prow google-oss-prow bot added size/M and removed size/S labels Dec 14, 2023
@TobiasGoerke TobiasGoerke changed the title WIP: feat(backend): preserve querystring in pipeline root (fixes #10318) feat(backend): preserve querystring in pipeline root (fixes #10318) Dec 14, 2023
@TobiasGoerke
Copy link
Contributor Author

Using custom S3 endpoints now works for me.

Ready for review!

Copy link
Member

@Tomcli Tomcli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@Tomcli
Copy link
Member

Tomcli commented Dec 14, 2023

/cc @chensun

@google-oss-prow google-oss-prow bot requested a review from chensun December 14, 2023 20:59
@TobiasGoerke TobiasGoerke changed the title feat(backend): preserve querystring in pipeline root (fixes #10318) WIP: feat(backend): preserve querystring in pipeline root (fixes #10318) Dec 15, 2023
* feat: remove query string from URIs
@google-oss-prow google-oss-prow bot removed the lgtm label Dec 15, 2023
@TobiasGoerke
Copy link
Contributor Author

TobiasGoerke commented Dec 15, 2023

Thanks @Tomcli.
I pushed another minor change, see here.

This causes filenames to be stored without the query string and the artifacts in the pipeline yaml don't contain the connection string.
It's more a cosmetic change but also lets the pipelines ui correctly download the artifacts.

@TobiasGoerke TobiasGoerke changed the title WIP: feat(backend): preserve querystring in pipeline root (fixes #10318) feat(backend): preserve querystring in pipeline root (fixes #10318) Dec 15, 2023
@@ -1078,12 +1079,6 @@ func provisionOutputs(pipelineRoot, taskName string, outputsSpec *pipelinespec.C
return outputs
}

func generateOutputURI(root, artifactName string, taskName string) string {
// we cannot path.Join(root, taskName, artifactName), because root
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since now you new function is similar to the old generateOutputURI function, it make sense to bring these comments to the changes you have at line 1065 or just update the generateOutputURI function.

Copy link

@TobiasGoerke: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
kubeflow-pipelines-samples-v2 e2c34bf link false /test kubeflow-pipelines-samples-v2

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. I understand the commands that are listed here.

Copy link
Member

@Tomcli Tomcli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @TobiasGoerke
/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Dec 18, 2023
@chensun
Copy link
Member

chensun commented Dec 18, 2023

/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

The full list of commands accepted by this bot can be found here.

The pull request process is described 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 google-oss-prow bot merged commit 9a30612 into kubeflow:master Dec 19, 2023
5 of 6 checks passed
@TobiasGoerke TobiasGoerke deleted the feat-pipeline-root-with-query branch January 16, 2024 19:28
stijntratsaertit pushed a commit to stijntratsaertit/kfp that referenced this pull request Feb 16, 2024
…10318) (kubeflow#10319)

* feat: preserve querystring in pipeline root

* refactor: create AppendToPipelineRoot
Also apply to client.go

* feat: remove query string from URIs (kubeflow#1)

* feat: remove query string from URIs

* refactor(GenerateOutputURI): move and preserve comments
@thesuperzapper
Copy link
Member

@chensun @Tomcli @connor-mccarthy is there any chance we can cut a 2.0.6 patch release that has this PR included?

It's pretty important, because external object stores can't be used with KFP v2 without query parameters (in the pipeline root URI).

This will be less important if we ever get #9689 resolved, but that's a longer term fix.

thesuperzapper added a commit to thesuperzapper/kubeflow-pipelines that referenced this pull request Mar 23, 2024
@RonaldFletcher
Copy link

Thanks @Tomcli. I pushed another minor change, see here.

This causes filenames to be stored without the query string and the artifacts in the pipeline yaml don't contain the connection string. It's more a cosmetic change but also lets the pipelines ui correctly download the artifacts.

How to use custom S3 endpoints ?Can you give an example? @TobiasGoerke

@TobiasGoerke
Copy link
Contributor Author

Thanks @Tomcli. I pushed another minor change, see here.
This causes filenames to be stored without the query string and the artifacts in the pipeline yaml don't contain the connection string. It's more a cosmetic change but also lets the pipelines ui correctly download the artifacts.

How to use custom S3 endpoints ?Can you give an example? @TobiasGoerke

For v2 pipelines, you'll need to set the kfp-launcher ConfigMap's data.defaultPipelineRoot to your desired endpoint. Depending on your endpoint, you may now also need to set some env vars for authentication in your pipeline pods and artifacts proxies etc.

thesuperzapper added a commit to deployKF/kubeflow-pipelines that referenced this pull request Apr 14, 2024
thesuperzapper added a commit to thesuperzapper/kubeflow-pipelines that referenced this pull request Apr 21, 2024
thesuperzapper added a commit to thesuperzapper/kubeflow-pipelines that referenced this pull request Apr 21, 2024
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.

5 participants