Expose "explicit-comms" option in shuffle-based dask_cudf functions by rjzamora · Pull Request #11576 · rapidsai/cudf

rjzamora · 2022-08-22T15:25:09Z

Description

This PR exposes an option to use Dask-CUDA's explicit-comms shuffle for the primary shuffle-based dask_cudf.DataFrame methods: shuffle, sort_values, and set_index. Although "explicit-comms" is still experimental, the explicit-shuffle algorithm is known to consistently outperform the "task"-based shuffle.

As far as I can tell, it is not currently possible to use an "explicit-comms" shuffle in dask_cudf without directly importing the function from Dask-CUDA (@madsbk - please do correct me if I am mistaken). In order to simplify benchmarking, and to utilize the optimized shuffle within high-cardinality groupby code, I propose that we make it easier to access the explicit shuffle.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

madsbk · 2022-08-23T09:44:26Z

As far as I can, it is not currently possible to use an "explicit-comms" shuffle in dask_cudf without directly importing the function from Dask-CUDA (@madsbk - please do correct me if I am mistaken).

Correct, and I think it is a good idea to introduce a new shuffle argument!
But notice, it should be possible to enable explicit-comms shuffle in Dask by setting DASK_EXPLICIT_COMMS=True and importing Dask-CUDA see: https://github.com/rapidsai/dask-cuda/blob/branch-22.10/dask_cuda/__init__.py#L21

rjzamora · 2022-08-23T14:05:17Z

But notice, it should be possible to enable explicit-comms shuffle in Dask by setting DASK_EXPLICIT_COMMS=True and importing Dask-CUDA

Aha - I thought I remembered there being a way to use explicit-comms from dask/dask_cudf. Thanks for pointing that out!

Do you know if anyone is using that config option? I do like that the monkey-patching approach enables explicit-comms for both cudf and pandas-backed data. However, I don't love that we are using explicit comms when shuffle="tasks". It would be more intuitive for there to be a distinct option, and for the default setting to be configurable (so that using ddf.shuffle(..., shuffle="tasks") will always use the task-based shuffle). Unfortunately, this behavior may be tricky to accomplish with the current interaction between dask and dask_cudf.

I may need to hold off on this PR for a bit until I can figure out a way to keep things simple.

python/dask_cudf/dask_cudf/core.py

…option

codecov · 2022-09-20T18:06:25Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.10@b2ffea7). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 6a99f1e differs from pull request most recent head 0cc0d22. Consider uploading reports for the commit 0cc0d22 to get more accurate results

Additional details and impacted files

@@               Coverage Diff               @@
##             branch-22.10   #11576   +/-   ##
===============================================
  Coverage                ?   87.40%           
===============================================
  Files                   ?      133           
  Lines                   ?    21833           
  Branches                ?        0           
===============================================
  Hits                    ?    19084           
  Misses                  ?     2749           
  Partials                ?        0

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.

python/dask_cudf/dask_cudf/core.py

wence-

A few small questions.

python/dask_cudf/dask_cudf/core.py

Motivated by discussions in rapidsai/cudf#11576 (primarilty [here](rapidsai/cudf#11576 (comment))). This PR updates the `rearrange_by_columns_tasks` wrapper to target `rearrange_by_columns` instead. The updated wrapper also handles the case that `shuffle="explicit-comms"`. Authors: - Richard (Rick) Zamora (https://github.com/rjzamora) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) - Lawrence Mitchell (https://github.com/wence-) URL: #992

rjzamora · 2022-09-27T15:08:07Z

@wence- @madsbk - This PR is now needed to utilize the changes in rapidsai/dask-cuda#992 in dask_cudf. That is, we need this PR to use the explicit-comms shuffle by using code like ddf.shuffle/sort_values/set_index/merge/join(..., shuffle="explicit-comms").

wence-

I think there is one possibility of a sequencing bug meaning that a call to sort_values wouldn't get an explicit-comms shuffle.

Other than that, a suggestion for refactoring _set_shuffle: WDYT?

python/dask_cudf/dask_cudf/sorting.py

rjzamora · 2022-09-27T19:05:58Z

@gpucibot merge

quasiben · 2022-09-27T20:57:01Z

I'll add @wence- to the list of reviewers for next time

@wence-

Due to some unfortunate issues with #11576 and rapidsai/dask-cuda#992, I feel that these PRs should be reverted before the 22.10 release. This PRs roll back some recent changes that allow users to explicitly pass `shuffle="explicit-comms"` to certain shuffle-based algorithms. cc @wence- Authors: - Richard (Rick) Zamora (https://github.com/rjzamora) Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) - Lawrence Mitchell (https://github.com/wence-) URL: #11803

