feat(sdk)!: Implement Graph Component #8179

chensun · 2022-08-22T21:30:39Z

Description of your changes:

Support re-using a pipeline in another pipeline (aka graph component).
Refactoring compiler code, and move pipeline compiling trigger to @pipeline decorator.

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.

connor-mccarthy · 2022-08-23T14:32:11Z

/retest

connor-mccarthy

@chensun, very nice to see this in action! My comments are mostly nitpicks, with a few questions/suggestions. Overall, I think adding more compiler test pipelines would be helpful for verifying compilation behavior.

connor-mccarthy · 2022-08-23T03:36:36Z

sdk/RELEASE.md

@@ -2,10 +2,13 @@

 ## Major Features and Improvements
 * Support parallelism setting in ParallelFor [\#8146](https://github.com/kubeflow/pipelines/pull/8146)
+* Support pipeline as a component [\#8179](https://github.com/kubeflow/pipelines/pull/8179)


Should we wait to add this until adding support for pipeline outputs?

(thought I added a reply to this but somehow it wasn't captured)

I think it's fine to add this release note. Pipeline output is a new feature independent from pipeline as component, although without pipeline outputs it limits the use case, this change alone unblocks use case like conditional email notification. WDYT?

@chensun, WDYT about this?

Added my reply but I guess somehow it didn't shown up? Anyway, removed the release note for now.

sdk/python/kfp/compiler/test_data/pipelines/pipeline_in_pipeline.py

connor-mccarthy · 2022-08-23T03:38:19Z

sdk/python/kfp/compiler/test_data/pipelines/pipeline_in_pipeline.py

+    return msg
+
+
+print_op2 = components.load_component_from_text("""


Is this included for a v1-v2 compatibility test? If not, WDYT about using the preferred lightweight component authoring approach instead?

Good question. I included this to make sure the inputs/outputs placeholders are still parsed/resolved correctly. The lightweight component that use {{$}} doesn't have this complex code path. I started this sample before @dsl.container_component was ready. Now I can replace this to the new recommended approach to demo best practice.

I'll still keep a v1 yaml sample in the other more complex example to cover v1-v2 compatibility.

sdk/python/kfp/compiler/test_data/pipelines/pipeline_in_pipeline.py

connor-mccarthy · 2022-08-23T03:42:49Z

sdk/python/kfp/compiler/read_write_test.py

@@ -29,13 +29,14 @@
 from kfp.components import structures
 import yaml

-PROJECT_ROOT = os.path.abspath(os.path.join(__file__, *([os.path.pardir] * 5)))
+_DEFAULT_PIPELINE_FUNC_NAME = 'my_pipeline'


nit:
Why underscore prefix in test file constants? My sense was that we were moving away from this pattern for all files except the main subpackage __init__.py files.

Yes. We are moving away from prefixing most of the methods assuming all of them unless explicitly exposed via __init__.py are private. That being said, I think underscore prefix can still apply to the local constants and methods which are not supposed to be referenced outside individual python files (modules). The prefix helps linter to flag such access--although we don't always pay attention to the lint errors, we probably should in the future.

One example shown in this PR is that _component_human_name reference which shouldn't exist.

WDYT?

Good point. I like the underscore to discouraging cross-module object usage. LGTM.

connor-mccarthy · 2022-08-23T14:59:40Z

sdk/python/kfp/components/pipeline_context.py

-            return pipeline_decorator_handler(func) or func
-        else:
-            return func
+        return component_factory.create_graph_component_from_func(func)


FYI, with this change, sample test code needs to be updated:

File "/workdir/samples/test/utils/kfp/samples/test/utils.py", line 119, in test_wrapper pipeline_name = case.pipeline_func._component_human_name AttributeError: 'GraphComponent' object has no attribute '_component_human_name' Error: exit status 1

Good catch! Updated.

sdk/python/kfp/components/component_factory.py

connor-mccarthy · 2022-08-23T15:12:13Z

sdk/python/kfp/components/pipeline_context.py

-            return pipeline_decorator_handler(func) or func
-        else:
-            return func
+        return component_factory.create_graph_component_from_func(func)

    return _pipeline



I think it's reasonable to not want to change too much at once here, but for a future PR, I wonder if the GraphComponent class eliminates the need to have this Pipeline class. It would certainly take some refactoring, but I don't think we need to get_default_pipeline anymore because the pipeline will be the GraphComponent being created. The GraphComponent object can handle registration of its tasks.

Curious what you think about this.

Yes, I think you're probably right. I'll give this a bit more thoughts and probably make the change in a follow up PR.

connor-mccarthy · 2022-08-23T15:17:42Z

sdk/python/kfp/components/pipeline_context.py

-# It can be used by command-line DSL compiler to inject code that runs for every
-# pipeline definition.
-pipeline_decorator_handler = None
-

 def pipeline(name: Optional[str] = None,


Do we want to force this decorator to be called? Or enable both patterns as with @dsl.component? I see the error message for uncalled an @dsl.pipeline decorator was removed.

When I look back at the change, I realize the issue was missing the user provided pipeline name which is used as pipeline context. Enforcing the decorator to be called doesn't really enforce that users must provide value for the name parameter. So I think we should probably find another way to make sure we have pipeline name.
With the introduction of graph component, I don't see we have to enforce the name when the pipeline is defined because it can be used only as an inner step. So maybe we can just rely on check at submission time to make sure there's a name for pipeline context? WDYT?

Doesn't pipeline name fall back to a sanitized version of the pipeline function name? It's possible I'm misunderstanding somewhere else name is used. In the following example, I do not provide a pipeline name, yet there is one in IR:

from kfp import dsl from kfp import compiler import yaml @dsl.component def nothing(): pass @dsl.pipeline() def my_pipeline(): nothing() output = 'out.yaml' compiler.Compiler().compile(pipeline_func=my_pipeline, package_path=output) with open(output, 'r') as f: ir = yaml.safe_load(f.read()) assert ir['pipelineInfo']['name'] == 'my-pipeline'

If I am correct, my current thinking is that irrespective of the introduction of graph components we should consider implementing the @dsl.pipeline decorator similarly to the @dsl.component decorator and not requiring that it be called when used.

This is somewhat unrelated to your PR (other than the removal of those tests) and I think this can be implemented in a separate PR if we go this route.

I thought we required users to provide a name explicitly because of it importance--being used for pipeline context. Maybe at some point our implementation added the fallback logic so that explicit name is no longer required.

Other than the name requirement, I can't think of any reason that we want to enforce it to be called.

connor-mccarthy · 2022-08-23T16:00:17Z

sdk/python/kfp/compiler/pipeline_spec_builder.py

-
-    Args:
-        root_group: The root group of a pipeline.
+def merge_deployment_spec(


This function causes a problem if there are multiple pipelines defined in a single Python file/execution, but only some of them are included in the final submitted pipeline. For example, try submitting my_pipeline:

from kfp import dsl @dsl.component def print_op1(msg: str) -> str: print(msg) return msg @dsl.component def print_op2(msg: str) -> str: print(msg) return msg @dsl.pipeline(name='test-graph-component') def graph_component(msg: str): task = print_op1(msg=msg) print_op2(msg=task.output) @dsl.pipeline(name='test-graph-component') def another_graph_comp(msg: str): task = print_op1(msg=msg) print_op2(msg=task.output) graph_component(msg='fifth') @dsl.pipeline(name='pipeline-in-pipeline') def my_pipeline(): t = print_op1(msg='Hello') graph_component(msg='world')

Great insight! This is a critical miss. I updated the code to make sure the merging doesn't modify the "template" in place, and added test case for it.

chensun · 2022-08-24T20:21:38Z

/test kubeflow-pipelines-samples-v2

zichuan-scott-xu

LGTM! There are only a few question/comments.

zichuan-scott-xu · 2022-08-24T20:55:34Z

sdk/python/kfp/compiler/_read_write_test_config.py

@@ -46,6 +46,7 @@
            'two_step_pipeline_containerized',
            'pipeline_with_multiple_exit_handlers',
            'pipeline_with_parallelfor_parallelism',
+            'pipeline_in_pipeline',
        ],


LGTM! There are few comments:

This might be irrevalent, but does this feature allow a component to call itself recursively? Or is it possible that the call stack is like A -> B -> C -> A?

Maybe also put the two compiler tests in v2 sample tests as well to make sure they pass.

connor-mccarthy

Thanks for your updates after the initial review! Only remaining comments are about the release note and sample tests.

connor-mccarthy · 2022-08-24T21:09:41Z

sdk/python/kfp/compiler/test_data/pipelines/pipeline_in_pipeline_complex.py

+    command:
+    - echo
+    - {inputValue: msg}
+""")


👍🏻 I like the three styles here

connor-mccarthy · 2022-08-24T21:12:04Z

samples/test/utils/kfp/samples/test/utils.py

+                # TODO: remove accessing of protected member
+                pipeline_name = getattr(case.pipeline_func,
+                                        '_component_human_name',
+                                        case.pipeline_func.name)


FYI:

File "/workdir/samples/test/utils/kfp/samples/test/utils.py", line 122, in test_wrapper case.pipeline_func.name) AttributeError: 'function' object has no attribute 'name'

Hmm... guessing the default is evaluated first regardless if getattr find the attribute.

connor-mccarthy · 2022-08-24T21:18:23Z

sdk/python/kfp/compiler/pipeline_spec_builder.py

+    # Make a copy of the sub_pipeline_spec so that the "template" remains
+    # unchanged and works even the pipeline is reused multiple times.
+    sub_pipeline_spec_copy = pipeline_spec_pb2.PipelineSpec()
+    sub_pipeline_spec_copy.CopyFrom(sub_pipeline_spec)


…raph-component

google-oss-prow · 2022-08-26T09:06:09Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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:

samples/OWNERS
sdk/OWNERS

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

* Implement Graph Component * isort imports * release note * address review comments * address review comments

Implement Graph Component

9b0539e

chensun requested a review from connor-mccarthy August 22, 2022 21:30

chensun assigned connor-mccarthy Aug 22, 2022

google-oss-prow bot requested a review from zijianjoy August 22, 2022 21:30

google-oss-prow bot added the size/XXL label Aug 22, 2022

isort imports

5c67cec

chensun removed the request for review from zijianjoy August 22, 2022 22:24

chensun assigned connor-mccarthy and unassigned connor-mccarthy Aug 22, 2022

release note

51de53a

connor-mccarthy mentioned this pull request Aug 23, 2022

fix(sdk)Remove unnecessary errors for dependencies on task groups #8167

Closed

1 task

connor-mccarthy requested changes Aug 23, 2022

View reviewed changes

address review comments

30ec8d3

chensun requested a review from zichuan-scott-xu August 24, 2022 18:04

zichuan-scott-xu reviewed Aug 24, 2022

View reviewed changes

connor-mccarthy requested changes Aug 24, 2022

View reviewed changes

chensun added 2 commits August 25, 2022 22:27

Merge branch 'master' of https://github.com/kubeflow/pipelines into g…

cdcd4f5

…raph-component

address review comments

8e3a8c4

chensun added lgtm approved labels Aug 26, 2022

google-oss-prow bot merged commit 5ce4954 into kubeflow:master Aug 26, 2022

chensun deleted the graph-component branch August 30, 2022 09:53

jlyaoyuli pushed a commit to jlyaoyuli/pipelines that referenced this pull request Jan 5, 2023

feat(sdk)!: Implement Graph Component (kubeflow#8179)

cdd3e9a

* Implement Graph Component * isort imports * release note * address review comments * address review comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk)!: Implement Graph Component #8179

feat(sdk)!: Implement Graph Component #8179

chensun commented Aug 22, 2022

connor-mccarthy commented Aug 23, 2022

connor-mccarthy left a comment

connor-mccarthy Aug 23, 2022

chensun Aug 24, 2022

connor-mccarthy Aug 24, 2022

chensun Aug 26, 2022

connor-mccarthy Aug 23, 2022

chensun Aug 24, 2022

connor-mccarthy Aug 23, 2022

chensun Aug 24, 2022

connor-mccarthy Aug 24, 2022

connor-mccarthy Aug 23, 2022

chensun Aug 24, 2022

connor-mccarthy Aug 23, 2022

chensun Aug 24, 2022

connor-mccarthy Aug 23, 2022

chensun Aug 24, 2022

connor-mccarthy Aug 24, 2022

chensun Aug 26, 2022

connor-mccarthy Aug 23, 2022

chensun Aug 24, 2022

chensun commented Aug 24, 2022

zichuan-scott-xu left a comment

zichuan-scott-xu Aug 24, 2022

connor-mccarthy left a comment •

edited

Loading

connor-mccarthy Aug 24, 2022

connor-mccarthy Aug 24, 2022

chensun Aug 25, 2022

connor-mccarthy Aug 24, 2022

google-oss-prow bot commented Aug 26, 2022

		return msg


		print_op2 = components.load_component_from_text("""

feat(sdk)!: Implement Graph Component #8179

feat(sdk)!: Implement Graph Component #8179

Conversation

chensun commented Aug 22, 2022

connor-mccarthy commented Aug 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chensun commented Aug 24, 2022

zichuan-scott-xu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connor-mccarthy left a comment • edited Loading

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 Aug 26, 2022

connor-mccarthy left a comment •

edited

Loading