Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(sdk): support dsl.If, dsl.Elif, and dsl.Else #9894

Merged
merged 6 commits into from
Sep 11, 2023
Merged

feat(sdk): support dsl.If, dsl.Elif, and dsl.Else #9894

merged 6 commits into from
Sep 11, 2023

Conversation

connor-mccarthy
Copy link
Member

@connor-mccarthy connor-mccarthy commented Aug 17, 2023 •
edited
Loading

Description of your changes:
Supports authoring pipelines that use dsl.If, dsl.Elif, and dsl.Else. For example:

from kfp import dsl

@dsl.pipeline
def flip_coin_pipeline():
    flip_coin_task = flip_coin()
    with dsl.If(flip_coin_task.output == 'heads'):
        print_and_return(text='Got heads!')
    with dsl.Elif(flip_coin_task.output == 'tails'):
        print_and_return(text='Got tails!')
    with dsl.Else():
        print_and_return(text='Draw!')

Note that dsl.If is merely an alias for dsl.Condition. dsl.Elif and dsl.Else are new functionality.

Checklist:

@google-oss-prow
Copy link

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

@google-oss-prow google-oss-prow bot requested review from gkcalat and Linchin August 17, 2023 17:36
@connor-mccarthy connor-mccarthy force-pushed the if-elif-else branch 3 times, most recently from d3e2612 to 572915d Compare August 31, 2023 15:33
@connor-mccarthy
Copy link
Member Author

/assign @chensun

@connor-mccarthy connor-mccarthy marked this pull request as ready for review August 31, 2023 15:34
@connor-mccarthy
Copy link
Member Author

/hold do not merge before 9/1/23

@connor-mccarthy connor-mccarthy mentioned this pull request Aug 31, 2023
Merged
@connor-mccarthy
Copy link
Member Author

/test all

chensun
chensun reviewed Aug 31, 2023
View reviewed changes
sdk/python/test_data/pipelines/if_else.py
@@ -0,0 +1,42 @@
# Copyright 2023 The Kubeflow Authors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exmaple seems completely covered by if_elif_else. Can we merge and reduce duplication?

Copy link
Member Author

@connor-mccarthy connor-mccarthy Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd slightly prefer to keep this test case to separately test that the compilation logic for dsl.Else is correct both following a dsl.If (single condition to negate) and following a dsl.If + dsl.Elif (>1 condition to negate). This is indeed a subset of the other tests, but I find having a "base case" tests for each DSL feature helps during the development workflow.

sdk/python/test_data/pipelines/if_elif_else_complex.py Outdated Show resolved Hide resolved
sdk/python/kfp/compiler/compiler_utils.py Outdated Show resolved Hide resolved
sdk/python/kfp/dsl/pipeline_channel.py Outdated Show resolved Hide resolved
sdk/python/kfp/dsl/tasks_group.py Outdated Show resolved Hide resolved
sdk/python/kfp/dsl/tasks_group.py
task2 = my_component2(...)
"""

def __init__(
self,
condition: pipeline_channel.ConditionOperator,
condition,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: type annotation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intentionally removed this, since I personally find it a bit confusing and I suspect users do too. See the current reference docs which use this annotation.

While annotation is technically correct, it isn't informative to the user and exposes an implementation detail. We don't actually want the user to provide a ConditionOperator (name changed in this PR)... we want them to provide a comparative expression involving one or more pipeline channels (i.e., to think of it as a "future" boolean).

WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG.

sdk/python/kfp/dsl/tasks_group.py Outdated Show resolved Hide resolved
sdk/python/kfp/dsl/tasks_group.py

def __init__(
self,
condition,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: type annotation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above

sdk/python/test_data/pipelines/if_elif_else.py Outdated Show resolved Hide resolved
sdk/python/kfp/dsl/pipeline_context.py Outdated Show resolved Hide resolved
@connor-mccarthy
Copy link
Member Author

/unhold

@connor-mccarthy connor-mccarthy force-pushed the if-elif-else branch 2 times, most recently from e750768 to 6628877 Compare September 8, 2023 19:53
@google-oss-prow
Copy link

@connor-mccarthy: 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-sdk-execution-tests fdde042 link false /test kubeflow-pipelines-sdk-execution-tests

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
chensun approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@chensun chensun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

Thanks!

@google-oss-prow google-oss-prow bot added the lgtm label Sep 11, 2023
@google-oss-prow
Copy link

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

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 google-oss-prow bot merged commit c6b236d into kubeflow:master Sep 11, 2023
stijntratsaertit pushed a commit to stijntratsaertit/kfp that referenced this pull request Feb 16, 2024
@connor-mccarthy @stijntratsaertit
feat(sdk): support dsl.If, dsl.Elif, and dsl.Else (kubeflow#9894)
f0e337b 
* support if/elif/else

* deprecate dsl.Condition

* alter rebase

* update release notes

* address review feedback

* change BinaryOperation to ConditionOperation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chensun chensun chensun approved these changes

@gkcalat gkcalat Awaiting requested review from gkcalat

@Linchin Linchin Awaiting requested review from Linchin

@james-jwu james-jwu Awaiting requested review from james-jwu

@zijianjoy zijianjoy Awaiting requested review from zijianjoy

Assignees

@chensun chensun

Labels
approved lgtm size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@connor-mccarthy @chensun