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 Parallelism in ParallelFor in KFP SDK V2 #8146

Merged
merged 10 commits into from
Aug 17, 2022
Merged

feat(sdk): Support Parallelism in ParallelFor in KFP SDK V2 #8146

merged 10 commits into from
Aug 17, 2022

Conversation

bokleung
Copy link
Contributor

Description of your changes:

  • Updated ParallelFor under tasks_group to handle parallelism setting
  • Updated Compiler to compile non-default parallelism setting in ParallelFor into iteratorPolicy in IR
  • Added related tests

Checklist:

@google-oss-prow
Copy link

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

@bokleung bokleung marked this pull request as ready for review August 15, 2022 21:49
@connor-mccarthy
Copy link
Member

/ok-to-test

connor-mccarthy
connor-mccarthy approved these changes Aug 15, 2022
View reviewed changes
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.

Looks good, @bokleung! Just a few small nitpicks and one larger authoring API consideration.

FYI, the pre-commit file can help with the isort test failure and the unused imports.

sdk/python/kfp/components/tasks_group.py Outdated Show resolved Hide resolved
sdk/python/kfp/compiler/test_data/pipelines/pipeline_with_parallelfor_parallelism.py Outdated Show resolved Hide resolved
sdk/python/kfp/compiler/test_data/pipelines/pipeline_with_parallelfor_parallelism.py 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/components/tasks_group_test.py Outdated Show resolved Hide resolved
@connor-mccarthy
Copy link
Member

Undo accidental approval above
/remove-approve

@connor-mccarthy
Copy link
Member

/retest

connor-mccarthy
connor-mccarthy reviewed Aug 16, 2022
View reviewed changes
Copy link
Member

@connor-mccarthy connor-mccarthy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks great, @bokleung. One last thing: can you add a release note to ./sdk/RELEASE.md? Then we will be all set to merge.

Because we just updated the release file on master, you'll have to first git fetch upstream and git rebase upstream/master or git merge upstream/master.

@connor-mccarthy
Copy link
Member

/retest

1 similar comment
@connor-mccarthy
Copy link
Member

/retest

@bokleung
Copy link
Contributor Author

/retest

@connor-mccarthy
Copy link
Member

/lgtm
/approve

Great work, @bokleung! Thanks so much for this contribution.

@connor-mccarthy
Copy link
Member

/retest

@google-oss-prow google-oss-prow bot removed the lgtm label Aug 17, 2022
connor-mccarthy
connor-mccarthy requested changes Aug 17, 2022
View reviewed changes
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, @bokleung. Just two small things.

sdk/python/kfp/components/tasks_group.py Outdated Show resolved Hide resolved
sdk/python/kfp/components/tasks_group.py Outdated Show resolved Hide resolved
@connor-mccarthy
Copy link
Member

/lgtm
/approve

Thanks, @bokleung! Great work.

@google-oss-prow google-oss-prow bot added the lgtm label Aug 17, 2022
@google-oss-prow
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

@google-oss-prow google-oss-prow bot merged commit 51bea09 into kubeflow:master Aug 17, 2022
jlyaoyuli pushed a commit to jlyaoyuli/pipelines that referenced this pull request Jan 5, 2023
@bokleung @jlyaoyuli
feat(sdk): Support Parallelism in ParallelFor in KFP SDK V2 (kubeflow…
1fd9836 
…#8146)

* feat(sdk): add support for ParallelFor parallelism setting

* add parallelism value check

* Add unit tests

* Adding a compiler test

* used None as default parallelism input signature, other minor fixes on format and tests

* fix import statements

* add release.md message

* update ParallelFor docstring

* fixed docstring comments

* removed 'optional' in docstrings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@connor-mccarthy connor-mccarthy connor-mccarthy requested changes

@chensun chensun Awaiting requested review from chensun

Assignees

@connor-mccarthy connor-mccarthy

Labels
approved lgtm ok-to-test size/XL
Projects
None yet
Milestone
No milestone
Development

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