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

Explicit-comms-shuffle: fine control of task scheduling #1025

Merged
merged 5 commits into from
Oct 28, 2022

Conversation

madsbk
Copy link
Member

@madsbk madsbk commented Oct 26, 2022

In shuffle, use Client.submit() to control where tasks are executed and release temporary dataframes ASAP.

Context

In the final step in explicit-comms shuffle, we call getitem() to extract the final dataframe partitions from the result of the local shuffle. In some cases, these getitem() tasks would not run on the worker that ran the local shuffle, which would result in extra communication and spilling.
We now use submit(..., worker=...) to make sure that the worker running the local shuffle also runs the getitem() task afterwards.

Is it possible to do this without the use of submit() to avoid the overhead of creating a Future for each dataframe partition?

@github-actions github-actions bot added the python python code needed label Oct 26, 2022
@madsbk madsbk added 2 - In Progress Currently a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed python python code needed labels Oct 26, 2022
@madsbk madsbk marked this pull request as ready for review October 26, 2022 14:27
@madsbk madsbk requested a review from a team as a code owner October 26, 2022 14:27
@madsbk madsbk added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Oct 26, 2022
Copy link
Member

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

Only added a minor code duplication reduction, apart from that LGTM. Thanks @madsbk !

dask_cuda/explicit_comms/dataframe/shuffle.py Outdated Show resolved Hide resolved
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

My only comment was similar to @pentschev's; don't know a better way to force location of the compute

dask_cuda/explicit_comms/dataframe/shuffle.py Outdated Show resolved Hide resolved
Co-authored-by: Peter Andreas Entschev <[email protected]>
@github-actions github-actions bot added the python python code needed label Oct 28, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2022

Codecov Report

Base: 0.00% // Head: 0.00% // No change to project coverage 👍

Coverage data is based on head (658c3da) compared to base (99894b1).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff               @@
##           branch-22.12   #1025     +/-   ##
==============================================
  Coverage          0.00%   0.00%             
==============================================
  Files                25      17      -8     
  Lines              3315    2216   -1099     
==============================================
+ Misses             3315    2216   -1099     
Impacted Files Coverage Δ
dask_cuda/explicit_comms/dataframe/shuffle.py 0.00% <0.00%> (ø)
dask_cuda/benchmarks/local_cupy.py
dask_cuda/_version.py
dask_cuda/benchmarks/local_cudf_merge.py
dask_cuda/benchmarks/local_cudf_shuffle.py
dask_cuda/benchmarks/common.py
dask_cuda/benchmarks/local_cupy_map_overlap.py
dask_cuda/benchmarks/local_cudf_groupby.py
dask_cuda/benchmarks/utils.py

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pentschev
Copy link
Member

rerun tests

@pentschev
Copy link
Member

Thanks @madsbk !

@pentschev
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 40bbfed into rapidsai:branch-22.12 Oct 28, 2022
@madsbk madsbk deleted the excomm_enforce_worker branch October 31, 2022 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python python code needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants