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

enable parallelism to be set to nil for array node #5214

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

pvditt
Copy link
Contributor

@pvditt pvditt commented Apr 10, 2024

Tracking issue

#3924

Why are the changes needed?

  • Currently, utilizing workflow parallelism is not backwards compatible as we want to default ArrayNode maptasks to utilize workflow parallelism.
    Setting concurrency=-1 on in flytekit would cause an error in flytekit if a user does not bump their flyte/flyteidl version.

What changes were proposed in this pull request?

  • enable parallelism to nil-able.
  • on the flytekit end, we will default concurrency=None. Before this merge, None would map to 0 which was the existing ArrayNode behavior. With this change, None would map to nil (utilizing the wf's parallelism)

How was this patch tested?

  • Ran ArrayNode map tasks w/ setting concurrency as None, 0, 1

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

flyteorg/flytekit#2268

Docs link

@pvditt pvditt marked this pull request as ready for review April 10, 2024 21:02
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. enhancement New feature or request labels Apr 10, 2024
Signed-off-by: Paul Dittamo <[email protected]>
@pvditt pvditt requested a review from hamersaw April 10, 2024 21:35
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 11, 2024
@pvditt pvditt merged commit ab95f7e into master Apr 11, 2024
46 checks passed
@pvditt pvditt deleted the update-array-node-parallelism-handling branch April 11, 2024 18:27
pmahindrakar-oss pushed a commit that referenced this pull request May 1, 2024
* Feature/array node workflow parallelism (#5062)

* update arraynode proto parallelism field to varint compatible int64

Signed-off-by: Paul Dittamo <[email protected]>

* have array nodes utilize workflow parallelism

Signed-off-by: Paul Dittamo <[email protected]>

* return if available parallelism is 0

Signed-off-by: Paul Dittamo <[email protected]>

* unit test

Signed-off-by: Paul Dittamo <[email protected]>

---------

Signed-off-by: Paul Dittamo <[email protected]>

* enable parallelism to be set to nil for array node (#5214)

* enable parallelism to be set to nil for array node

Signed-off-by: Paul Dittamo <[email protected]>

* unit test

Signed-off-by: Paul Dittamo <[email protected]>

---------

Signed-off-by: Paul Dittamo <[email protected]>

* added configuration for arraynode default parallelism behavior (#5268)

* added configuration for arraynode default parallelism behavior

Signed-off-by: Daniel Rammer <[email protected]>

* added unit tests and fixed linter

Signed-off-by: Daniel Rammer <[email protected]>

* cleanup / docs

Signed-off-by: Daniel Rammer <[email protected]>

* fixed ytpo

Signed-off-by: Daniel Rammer <[email protected]>

* docs update

Signed-off-by: Daniel Rammer <[email protected]>

* fixed unit tests

Signed-off-by: Daniel Rammer <[email protected]>

---------

Signed-off-by: Daniel Rammer <[email protected]>

---------

Signed-off-by: Paul Dittamo <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Co-authored-by: Paul Dittamo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants