Correct Runtime Artifact custom path logic/testing. #12402

alyssacgoins · 2025-10-31T13:49:24Z

Description of your changes:
Updates the logic added in #12248 and also adds spec files to compiled workflow and pipeline files in order to correct the pipeline_with_artifact_custom_path test case.

Checklist:

You have signed off your commits
The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

google-oss-prow · 2025-10-31T13:49:28Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

mprahl · 2025-11-03T14:17:46Z

test_data/sdk_compiled_pipelines/valid/critical/pipeline_with_artifact_custom_path.py

+def validate_custom_artifact_path(num: int, out_dataset: Output[Dataset]):
+    with open(out_dataset.path, 'w') as f:
+        f.write(str(2 * num))
+    out_dataset._set_path('/etc/test/file/path')


I thought the user was supposed to call it via out_dataset.set_path('/etc/test/file/path') or out_dataset.custom_path = '/etc/test/file/path'?

The Jira for this task specifies I can call set_path on the output artifact - I took that to mean that when a user calls set_path, it sets the custom path. But I also see both interpretations

@mprahl let me know if you think it makes more sense to update to your comment

@alyssacgoins it was more that it should be set_path and not _set_path to test the way the user would leverage the feature.

mprahl · 2025-11-04T17:21:19Z

test_data/sdk_compiled_pipelines/valid/critical/pipeline_with_artifact_custom_path.py

-    output_artifact_task = generate_artifact()
-    output_artifact_task.output.set_path('/etc/test/file/path')
+    # Generate an artifact, set a custom path and validate the path.
+    task = validate_custom_artifact_path(num=1).set_caching_options(False)


Please add another task that imports the artifact to ensure the correct thing got uploaded and then redownloaded.

mprahl · 2025-11-04T17:21:54Z

test_data/sdk_compiled_pipelines/valid/critical/pipeline_with_artifact_custom_path.py

+def validate_custom_artifact_path(num: int, out_dataset: Output[Dataset]):
+    with open(out_dataset.path, 'w') as f:
+        f.write(str(2 * num))
+    out_dataset.set_path('/etc/test/file/path')


Could you use a writable path for non-root users like something in /tmp?

mprahl · 2025-11-07T16:10:09Z

test_data/sdk_compiled_pipelines/valid/critical/pipeline_with_artifact_custom_path.py

+def component_output_artifact(out_dataset: Output[Dataset]):
+    with open(out_dataset.path, 'w') as f:
+        f.write('Hello, World!')
+    out_dataset.set_path('/tmp/output/dataset')


Call this before writing to out_dataset.path. Then validate_input_artifact should assert the contents are as expected.

Not sure I follow - to clarify, which line should be executed before writing to out_dataset.path?

mprahl · 2025-11-07T23:52:11Z

backend/src/v2/metadata/converter.go

 	}
