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.

Sign up for GitHub

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

Jump to bottom

[REVIEW]Expose rmm-maximum_pool_size argument #827

Merged
merged 8 commits into from
Jan 12, 2022
Merged

[REVIEW]Expose rmm-maximum_pool_size argument #827

merged 8 commits into from
Jan 12, 2022

Conversation

VibhuJawa
Copy link
Member

This PR closes #826

@github-actions github-actions bot added the python python code needed label Jan 12, 2022
@VibhuJawa VibhuJawa marked this pull request as ready for review January 12, 2022 16:35
@VibhuJawa VibhuJawa requested a review from a team as a code owner January 12, 2022 16:35
@VibhuJawa VibhuJawa changed the title [WIP]Expose rmm-maximum_pool_size argument [REVIEW]Expose rmm-maximum_pool_size argument Jan 12, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2022 •
edited
Loading

Codecov Report

Merging #827 (53e8572) into branch-22.02 (172e449) will increase coverage by 2.03%.
The diff coverage is 84.61%.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.02     #827      +/-   ##
================================================
+ Coverage         87.07%   89.10%   +2.03%     
================================================
  Files                16       16              
  Lines              2065     2074       +9     
================================================
+ Hits               1798     1848      +50     
+ Misses              267      226      -41
Impacted Files Coverage Δ
dask_cuda/cuda_worker.py 77.01% <50.00%> (-0.64%) ⬇️
dask_cuda/local_cuda_cluster.py 77.98% <66.66%> (-0.33%) ⬇️
dask_cuda/cli/dask_cuda_worker.py 97.22% <100.00%> (+1.44%) ⬆️
dask_cuda/utils.py 84.44% <100.00%> (+1.29%) ⬆️
dask_cuda/proxify_host_file.py 94.23% <0.00%> (+0.54%) ⬆️
dask_cuda/explicit_comms/dataframe/shuffle.py 98.69% <0.00%> (+0.65%) ⬆️
dask_cuda/proxy_object.py 91.74% <0.00%> (+1.11%) ⬆️
dask_cuda/device_host_file.py 71.66% <0.00%> (+1.66%) ⬆️
... and 4 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 172e449...53e8572. Read the comment docs.

pentschev
pentschev requested changes Jan 12, 2022
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.

Looks good overall, just have a few minor requests on docstrings.

I don't know if there's a way to do that via Python RMM API, but do you know if we can query that value and add that in a test @VibhuJawa ?

dask_cuda/cli/dask_cuda_worker.py Outdated Show resolved Hide resolved
dask_cuda/cli/dask_cuda_worker.py Outdated Show resolved Hide resolved
dask_cuda/local_cuda_cluster.py Outdated Show resolved Hide resolved
dask_cuda/local_cuda_cluster.py Outdated Show resolved Hide resolved
VibhuJawa and others added 4 commits January 12, 2022 22:28
@VibhuJawa @pentschev
Update dask_cuda/cli/dask_cuda_worker.py
b804353 
Co-authored-by: Peter Andreas Entschev <[email protected]>
@VibhuJawa @pentschev
Update dask_cuda/cli/dask_cuda_worker.py
c024f08 
Co-authored-by: Peter Andreas Entschev <[email protected]>
@VibhuJawa @pentschev
Update dask_cuda/local_cuda_cluster.py
afe0046 
Co-authored-by: Peter Andreas Entschev <[email protected]>
@VibhuJawa @pentschev
Update dask_cuda/local_cuda_cluster.py
b1bc7e6 
Co-authored-by: Peter Andreas Entschev <[email protected]>
@VibhuJawa
Copy link
Member Author

Thanks for the suggestions on doc strings.

I don't know if there's a way to do that via Python RMM API, but do you know if we can query that value and add that in a test @VibhuJawa ?

I have been trying to search for that but i could not find a way to query it. I dont think we expose that for the PoolMemoryResource . Maybe @shwina knows.

@pentschev pentschev added 3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change labels Jan 12, 2022
@pentschev
Copy link
Member

rerun tests

pentschev
pentschev approved these changes Jan 12, 2022
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 @VibhuJawa !

@pentschev
Copy link
Member

@gpucibot merge

1 similar comment
@pentschev
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4fd5f64 into rapidsai:branch-22.02 Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charlesbluca charlesbluca charlesbluca left review comments

@pentschev pentschev pentschev approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team feature request New feature or request 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.

[FEA] Expose rmm maximum_pool_size to LocalCUDACluster and dask-cuda-worker API
4 participants
@VibhuJawa @codecov-commenter @pentschev @charlesbluca