feat(sdk): backport PipelineTaskFinalStatus change to `sdk/release-1.8` branch #7483

chensun · 2022-03-30T06:52:47Z

Description of your changes:
Re-implement #7309 in sdk/release-1.8 branch.

Checklist:

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 · 2022-03-30T06:53:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from chensun after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

sdk/OWNERS

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

chensun · 2022-03-30T07:05:28Z

@chensun: 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 8ccb372 link true /test kubeflow-pipelines-samples-v2

Again, going to ignore this test failure.

connor-mccarthy

/lgtm

This looks great, @chensun. Just some small nitpick comments and things to consider.

connor-mccarthy · 2022-03-30T15:30:16Z

sdk/python/kfp/v2/components/executor.py

@@ -16,6 +16,7 @@
 from typing import Any, Callable, Dict, List, Optional, Union

 from kfp.v2.components.types import artifact_types, type_annotations
+from kfp.v2.components import task_final_status


In the samples PipelineTaskFinalStatus is imported from from dsl, rather than components. Should we import from the same place in samples and source code? Or is convention to import from dsl as a "user/author" but from the "true" path in the kfp source code?

Applies to multiple files

Or is convention to import from dsl as a "user/author"

This is definitely true. We want users to get our public interface from one of the packages. They shouldn't be aware of the true path, as they're considered internal and could be changed/moved.

but from the "true" path in the kfp source code?

This is probably my personal preference. From my view, the benefit is you won't get any surprise due to possible intended or unintended import aliasing, and you can always tell easily where's the source code for the dependency.

Thank you for explaining -- that makes sense to me.

connor-mccarthy · 2022-03-30T15:30:40Z

sdk/python/kfp/v2/compiler_cli_tests/test_data/pipeline_with_task_final_status.py

+
+
+@component
+def exit_op(user_input: str, status: PipelineTaskFinalStatus):


nit: I think this should be dsl.PipelineTaskFinalStatus per Google Python Style Guide 2.2 Imports.

I haven't been entirely consistent about this in my own PRs, FWIW.

I asked the same question before. :)
The answer was it's okay to break the style guide to keep user experience simple, and I think that makes sense.
For the same reason, you may see our public examples like below.

from kfp.dsl import Input, Dataset @component def my_component(data: Input[Dataset]): ...

connor-mccarthy · 2022-03-30T15:31:39Z

sdk/python/kfp/v2/components/executor.py

+                    state=value.get('state'),
+                    pipeline_job_resource_name=value.get(
+                        'pipelineJobResourceName'),
+                    # pipelineTaskName won't be None once the BE change is


Is there any more info you can provide on which BE change this refers to (issue link, PR, etc)?

connor-mccarthy · 2022-03-30T15:32:38Z

sdk/python/kfp/v2/components/executor_test.py

@@ -25,6 +25,7 @@
                                                    Model)
 from kfp.v2.components.types.type_annotations import (Input, InputPath, Output,
                                                      OutputPath)
+from kfp.v2.components.task_final_status import PipelineTaskFinalStatus


(duplicate)
nit: I think this should be dsl.PipelineTaskFinalStatus (or task_final_status.PipelineTaskFinalStatus) per Google Python Style Guide 2.2 Imports.

I haven't been entirely consistent about this in my own PRs, FWIW.

connor-mccarthy · 2022-03-30T15:33:46Z

sdk/python/kfp/components/_components.py

@@ -30,6 +30,8 @@
 from .structures import *
 from ._data_passing import serialize_value, get_canonical_type_for_type_name

+from kfp.v2.components.types import type_utils


nit: update copyright to 2018-2022, I think?

Applies to multiple files

Adding current year is optional per https://opensource.google/documentation/reference/copyright
I'll skip that for this branch. :)

connor-mccarthy · 2022-03-30T15:38:15Z

sdk/python/kfp/v2/components/task_final_status.py