+	// If runtime artifact is set with a custom path, add this path to the custom properties map.
+	if runtimeArtifact.CustomPath != nil {
+		artifact.CustomProperties["custom_path"] = stringMLMDValue(*runtimeArtifact.CustomPath)


I don't think we should be storing this as a custom property. It's not relevant to the artifact once the artifact is uploaded.

The issue is that metadata_store.Artifact, which I believe is a third-party resource, does not have a custom_path field. If I'm following the logic correctly, type metadata_store.Artifact is uploaded to MLMD - I added custom_path as a property in order to preserve this value upon retrieval from MLMD

mprahl · 2025-11-08T00:05:45Z

backend/src/v2/metadata/converter.go

+	// Retrieve custom_path value from artifact.CustomProperties, if present.
+	var customPathStr string
+	if artifact.CustomProperties != nil {
+		customPath := artifact.CustomProperties["custom_path"]


The custom path flow should be:

Executor sets the custom path property on the output artifact in the executor output

The uploadOutputArtifacts function checks if the custom path is present and uses that to upload instead of the predefined path

The custom path is no longer needed and be ignored and not set on the MLMD artifact.

The issue I found is that, while I modified pipeline_spec.RuntimeArtifact to include the custom_path field, and uploadOutputArtifacts uploads to that custom path, when the MLMD artifact is retrieved with no custom path set on the object, the executor does not know what path to retrieve the file from when launcher_v2.retrieveArtifactPath() executes

mprahl · 2025-11-11T01:07:08Z

test_data/sdk_compiled_pipelines/valid/critical/pipeline_with_artifact_custom_path.py

-        raise ValueError(f"File uri is {input_list.path} but should be {exp_path}.")
+def validate_input_artifact(in_dataset: Input[Dataset]) -> bool:
+    if in_dataset is None:
+        raise ValueError("Input artifact is None.")


Could you please have the test read the input dataset at in_dataset.path and assert it equals Hello, World!?

mprahl · 2025-11-11T01:07:59Z

sdk/python/kfp/dsl/types/artifact_types.py

    def _get_custom_path(self) -> str:
        return self._custom_path

-    def _set_path(self, path: str) -> None:


Optional: It'd be nice if this was kept at its original location to not shadow the original commit history.

mprahl

/approve
/lgtm

Signed-off-by: agoins <[email protected]>

mprahl · 2025-11-11T16:32:46Z

/approve
/lgtm

google-oss-prow · 2025-11-11T16:32:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mprahl

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:

~~OWNERS~~ [mprahl]

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 added the do-not-merge/work-in-progress label Oct 31, 2025

google-oss-prow bot requested review from HumairAK, droctothorpe and hbelmiro October 31, 2025 13:49

google-oss-prow bot added size/L size/XL size/XXL and removed size/L size/XL size/XXL labels Oct 31, 2025

alyssacgoins force-pushed the model-upload-custom-path-fix branch 2 times, most recently from 5afbe00 to a867d8a Compare October 31, 2025 20:26

google-oss-prow bot added size/XL size/L and removed size/L size/XL labels Oct 31, 2025

alyssacgoins force-pushed the model-upload-custom-path-fix branch from d3b3810 to 52f2f5a Compare November 3, 2025 13:02

alyssacgoins changed the title ~~Update logic and testing.~~ Correct Runtime Artifact custom path logic/testing. Nov 3, 2025

alyssacgoins marked this pull request as ready for review November 3, 2025 13:35

google-oss-prow bot removed the do-not-merge/work-in-progress label Nov 3, 2025

google-oss-prow bot requested review from VaniHaripriya and zazulam November 3, 2025 13:35

alyssacgoins force-pushed the model-upload-custom-path-fix branch from 0bca2f8 to 987593b Compare November 3, 2025 13:40

mprahl reviewed Nov 3, 2025

View reviewed changes

alyssacgoins force-pushed the model-upload-custom-path-fix branch 2 times, most recently from 1d4b9d4 to c6042ed Compare November 3, 2025 21:25

mprahl reviewed Nov 4, 2025

View reviewed changes

google-oss-prow bot added size/XXL size/XL and removed size/XXL labels Nov 6, 2025

alyssacgoins force-pushed the model-upload-custom-path-fix branch 4 times, most recently from 88b62d4 to c3341e1 Compare November 7, 2025 15:52

mprahl reviewed Nov 7, 2025

View reviewed changes

alyssacgoins force-pushed the model-upload-custom-path-fix branch 3 times, most recently from 253f9f3 to 0e0b1d2 Compare November 7, 2025 21:09

mprahl reviewed Nov 7, 2025

View reviewed changes

mprahl reviewed Nov 8, 2025

View reviewed changes

alyssacgoins force-pushed the model-upload-custom-path-fix branch 3 times, most recently from e5f5704 to 9eb3e37 Compare November 10, 2025 23:00

mprahl reviewed Nov 11, 2025

View reviewed changes

alyssacgoins force-pushed the model-upload-custom-path-fix branch from 6315e42 to e91af08 Compare November 11, 2025 15:45

mprahl approved these changes Nov 11, 2025

View reviewed changes

google-oss-prow bot assigned mprahl Nov 11, 2025

google-oss-prow bot added lgtm approved labels Nov 11, 2025

Correct Runtime Artifact custom path logic/testing.

19d000b

Signed-off-by: agoins <[email protected]>

alyssacgoins force-pushed the model-upload-custom-path-fix branch from e91af08 to 19d000b Compare November 11, 2025 16:26

google-oss-prow bot removed the lgtm label Nov 11, 2025

google-oss-prow bot added the lgtm label Nov 11, 2025

mprahl merged commit b01a82a into kubeflow:master Nov 11, 2025
126 of 129 checks passed

Correct Runtime Artifact custom path logic/testing. #12402

Correct Runtime Artifact custom path logic/testing. #12402

Uh oh!

Conversation

alyssacgoins commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-prow bot commented Oct 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprahl left a comment

Choose a reason for hiding this comment

Uh oh!

mprahl commented Nov 11, 2025

Uh oh!

google-oss-prow bot commented Nov 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alyssacgoins commented Oct 31, 2025 •

edited

Loading