feat(backend): condition combinators #5405

Udiknedormin · 2021-03-31T23:24:12Z

Description of your changes:

Addresses issue #5404.

Changes ConditionOperator and PipelineParamTuple into attr classes.
Overwriting ConditionOperator's __bool__ method to produce a warning in case of using and or or on it (as it's probably not what the user intended to do)
Defining __and__ and __or__ operators on ConditionOperator, which generate a tree.
Handling trees of ConditionOperator in the compiler.
Optimising braces for cases where they actually decrease readability (multiple operators of the same kind)

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.

Do you want this pull request (PR) cherry-picked into the current release branch?

Learn more about cherry-picking updates into the release branch.

Please consider cherry-picking this change.

google-oss-robot · 2021-03-31T23:24:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign chensun after the PR has been reviewed.
You can assign the PR to them by writing /assign @chensun in a comment when ready.

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

google-oss-robot · 2021-03-31T23:24:22Z

Hi @Udiknedormin. 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.

Tomcli · 2021-03-31T23:48:00Z

/ok-to-test

Tomcli · 2021-03-31T23:49:45Z

This PR also addresses the and and or logical operators for issue #482

Tomcli · 2021-04-01T00:07:58Z

sdk/python/kfp/dsl/_pipeline_param.py

+      "If operator combinator was meant, use & or | instead."
+    ])
+    warnings.warn(msg)
+    return super().__bool__()


Looks like you didn't assign a parent class to ConditionOperator?

It's inherited from object.

The build is failing with these errors, so probably the object didn't inherent correctly

Traceback (most recent call last): File "/home/prow/go/src/github.com/kubeflow/pipelines/sdk/python/kfp/compiler/v2_compatible_compiler_test.py", line 93, in test_two_step_pipeline self._assert_compiled_pipeline_equals_golden( File "/home/prow/go/src/github.com/kubeflow/pipelines/sdk/python/kfp/compiler/v2_compatible_compiler_test.py", line 59, in _assert_compiled_pipeline_equals_golden skipped 9 lines unfold_more File "/home/prow/go/src/github.com/kubeflow/pipelines/sdk/python/kfp/dsl/_pipeline_param.py", line 32, in __bool__ return super().__bool__() AttributeError: 'super' object has no attribute '__bool__

Looks like you're right, so it's not inheritance, but some magic instead. I'll just replace it with True then. Good catch!

Ark-kun · 2021-04-01T22:51:06Z

sdk/python/kfp/dsl/_pipeline_param.py

+  operator = attr.ib(type=str)
+  operand1 = attr.ib(type=Union['PipelineParam', Any])
+  operand2 = attr.ib(type=Union['PipelineParam', Any])


Can we use normal __init__ or typed class variables?
What is the advantage of using attrs over the standard Python typing?

What is the advantage of using attrs

It provides __str__, __repr__, __eq__, __hash__ and lots of other utilities out of the box.

Can we use normal __init__

Normal __init__ will provide neither of the good stuff mentioned above.

or typed class variables?

We don't use any class variables here, these are all instance variables. Class variables are introduced via ClassVar[T].

Ark-kun · 2021-04-01T22:51:46Z

sdk/python/kfp/dsl/_pipeline_param.py

+import warnings
+
+@attr.s
+class ConditionOperator:


Should we name this BinaryPredicate?

Sure! I just wanted to change as little as possible, but if you're ok with that --- I'd actually prefer that. Especially considering that UnaryPredicate would be useful too (e.g. for __not__).

Ark-kun · 2021-04-01T22:52:02Z

sdk/python/kfp/dsl/_pipeline_param.py

+  def __and__(self, other) -> 'ConditionOperator':
+    return ConditionOperator('&&', self, other)
+
+  def __or__(self, other) -> 'ConditionOperator':


Can we support not?

Sure, it would require either:

ignoring one of the arguments (probably setting it to None?), like postfix operators in C++

creating a separate class UnaryOperator (and rename this one to BinaryOperator)

changing operand1 and operand2 into a sequence/list/ordered-iterable of operands

Ark-kun · 2021-04-01T22:55:46Z

sdk/python/tests/compiler/testdata/complex_coin.py

+import kfp.dsl as dsl
+
+
+class FlipCoinOp(dsl.ContainerOp):


We should not directly use ContainerOp in new code

@kfp.components.create_component_from_func def flip_coin_op() -> str: import random return random.choice(["heads", "tails"])

It's an example based on flip-coin. Should we change flip-coin too then?

Ark-kun · 2021-04-01T22:56:17Z

sdk/python/tests/compiler/testdata/complex_coin.py

+            file_outputs={'output': '/tmp/output'})
+
+
+class PrintOp(dsl.ContainerOp):


