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

Test broadcast merge in local_cudf_merge benchmark #507

Merged
merged 4 commits into from
Mar 5, 2021

Conversation

rjzamora
Copy link
Member

@rjzamora rjzamora commented Jan 29, 2021

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

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

@jakirkham
Copy link
Member

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

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

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

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.

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 !

dask_cuda/benchmarks/local_cudf_merge.py Outdated Show resolved Hide resolved
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

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

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

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

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

@jakirkham
Copy link
Member

jakirkham commented Mar 5, 2021

rerun tests

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

@jakirkham
Copy link
Member

@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
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
Development

Successfully merging this pull request may close these issues.

5 participants