rjzamora added 3 commits August 22, 2022 07:59

add explicit-comms shuffle option for sort_values and shuffle

edca3aa

use npartitions=4 in test

750a30b

use npartitions=4 in test

76d8faa

rjzamora added 2 - In Progress Currently a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 22, 2022

rjzamora self-assigned this Aug 22, 2022

github-actions bot added the Python Affects Python cuDF API. label Aug 22, 2022

wence- reviewed Aug 25, 2022

View reviewed changes

python/dask_cudf/dask_cudf/core.py Outdated Show resolved Hide resolved

rjzamora added 2 commits September 20, 2022 07:39

Merge remote-tracking branch 'upstream/branch-22.10' into ec-shuffle-…

b3de1a8

…option

use dask-config to support explicit-comms shuffle

eccf5a6

rjzamora marked this pull request as ready for review September 20, 2022 16:14

rjzamora requested a review from a team as a code owner September 20, 2022 16:14

rjzamora added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Sep 20, 2022

rjzamora added 2 commits September 20, 2022 09:25

move _shuffle_context

b01eef9

fix test

694b25b

use ec shuffle directly for repartitioning

9137662

rjzamora commented Sep 20, 2022

View reviewed changes

python/dask_cudf/dask_cudf/core.py Outdated Show resolved Hide resolved

Update python/dask_cudf/dask_cudf/core.py

cefbd40

rjzamora commented Sep 21, 2022

View reviewed changes

python/dask_cudf/dask_cudf/core.py Outdated Show resolved Hide resolved

wence- reviewed Sep 22, 2022

View reviewed changes

python/dask_cudf/dask_cudf/core.py Show resolved Hide resolved

python/dask_cudf/dask_cudf/core.py Outdated Show resolved Hide resolved

python/dask_cudf/dask_cudf/core.py Outdated Show resolved Hide resolved

rjzamora mentioned this pull request Sep 22, 2022

Update rearrange_by_column patch for explicit comms rapidsai/dask-cuda#992

Merged

rjzamora added 3 commits September 27, 2022 07:34

remove _shuffle_context and simplify shuffle-arg check

5b55cef

move _set_shuffle

6269e6e

test merge and join

6477952

wence- requested changes Sep 27, 2022

View reviewed changes

python/dask_cudf/dask_cudf/sorting.py Show resolved Hide resolved

python/dask_cudf/dask_cudf/sorting.py Outdated Show resolved Hide resolved

python/dask_cudf/dask_cudf/sorting.py Outdated Show resolved Hide resolved

address code review

6362797

rjzamora commented Sep 27, 2022

View reviewed changes

python/dask_cudf/dask_cudf/sorting.py Outdated Show resolved Hide resolved

fix join test

0cc0d22

wence- approved these changes Sep 27, 2022

View reviewed changes

rjzamora added 5 - Ready to Merge Testing and reviews complete, ready to merge 4 - Needs Dask Reviewer and removed 3 - Ready for Review Ready for review by team 5 - Ready to Merge Testing and reviews complete, ready to merge labels Sep 27, 2022

quasiben approved these changes Sep 27, 2022

View reviewed changes

rapids-bot bot merged commit bcf361f into rapidsai:branch-22.10 Sep 27, 2022

rjzamora deleted the ec-shuffle-option branch September 27, 2022 21:01

rjzamora mentioned this pull request Sep 28, 2022

Support shuffle-based groupby aggregations in dask_cudf #11800

Merged

3 tasks

rjzamora added a commit to rjzamora/cudf that referenced this pull request Sep 28, 2022

revert rapidsai#11576

a39b8aa

rjzamora mentioned this pull request Sep 28, 2022

Revert problematic shuffle=explicit-comms changes #11803

Merged

3 tasks

vyasr added 4 - Needs Review Waiting for reviewer to review or respond and removed 4 - Needs Dask Reviewer labels Feb 23, 2024

Conversation

rjzamora commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

madsbk commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjzamora commented Aug 23, 2022

Uh oh!

Uh oh!

codecov bot commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

wence- left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rjzamora commented Sep 27, 2022

Uh oh!

wence- left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rjzamora commented Sep 27, 2022

Uh oh!

quasiben commented Sep 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rjzamora commented Aug 22, 2022 •

edited

Loading

madsbk commented Aug 23, 2022 •

edited

Loading

codecov bot commented Sep 20, 2022 •

edited

Loading