We should not directly use ContainerOp in new code

@kfp.components.create_component_from_func def print_op(msg: str): print(msg)

object doesn't implement __bool__, it's some internal magic instead therefore, super().__bool__() cannot be used

google-oss-robot · 2021-04-06T18:32:16Z

@Udiknedormin: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
kubeflow-pipelines-sdk-python38	`2245bab`	link	`/test kubeflow-pipelines-sdk-python38`
kubeflow-pipelines-sdk-python39	`2245bab`	link	`/test kubeflow-pipelines-sdk-python39`
kubeflow-pipelines-sdk-python37	`2245bab`	link	`/test kubeflow-pipelines-sdk-python37`
kubeflow-pipelines-sdk-python36	`2245bab`	link	`/test kubeflow-pipelines-sdk-python36`
kubeflow-pipeline-e2e-test	`2245bab`	link	`/test kubeflow-pipeline-e2e-test`

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.

stale · 2022-03-02T12:04:36Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

rimolive · 2024-03-24T14:51:06Z

Closing this PR. No activity for more than a year.

/close

google-oss-prow · 2024-03-24T14:51:10Z

@rimolive: Closed this PR.

In response to this:

Closing this PR. No activity for more than a year.

/close

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.

Udiknedormin added 8 commits March 29, 2021 22:43

add requirement: attrs

13a0e54

ConditionOperator, PipelineParamTuple: namedtuple --> attr

56f5a06

add type annotations to dsl.Condition.__init__

bbfd5e2

add & and | operators to ConditionOperator

c39cd28

compile complex conditions

43d6729

add complex coin example

4aabd4b

better error message

922463e

error -> warning

9eff31e

google-oss-robot requested review from Ark-kun and numerology March 31, 2021 23:24

google-cla bot added the cla: yes label Mar 31, 2021

google-oss-robot added the size/L label Mar 31, 2021

google-oss-robot added the needs-ok-to-test label Mar 31, 2021

google-oss-robot added ok-to-test and removed needs-ok-to-test labels Mar 31, 2021

Tomcli reviewed Apr 1, 2021

View reviewed changes

Ark-kun reviewed Apr 1, 2021

View reviewed changes

fix: __bool__ should return True

2245bab

object doesn't implement __bool__, it's some internal magic instead therefore, super().__bool__() cannot be used

stale bot added the lifecycle/stale The issue / pull request is stale, any activities remove this label. label Mar 2, 2022

stale bot removed the lifecycle/stale The issue / pull request is stale, any activities remove this label. label Mar 24, 2024

google-oss-prow bot closed this Mar 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backend): condition combinators #5405

feat(backend): condition combinators #5405

Udiknedormin commented Mar 31, 2021

google-oss-robot commented Mar 31, 2021

google-oss-robot commented Mar 31, 2021

Tomcli commented Mar 31, 2021

Tomcli commented Mar 31, 2021

Tomcli Apr 1, 2021

Udiknedormin Apr 6, 2021 •

edited

Loading

Tomcli Apr 6, 2021 •

edited

Loading

Udiknedormin Apr 6, 2021

Ark-kun Apr 1, 2021

Udiknedormin Apr 6, 2021

Ark-kun Apr 1, 2021

Udiknedormin Apr 6, 2021

Ark-kun Apr 1, 2021

Udiknedormin Apr 6, 2021

Ark-kun Apr 1, 2021

Udiknedormin Apr 6, 2021

Ark-kun Apr 1, 2021

google-oss-robot commented Apr 6, 2021

stale bot commented Mar 2, 2022

rimolive commented Mar 24, 2024

google-oss-prow bot commented Mar 24, 2024

		file_outputs={'output': '/tmp/output'})


		class PrintOp(dsl.ContainerOp):

feat(backend): condition combinators #5405

feat(backend): condition combinators #5405

Conversation

Udiknedormin commented Mar 31, 2021

google-oss-robot commented Mar 31, 2021

google-oss-robot commented Mar 31, 2021

Tomcli commented Mar 31, 2021

Tomcli commented Mar 31, 2021

Choose a reason for hiding this comment

Udiknedormin Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Tomcli Apr 6, 2021 • 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

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-robot commented Apr 6, 2021

stale bot commented Mar 2, 2022

rimolive commented Mar 24, 2024

google-oss-prow bot commented Mar 24, 2024

Udiknedormin Apr 6, 2021 •

edited

Loading

Tomcli Apr 6, 2021 •

edited

Loading