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

Feature/array node workflow parallelism #2268

Merged
merged 5 commits into from
Apr 18, 2024

Conversation

pvditt
Copy link
Contributor

@pvditt pvditt commented Mar 15, 2024

Tracking issue

flyteorg/flyte#3924

Why are the changes needed?

Want to add support for array node map tasks to utilize threads under the parent workflow's set parallelism. Decrease confusion on default/expected map task parallelism

What changes were proposed in this pull request?

Set default parallelism to None. If a user bumps their Flyte version to include flyteorg/flyte#5214 this will default the array node map task to utilize the parent wf's parallelism. If a user has an older flyte version this will map to 0, which will maintain the existing parallelism behavior.

NOTE this will cause cache misses on the first re-runs as the hashed name will change

How was this patch tested?

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/flyte#5062 + flyteorg/flyte#5214 were already merged

Docs link

pvditt added 2 commits March 15, 2024 00:37
Signed-off-by: Paul Dittamo <[email protected]>
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.57%. Comparing base (f4d894a) to head (451fd03).
Report is 22 commits behind head on master.

❗ Current head 451fd03 differs from pull request most recent head 3f596c2. Consider uploading reports for the commit 3f596c2 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2268       +/-   ##
===========================================
- Coverage   83.09%   53.57%   -29.52%     
===========================================
  Files         324      180      -144     
  Lines       24860    18133     -6727     
  Branches     3761     3547      -214     
===========================================
- Hits        20657     9715    -10942     
- Misses       3582     7991     +4409     
+ Partials      621      427      -194     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pvditt pvditt marked this pull request as ready for review March 15, 2024 18:25
@pvditt pvditt requested a review from hamersaw March 15, 2024 20:59
@pvditt pvditt requested a review from samhita-alla as a code owner April 4, 2024 23:01
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Apr 4, 2024
@eapolinario
Copy link
Collaborator

Can you fix the unit test failure? The error:

FAILED tests/flytekit/unit/core/test_array_node_map_task.py::test_inputs_outputs_length - AssertionError: assert equals failed
  'tests.flytekit.unit.core.test_  'tests.flytekit.unit.core.test_ 
  array_node_map_task.map_many_in  array_node_map_task.map_many_in 
  puts_-6�b^3b�d0^353d�a5d^e6e�8-4d7�9-82921�  puts_b^f51001578�d0^ae197�a5+2c0af0a� 
  -ead2b3�-arraynode'                +99�d^d�89-arraynode'

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Apr 18, 2024
@pvditt
Copy link
Contributor Author

pvditt commented Apr 18, 2024

Can you fix the unit test failure? The error:

FAILED tests/flytekit/unit/core/test_array_node_map_task.py::test_inputs_outputs_length - AssertionError: assert equals failed
  'tests.flytekit.unit.core.test_  'tests.flytekit.unit.core.test_ 
  array_node_map_task.map_many_in  array_node_map_task.map_many_in 
  puts_-6�b^3b�d0^353d�a5d^e6e�8-4d7�9-82921�  puts_b^f51001578�d0^ae197�a5+2c0af0a� 
  -ead2b3�-arraynode'                +99�d^d�89-arraynode'

@eapolinario oof on my end. Just updated - will re-request your review after the builds pass

@dosubot dosubot bot added the lgtm This PR has been approved by maintainer label Apr 18, 2024
@eapolinario eapolinario merged commit 824353d into master Apr 18, 2024
47 checks passed
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
* default array node concurrency to -1

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

* typo

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

* set default concurrency to None for backwards compatibility

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

* update unit test - hashed name

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

---------

Signed-off-by: Paul Dittamo <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants