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] Fixes memory allocation for experimental backend and improves quantile computations #3586

Merged
merged 40 commits into from
Mar 17, 2021
Merged

[REVIEW] Fixes memory allocation for experimental backend and improves quantile computations #3586

merged 40 commits into from
Mar 17, 2021

Conversation

vinaydes
Copy link
Contributor

@vinaydes vinaydes commented Mar 8, 2021

Previous to this PR, when new/experimental backend is used for training, the temporary memory needed by old backend is also getting allocated. This PR fixes the issue. The temporary memory is allocated conditionally now. This PR also changes the computation of quantiles for new backend. The old way of computing quantiles may leave last few samples due to incorrect quantile thresholds. Impact on accuracy is still to be evaluated thoroughly.

@vinaydes
Added new quantile computation code
aa09671
@vinaydes
Removing commented code
4eec348
@vinaydes
Adding quantile argument to decision tree fit function and other mino…
d4a0d21 
…r changes
@vinaydes
Changing RF code to handle temporary memory allocation when new backe…
1d729da 
…nd is used
@vinaydes
Making necessary changes in decision tree API to pass global quantile…
8af51b7 
… argument
@vinaydes
Simplifying branching in RF fit functions and fixing deallocation
4633080
@vinaydes
Dumping pre-plant data to file instead of stdout
e7f52ce
@vinaydes
Removing inconsistent printing
e580ac8
@vinaydes
Computing qunatiles in DT API
3251bb9
@vinaydes
Corrections related to quatile size computations and clean-up
0e3820e
@vinaydes
Changing the entry point for batched backend
a52a109
@vinaydes
Chaning the quantile variable data type to address crash issue
8c4aa53
@vinaydes
Merge branch 'branch-0.19' into fix-rf-memory-allocation
3955f27
@vinaydes
Including .h instead of .cuh as .cuh is included randomforest_impl.cuh
a95a3a3
@vinaydes
Removing debug prints from quantile computations
2ea29fa
@vinaydes
Minor changes
004e71e
@vinaydes
Undoing more minor changes
a51c471
@vinaydes
Undoing even more minor changes
80e402b
@vinaydes
Blank line added
7104450
@vinaydes
Removing commented code
0e1bc6a
@vinaydes
Whitespace and other minor changes
774eae4
@vinaydes
Matching whitespaces with branch-0.19
7bf0e23
@vinaydes
Using correct raft header and fixing call to computeQuantilesSorted
8d94e1e
@vinaydes
Removing additional debug prints
0aa5c97
@vinaydes
Fixing formatting
8a87dbb
@vinaydes
Fixing more formatting issues
f01e0b6
@vinaydes
Deleting debug code
619ea7d
@vinaydes
Brining RF classifier and regressor functions on parity with each other
51ef99e
@vinaydes
Readding the experimental backend warning
25118f2
@vinaydes
Removed unused argument from computeQuantiles()
5634ff2
teju85
teju85 requested changes Mar 8, 2021
View reviewed changes
cpp/src/decisiontree/decisiontree.cu Outdated Show resolved Hide resolved
cpp/src/decisiontree/decisiontree.cu Outdated Show resolved Hide resolved
cpp/src/decisiontree/decisiontree.cu Outdated Show resolved Hide resolved
cpp/src/decisiontree/decisiontree.cu Outdated Show resolved Hide resolved
cpp/src/decisiontree/quantile/quantile.cuh Outdated Show resolved Hide resolved
cpp/src/randomforest/randomforest_impl.cuh Outdated Show resolved Hide resolved
cpp/src/randomforest/randomforest_impl.cuh Outdated Show resolved Hide resolved
cpp/src/decisiontree/quantile/quantile.cuh Outdated Show resolved Hide resolved
@teju85 teju85 added non-breaking Non-breaking change 2 - In Progress Currenty a work in progress bug Something isn't working improvement Improvement / enhancement to an existing function and removed improvement Improvement / enhancement to an existing function labels Mar 8, 2021
teju85
teju85 approved these changes Mar 8, 2021
View reviewed changes
Copy link
Member

@teju85 teju85 left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

cpp/src/decisiontree/quantile/quantile.cuh Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #3586 (a33703c) into branch-0.19 (a3bfb36) will increase coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #3586      +/-   ##
===============================================
+ Coverage        80.69%   80.83%   +0.13%     
===============================================
  Files              227      227              
  Lines            17615    17737     +122     
===============================================
+ Hits             14214    14337     +123     
+ Misses            3401     3400       -1
Flag Coverage Δ
dask 45.30% <ø> (+0.31%) ⬆️
non-dask 73.09% <ø> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/cuml/dask/solvers/cd.py 100.00% <0.00%> (ø)
python/cuml/common/numba_utils.py 0.00% <0.00%> (ø)
python/cuml/internals/global_settings.py 100.00% <0.00%> (ø)
python/cuml/dask/preprocessing/encoders.py 100.00% <0.00%> (ø)
python/cuml/metrics/_ranking.py 98.57% <0.00%> (+0.02%) ⬆️
python/cuml/preprocessing/encoders.py 95.10% <0.00%> (+0.02%) ⬆️
python/cuml/common/array_descriptor.py 98.24% <0.00%> (+0.03%) ⬆️
python/cuml/common/array.py 96.99% <0.00%> (+0.04%) ⬆️
python/cuml/dask/common/dask_arr_utils.py 95.83% <0.00%> (+0.05%) ⬆️
python/cuml/dask/cluster/dbscan.py 97.29% <0.00%> (+0.07%) ⬆️
... and 34 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 a3bfb36...a33703c. Read the comment docs.

@vinaydes vinaydes changed the title [WIP] Fixes memory allocation for experimental backend and improves quantile computations [REVIEW] Fixes memory allocation for experimental backend and improves quantile computations Mar 8, 2021
@hcho3 hcho3 self-assigned this Mar 8, 2021
@dantegd dantegd added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 2 - In Progress Currenty a work in progress labels Mar 9, 2021
@teju85
Copy link
Member

teju85 commented Mar 10, 2021

This should help address issue #3529

@teju85
Copy link
Member

teju85 commented Mar 10, 2021

This should help also address issue #3530

hcho3
hcho3 approved these changes Mar 11, 2021
View reviewed changes
Copy link
Contributor

@hcho3 hcho3 left a comment

Choose a reason for hiding this comment

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

LGTM. I'm confident that this PR will reduce memory consumption of the new RF backend.

cpp/src/decisiontree/quantile/quantile.cuh Outdated Show resolved Hide resolved
@vinaydes vinaydes requested a review from a team as a code owner March 15, 2021 14:47
@github-actions github-actions bot added the CMake label Mar 15, 2021
@vinaydes vinaydes mentioned this pull request Mar 16, 2021
Merged
@JohnZed
Copy link
Contributor

JohnZed commented Mar 17, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit de42e7f into rapidsai:branch-0.19 Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teju85 teju85 teju85 approved these changes

@hcho3 hcho3 hcho3 approved these changes

Assignees

@hcho3 hcho3

Labels
4 - Waiting on Reviewer Waiting for reviewer to review or respond bug Something isn't working CMake non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@vinaydes @codecov-io @teju85 @JohnZed @hcho3 @dantegd