+    """The final status of a pipeline task.
+
+    This is the Python representation of the proto: PipelineTaskFinalStatus
+    https://github.com/kubeflow/pipelines/blob/61d9210c61ff9be780b678ab78a7f7200d50cffa/api/v2alpha1/pipeline_spec.proto#L886


This references a hash where kfp-pipeline-spec==0.1.13. Should it be 0.1.14 per the change to setup.py in this PR?

Good catch. And I'll update the master branch on this shortly.

connor-mccarthy · 2022-03-30T15:39:33Z

sdk/python/kfp/v2/components/executor_test.py

-                output_parameter_path,
-                'gs://some-bucket/some_task/nested/output_parameter')
+                output_parameter_path, self._test_dir +
+                '/gcs/some-bucket/some_task/nested/output_parameter')


connor-mccarthy · 2022-03-30T15:48:48Z

sdk/python/kfp/v2/components/executor.py

@@ -273,8 +274,23 @@ def execute(self):
            # `Optional[]` to get the actual parameter type.
            v = type_annotations.maybe_strip_optional_from_annotation(v)

-            if self._is_parameter(v):
-                func_kwargs[k] = self._get_input_parameter_value(k, v)
+            if v == task_final_status.PipelineTaskFinalStatus:


nit: Should this be is instead of == because we're asking if the class is the same and the class itself is a singleton?

Feel free to ignore. This isn't entirely consistent throughout.

connor-mccarthy · 2022-03-30T15:52:58Z

sdk/python/kfp/v2/compiler/compiler_test.py

@@ -21,6 +21,7 @@
 from kfp import components
 from kfp.v2 import compiler
 from kfp.v2 import dsl
+from kfp.v2.dsl import PipelineTaskFinalStatus


(duplicate) see other import path comments

connor-mccarthy · 2022-03-30T15:55:13Z

sdk/python/kfp/v2/compiler/compiler.py

+                        if type_utils.is_task_final_status_type(
+                                input_spec.type):
+                            raise ValueError(
+                                'PipelineTaskFinalStatus can only be used in an exit task.'


nit / personal preference: consider using PipelineTaskFinalStatus.__name__ so that the string name will remain up-to-date if the dataclass name ever changes.

google-oss-prow · 2022-03-30T18:37:43Z

@chensun: 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	`9709a1f`	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.

connor-mccarthy · 2022-03-30T20:19:50Z

/lgtm

backport PipelineTaskFinalStatus change to sdk/release-1.8 branch

8ccb372

chensun requested review from ji-yaqi and connor-mccarthy March 30, 2022 06:52

google-oss-prow bot added the size/XL label Mar 30, 2022

google-oss-prow bot requested a review from Ark-kun March 30, 2022 06:53

connor-mccarthy reviewed Mar 30, 2022

View reviewed changes

google-oss-prow bot assigned connor-mccarthy Mar 30, 2022

google-oss-prow bot added the lgtm label Mar 30, 2022

address comments

9709a1f

google-oss-prow bot removed the lgtm label Mar 30, 2022

google-oss-prow bot added the lgtm label Mar 30, 2022

chensun merged commit 706b16e into kubeflow:sdk/release-1.8 Mar 30, 2022

chensun deleted the task-final-status-1-8 branch March 30, 2022 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk): backport PipelineTaskFinalStatus change to `sdk/release-1.8` branch #7483

feat(sdk): backport PipelineTaskFinalStatus change to `sdk/release-1.8` branch #7483

chensun commented Mar 30, 2022

google-oss-prow bot commented Mar 30, 2022

chensun commented Mar 30, 2022

connor-mccarthy left a comment

connor-mccarthy Mar 30, 2022

connor-mccarthy Mar 30, 2022

chensun Mar 30, 2022

connor-mccarthy Mar 30, 2022

connor-mccarthy Mar 30, 2022

chensun Mar 30, 2022

connor-mccarthy Mar 30, 2022

connor-mccarthy Mar 30, 2022

connor-mccarthy Mar 30, 2022

connor-mccarthy Mar 30, 2022

chensun Mar 30, 2022

connor-mccarthy Mar 30, 2022

chensun Mar 30, 2022

connor-mccarthy Mar 30, 2022

connor-mccarthy Mar 30, 2022

connor-mccarthy Mar 30, 2022

connor-mccarthy Mar 30, 2022

google-oss-prow bot commented Mar 30, 2022

connor-mccarthy commented Mar 30, 2022



		@component
		def exit_op(user_input: str, status: PipelineTaskFinalStatus):

feat(sdk): backport PipelineTaskFinalStatus change to sdk/release-1.8 branch #7483

feat(sdk): backport PipelineTaskFinalStatus change to sdk/release-1.8 branch #7483

Conversation

chensun commented Mar 30, 2022

google-oss-prow bot commented Mar 30, 2022

chensun commented Mar 30, 2022

connor-mccarthy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-prow bot commented Mar 30, 2022

connor-mccarthy commented Mar 30, 2022

feat(sdk): backport PipelineTaskFinalStatus change to `sdk/release-1.8` branch #7483

feat(sdk): backport PipelineTaskFinalStatus change to `sdk/release-1.8` branch #7483