fix(sdk): fixes the specified 'mlpipeline-ui-metadata','mlpipeline-metrics' path is overrided by default value #6796

dinimicky · 2021-10-23T02:04:33Z

Description of your changes:
the 'mlpipeline-ui-metadata','mlpipeline-metrics' path is replaced by the default path as '_components._generate_output_file_name(output.name)'

When the user set the path of 'mlpipeline-ui-metadata','mlpipeline-metrics' in fileOutputs,

pipelines/sdk/python/kfp/dsl/_container_op.py

Lines 1312 to 1322 in 98677b2

    
           # Skip the special handling that is unnecessary in v2. 
        
           if not kfp.COMPILING_FOR_V2: 
        
               # Special handling for the mlpipeline-ui-metadata and mlpipeline-metrics 
        
               # outputs that should always be saved as artifacts 
        
               # TODO: Remove when outputs are always saved as artifacts 
        
               for output_name, path in dict(file_outputs).items(): 
        
                   is_legacy_name, normalized_name = _is_legacy_output_name( 
        
                       output_name) 
        
                   if is_legacy_name: 
        
                       output_artifact_paths[normalized_name] = path 
        
                       del file_outputs[output_name]

the ContainerOp.__init__ will move the path of 'mlpipeline-ui-metadata','mlpipeline-metrics' to output_artifact_paths and is deleted in file_outputs.

but in the 'set_metadata' function, as the 'mlpipeline-ui-metadata','mlpipeline-metrics' still exist in the _metadata.outputs but not in self.file_outputs as it has been deleted in ContainerOp.__init__, the path will be rewritten into the file_outputs by the function _components._generate_output_file_name(output.name)

pipelines/sdk/python/kfp/dsl/_container_op.py

Lines 1442 to 1455 in 98677b2

    
           for output in self._metadata.outputs: 
        
               if output.name not in self.file_outputs: 
        
                   output_filename = _components._generate_output_file_name( 
        
                       output.name) 
        
                   self.file_outputs[output.name] = output_filename 
        
           if not kfp.COMPILING_FOR_V2: 
        
               for output_name, path in dict(self.file_outputs).items(): 
        
                   is_legacy_name, normalized_name = _is_legacy_output_name( 
        
                       output_name) 
        
                   if is_legacy_name: 
        
                       self.output_artifact_paths[normalized_name] = path 
        
                       del self.file_outputs[output_name] 
        
                       del self.outputs[output_name]

And then the new path will be written into the output_artifact_paths in the next step.

Finally the user specified path of 'mlpipeline-ui-metadata','mlpipeline-metrics' is overrided.

So in the first step, it should check if the output name has the path definition in both the file_outputs and the output_artifact_paths.

the 'mlpipeline-ui-metadata','mlpipeline-metrics' path is replaced by the default path as '_components._generate_output_file_name(output.name)'

google-cla · 2021-10-23T02:04:36Z

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

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-oss-robot · 2021-10-23T02:04:45Z

Hi @dinimicky. 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-cla · 2021-10-23T02:08:16Z

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

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

dinimicky · 2021-10-23T03:13:54Z

@googlebot I signed it!

google-cla · 2021-10-23T03:15:14Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

dinimicky · 2021-10-23T03:38:57Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot I fix it.

chensun · 2021-10-27T05:20:09Z

/ok-to-test

chensun · 2021-10-27T05:54:18Z

@dinimicky Thank you for the fix. Can you please add a release note for this following this format:

pipelines/sdk/RELEASE.md

Line 18 in 4360652

    
           * Fix placeholder mapping error in v2. [\#6794](https://github.com/kubeflow/pipelines/pull/6794)

/lgtm

dinimicky · 2021-10-28T01:46:09Z

@dinimicky: The following tests 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 b8d9cd4 link true /test kubeflow-pipelines-samples-v2
kubeflow-pipelines-sdk-python37 b8d9cd4 link true /test kubeflow-pipelines-sdk-python37
kubeflow-pipelines-sdk-python38 b8d9cd4 link true /test kubeflow-pipelines-sdk-python38
kubeflow-pipelines-sdk-python36 b8d9cd4 link true /test kubeflow-pipelines-sdk-python36
kubeflow-pipelines-tfx-python37 b8d9cd4 link true /test kubeflow-pipelines-tfx-python37
kubeflow-pipelines-sdk-python39 b8d9cd4 link true /test kubeflow-pipelines-sdk-python39
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.

/retest

chensun · 2021-10-28T06:14:18Z

/lgtm
/approve

Thanks!

google-oss-robot · 2021-10-28T06:14:25Z

[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:

~~sdk/OWNERS~~ [chensun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

chensun · 2021-10-28T06:46:39Z

I checked the sample tests log: https://oss-prow.knative.dev/view/gs/oss-prow/pr-logs/pull/kubeflow_pipelines/6796/kubeflow-pipelines-samples-v2/1453537694247292928

Three sample tests reported failure state, but actually all three pipeline succeeded. Going to ignore the test and merge as-is.

Adding a data point for #6815

chensun · 2021-10-28T06:50:12Z

/retest since head has changed.

google-oss-robot · 2021-10-28T06:50:13Z

@chensun: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test kubeflow-pipeline-backend-test
/test kubeflow-pipeline-e2e-test
/test kubeflow-pipeline-frontend-test
/test kubeflow-pipeline-integration-test
/test kubeflow-pipeline-mkp-test
/test kubeflow-pipeline-sample-test
/test kubeflow-pipeline-upgrade-test
/test kubeflow-pipelines-backend-visualization
/test kubeflow-pipelines-component-yaml
/test kubeflow-pipelines-components-gcp-python27
/test kubeflow-pipelines-components-gcp-python37
/test kubeflow-pipelines-components-google-cloud-python38
/test kubeflow-pipelines-manifests
/test kubeflow-pipelines-samples-v2
/test kubeflow-pipelines-sdk-python36
/test kubeflow-pipelines-sdk-python37
/test kubeflow-pipelines-sdk-python38
/test kubeflow-pipelines-sdk-python39
/test kubeflow-pipelines-tfx-python37
/test kubeflow-pipelines-v2-go-test

Use /test all to run the following jobs that were automatically triggered:

kubeflow-pipelines-samples-v2
kubeflow-pipelines-sdk-python36
kubeflow-pipelines-sdk-python37
kubeflow-pipelines-sdk-python38
kubeflow-pipelines-sdk-python39
kubeflow-pipelines-tfx-python37

In response to this:

/retest since head has changed.

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.

chensun · 2021-10-28T06:51:16Z

/retest

google-oss-robot · 2021-10-28T07:33:04Z

@dinimicky: 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	`a116e17`	link	true	`/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.

chensun · 2021-10-28T08:32:55Z

Sample test failures are irrelevant

In the last run, two random tests failed with

This step is in Failed state with this message: Pod was active on the node longer than the specified deadline

Data point for #6815

fix the issue:

982d7aa

the 'mlpipeline-ui-metadata','mlpipeline-metrics' path is replaced by the default path as '_components._generate_output_file_name(output.name)'

google-cla bot added the cla: no label Oct 23, 2021

google-oss-robot added the size/S label Oct 23, 2021

google-oss-robot added the needs-ok-to-test label Oct 23, 2021

google-oss-robot requested review from chensun and ji-yaqi October 23, 2021 02:05

reformat code

45b7229

google-cla bot added cla: yes and removed cla: no labels Oct 23, 2021

Update _container_op.py

9189395

google-oss-robot added ok-to-test and removed needs-ok-to-test labels Oct 27, 2021

google-oss-robot assigned chensun Oct 27, 2021

google-oss-robot added the lgtm label Oct 27, 2021

Update RELEASE.md

b8d9cd4

google-oss-robot removed the lgtm label Oct 28, 2021

Merge branch 'master' into fix_output_artifact_paths_is_overrided

a116e17

google-oss-robot added the lgtm label Oct 28, 2021

google-oss-robot added the approved label Oct 28, 2021

chensun merged commit 41275b4 into kubeflow:master Oct 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sdk): fixes the specified 'mlpipeline-ui-metadata','mlpipeline-metrics' path is overrided by default value #6796

fix(sdk): fixes the specified 'mlpipeline-ui-metadata','mlpipeline-metrics' path is overrided by default value #6796

dinimicky commented Oct 23, 2021 •

edited

Loading

google-cla bot commented Oct 23, 2021

google-oss-robot commented Oct 23, 2021

google-cla bot commented Oct 23, 2021

dinimicky commented Oct 23, 2021 •

edited

Loading

google-cla bot commented Oct 23, 2021

dinimicky commented Oct 23, 2021

chensun commented Oct 27, 2021

chensun commented Oct 27, 2021

dinimicky commented Oct 28, 2021

chensun commented Oct 28, 2021

google-oss-robot commented Oct 28, 2021

chensun commented Oct 28, 2021

chensun commented Oct 28, 2021

google-oss-robot commented Oct 28, 2021

chensun commented Oct 28, 2021

google-oss-robot commented Oct 28, 2021

chensun commented Oct 28, 2021

	# Skip the special handling that is unnecessary in v2.
	if not kfp.COMPILING_FOR_V2:
	# Special handling for the mlpipeline-ui-metadata and mlpipeline-metrics
	# outputs that should always be saved as artifacts
	# TODO: Remove when outputs are always saved as artifacts
	for output_name, path in dict(file_outputs).items():
	is_legacy_name, normalized_name = _is_legacy_output_name(
	output_name)
	if is_legacy_name:
	output_artifact_paths[normalized_name] = path
	del file_outputs[output_name]

	for output in self._metadata.outputs:
	if output.name not in self.file_outputs:
	output_filename = _components._generate_output_file_name(
	output.name)
	self.file_outputs[output.name] = output_filename

	if not kfp.COMPILING_FOR_V2:
	for output_name, path in dict(self.file_outputs).items():
	is_legacy_name, normalized_name = _is_legacy_output_name(
	output_name)
	if is_legacy_name:
	self.output_artifact_paths[normalized_name] = path
	del self.file_outputs[output_name]
	del self.outputs[output_name]

fix(sdk): fixes the specified 'mlpipeline-ui-metadata','mlpipeline-metrics' path is overrided by default value #6796

fix(sdk): fixes the specified 'mlpipeline-ui-metadata','mlpipeline-metrics' path is overrided by default value #6796

Conversation

dinimicky commented Oct 23, 2021 • edited Loading

google-cla bot commented Oct 23, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

google-oss-robot commented Oct 23, 2021

google-cla bot commented Oct 23, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

dinimicky commented Oct 23, 2021 • edited Loading

google-cla bot commented Oct 23, 2021

dinimicky commented Oct 23, 2021

chensun commented Oct 27, 2021

chensun commented Oct 27, 2021

dinimicky commented Oct 28, 2021

chensun commented Oct 28, 2021

google-oss-robot commented Oct 28, 2021

chensun commented Oct 28, 2021

chensun commented Oct 28, 2021

google-oss-robot commented Oct 28, 2021

chensun commented Oct 28, 2021

google-oss-robot commented Oct 28, 2021

chensun commented Oct 28, 2021

dinimicky commented Oct 23, 2021 •

edited

Loading

dinimicky commented Oct 23, 2021 •

edited

Loading