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.

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

fix(sdk): Prevents dsl.ParallelFor over single parameter from compiling. #10494

Merged
merged 15 commits into from
Feb 27, 2024

Conversation

KevinGrantLee
Copy link
Contributor

@KevinGrantLee KevinGrantLee commented Feb 15, 2024

Description of your changes:

Prevents the following pipeline from compiling

@dsl.component
def str_identity(s: str) -> str:
    return s


@dsl.pipeline
def my_pipeline():
  single_param = str_identity(s='string')
  with dsl.ParallelFor(items=single_param.output) as item:
      str_identity(s=item)

The component str_identity outputs a str so routing this to dsl.ParallelFor does not make sense.

Checklist:

Copy link
Member

@connor-mccarthy connor-mccarthy left a comment

Choose a reason for hiding this comment

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

Thank you, @KevinGrantLee!

sdk/python/kfp/dsl/for_loop_test.py Show resolved Hide resolved
sdk/python/kfp/dsl/for_loop.py Outdated Show resolved Hide resolved
Copy link
Member

@connor-mccarthy connor-mccarthy left a comment

Choose a reason for hiding this comment

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

Thanks, Kevin!

sdk/python/kfp/compiler/compiler_test.py Outdated Show resolved Hide resolved
sdk/python/kfp/compiler/compiler_test.py Outdated Show resolved Hide resolved
sdk/python/kfp/compiler/compiler_test.py Outdated Show resolved Hide resolved
sdk/python/kfp/dsl/for_loop.py Outdated Show resolved Hide resolved
sdk/python/kfp/dsl/for_loop.py Outdated Show resolved Hide resolved
sdk/python/kfp/dsl/for_loop.py Outdated Show resolved Hide resolved
Copy link
Member

@connor-mccarthy connor-mccarthy 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

Thank you, @KevinGrantLee!

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: connor-mccarthy

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

@KevinGrantLee
Copy link
Contributor Author

/retest

@KevinGrantLee
Copy link
Contributor Author

/retest-required

@KevinGrantLee
Copy link
Contributor Author

/retest

Copy link

@KevinGrantLee: 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
kfp-kubernetes-execution-tests d50d1e7 link false /test kfp-kubernetes-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.

@google-oss-prow google-oss-prow bot merged commit 144761c into kubeflow:master Feb 27, 2024
28 of 29 checks passed
@KevinGrantLee KevinGrantLee deleted the single-param2 branch February 28, 2024 00:04
petethegreat pushed a commit to petethegreat/pipelines that referenced this pull request Mar 27, 2024
…ng. (kubeflow#10494)

* fix(sdk): Prevents dsl.ParallelFor over single paramter from compiling.

* fix(sdk): Prevents dsl.ParallelFor over single paramter from compiling.

* update PR number in release notes

* formatting

* Add compiler_test.py test for single param compile failure

* Update some docstrings and add todo

* formatting

* Update sdk/python/kfp/compiler/compiler_test.py

Co-authored-by: Connor McCarthy <[email protected]>

* Update sdk/python/kfp/compiler/compiler_test.py

Co-authored-by: Connor McCarthy <[email protected]>

* Update sdk/python/kfp/dsl/for_loop.py

Co-authored-by: Connor McCarthy <[email protected]>

* Use print_and_return and other small changes

* typo

* typo

---------

Co-authored-by: Connor McCarthy <[email protected]>
petethegreat pushed a commit to petethegreat/pipelines that referenced this pull request Mar 29, 2024
…ng. (kubeflow#10494)

* fix(sdk): Prevents dsl.ParallelFor over single paramter from compiling.

* fix(sdk): Prevents dsl.ParallelFor over single paramter from compiling.

* update PR number in release notes

* formatting

* Add compiler_test.py test for single param compile failure

* Update some docstrings and add todo

* formatting

* Update sdk/python/kfp/compiler/compiler_test.py

Co-authored-by: Connor McCarthy <[email protected]>

* Update sdk/python/kfp/compiler/compiler_test.py

Co-authored-by: Connor McCarthy <[email protected]>

* Update sdk/python/kfp/dsl/for_loop.py

Co-authored-by: Connor McCarthy <[email protected]>

* Use print_and_return and other small changes

* typo

* typo

---------

Co-authored-by: Connor McCarthy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants