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

added configuration for arraynode default parallelism behavior #5268

Merged
merged 7 commits into from
Apr 23, 2024

Conversation

hamersaw
Copy link
Contributor

@hamersaw hamersaw commented Apr 23, 2024

Tracking issue

NA

Why are the changes needed?

Defaulting to use the workflow parallleism in ArrayNode rather than unlimited may confuse some users and result in unexpected behavior.

What changes were proposed in this pull request?

This PR introduces ArrayNode default parallelism configuration, with three options:

  • unlimited: works as the legacy maptask parallelism where all subNodes are evaluated
  • workflow: applies the parent workflow parallelism to ArrayNode subNode evaluations
  • hybrid: the previous changes allowed differentiation withing flytekit, where nil / None parallelism indicates using the parent workflow and 0 indicates using unlimited.

How was this patch tested?

Extensive unit testing and deployed locally in multiple scenarios / configurations.

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

#5214
flyteorg/flytekit#2268

Docs link

NA

@hamersaw hamersaw marked this pull request as ready for review April 23, 2024 14:26
@hamersaw hamersaw requested review from pvditt and pingsutw April 23, 2024 14:26
Signed-off-by: Daniel Rammer <[email protected]>
pvditt
pvditt previously approved these changes Apr 23, 2024
Copy link
Contributor

@pvditt pvditt left a comment

Choose a reason for hiding this comment

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

definitely a lot clearer implementation. Left a nit/possible update to the config description.

pvditt
pvditt previously approved these changes Apr 23, 2024
@hamersaw hamersaw enabled auto-merge (squash) April 23, 2024 17:51
@hamersaw hamersaw disabled auto-merge April 23, 2024 20:44
Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw hamersaw merged commit 6b03731 into master Apr 23, 2024
46 checks passed
@hamersaw hamersaw deleted the feature/arraynode-parallelism-config branch April 23, 2024 21:30
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]>
austin362667 pushed a commit to austin362667/flyte that referenced this pull request May 7, 2024
…org#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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants