Skip to content

Test broadcast merge in local_cudf_merge benchmark#507

Merged
rapids-bot[bot] merged 4 commits intorapidsai:branch-0.19from
rjzamora:mismatched-chunks
Mar 5, 2021
Merged

Test broadcast merge in local_cudf_merge benchmark#507
rapids-bot[bot] merged 4 commits intorapidsai:branch-0.19from
rjzamora:mismatched-chunks

Conversation

@rjzamora
Copy link
Member

@rjzamora rjzamora commented Jan 29, 2021 •
edited
Loading

Includes the necessary changes to test dask#7143. More specifically, this adds the following options:

  • --base-chunks : Number of base-DataFrame partitions (default: n_workers)
  • --other-chunks : Number of other-DataFrame partitions (default: n_workers)
  • --broadcast-join : Use broadcast join when possible
  • --shuffle-join : Use shuffle join (takes precedence over '--broadcast-join')

@rjzamora rjzamora requested a review from a team as a code owner January 29, 2021 22:43
@rjzamora rjzamora marked this pull request as draft January 29, 2021 22:43
@quasiben
Copy link
Member

quasiben commented Feb 2, 2021

run tests

@rjzamora
Copy link
Member Author

rjzamora commented Mar 4, 2021

Now that dask#7143 was merged, it is now safe to consider this PR for integration.

@pentschev pentschev added 2 - In Progress Currently a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 4, 2021
@pentschev pentschev changed the base branch from branch-0.18 to branch-0.19 March 4, 2021 19:42
@pentschev
Copy link
Member

pentschev commented Mar 4, 2021

Retargeting seems to have no conflicts, let's see what the tests say. 🙂

@jakirkham
Copy link
Member

jakirkham commented Mar 4, 2021

Could you please run black locally Rick and push the changes? Currently failing the style job. Idk if the others will run without that

@pentschev
Copy link
Member

pentschev commented Mar 4, 2021

Idk if the others will run without that

They still run, so we can wait another hour or so to see the results, but feel free to restyle it anyway in the meantime Rick.

@jakirkham
Copy link
Member

jakirkham commented Mar 4, 2021

Ah ok. If they still run, great let's wait

Know that previously changelog (ofc no longer included) and style were required to pass before running tests. Given this, couldn't tell if the jobs here were actually queued to run or maybe we were running into some edge case in gpuCI where they only appear queued (but don't actually run).

@github-actions github-actions bot added the python python code needed label Mar 4, 2021
@rjzamora rjzamora marked this pull request as ready for review March 4, 2021 21:17
@codecov-io
Copy link

codecov-io commented Mar 4, 2021 •
edited
Loading

Codecov Report

Merging #507 (77346b7) into branch-0.19 (f99a037) will increase coverage by 1.39%.
The diff coverage is 0.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19     #507      +/-   ##
===============================================
+ Coverage        62.37%   63.77%   +1.39%     
===============================================
  Files               22       22              
  Lines             2517     2523       +6     
===============================================
+ Hits              1570     1609      +39     
+ Misses             947      914      -33
Impacted Files Coverage Δ
dask_cuda/benchmarks/local_cudf_merge.py 0.00% <0.00%> (ø)
dask_cuda/explicit_comms/dataframe/shuffle.py 97.94% <0.00%> (+0.68%) ⬆️
dask_cuda/_version.py 44.80% <0.00%> (+0.71%) ⬆️
dask_cuda/utils.py 90.86% <0.00%> (+1.02%) ⬆️
dask_cuda/proxy_object.py 91.36% <0.00%> (+1.11%) ⬆️
dask_cuda/cli/dask_cuda_worker.py 96.96% <0.00%> (+1.51%) ⬆️
dask_cuda/device_host_file.py 90.90% <0.00%> (+2.27%) ⬆️
dask_cuda/initialize.py 92.59% <0.00%> (+3.70%) ⬆️
dask_cuda/proxify_device_objects.py 90.16% <0.00%> (+9.83%) ⬆️
dask_cuda/get_device_memory_objects.py 89.04% <0.00%> (+19.17%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f99a037...b80c70a. Read the comment docs.

pentschev
pentschev reviewed Mar 4, 2021
View reviewed changes
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.

I left a minor nitpick, but other than that LGTM. Thanks @rjzamora !

@rjzamora @pentschev
Update dask_cuda/benchmarks/local_cudf_merge.py
b80c70a 
Co-authored-by: Peter Andreas Entschev <peter@entschev.com>
pentschev
pentschev approved these changes Mar 4, 2021
View reviewed changes
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.

Thanks @rjzamora !

@pentschev pentschev added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 2 - In Progress Currently a work in progress labels Mar 4, 2021
@jakirkham
Copy link
Member

jakirkham commented Mar 5, 2021

Looks like we are seeing some failures in test_dataframe_merge (subprocess is exiting with a non-zero exit code)

@rjzamora
Copy link
Member Author

rjzamora commented Mar 5, 2021

Looks like we are seeing some failures in test_dataframe_merge (subprocess is exiting with a non-zero exit code)

Hmm - It doesn't seem like that test is running the benchmark modified in this PR, is it?

@jakirkham
Copy link
Member

jakirkham commented Mar 5, 2021

Was this from a change in Dask or Distributed then? Both of those are planning to be released tomorrow. So if they have issues, we should try to identify those quickly

@jakirkham
Copy link
Member

jakirkham commented Mar 5, 2021

Just to update this thread, Rick did find a recent Dask change that is causing failures here as mentioned in this comment ( dask/dask#7305 (comment) ) in the PR with the change. We have surfaced this in the Dask release issue ( dask/community#129 (comment) ). Trying to figure out now how best to address this before the Dask + Distributed release tomorrow

@pentschev
Copy link
Member

pentschev commented Mar 5, 2021

I'm attempting to fix the issue mentioned above in dask/dask#7325 .

@jakirkham
Copy link
Member

jakirkham commented Mar 5, 2021 •
edited
Loading

rerun tests

Edit: As Peter's fix has landed. Thanks Peter! 😄

@jakirkham
Copy link
Member

jakirkham commented Mar 5, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit e72d776 into rapidsai:branch-0.19 Mar 5, 2021
@rjzamora rjzamora deleted the mismatched-chunks branch March 5, 2021 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pentschev pentschev pentschev approved these changes

Assignees

No one assigned

Labels

5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python python code needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@rjzamora @quasiben @pentschev @jakirkham @codecov-io @Nicholas-7