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] Miscellaneous tech debts/cleanups #286

Merged
merged 25 commits into from
Aug 27, 2021
Merged

[REVIEW] Miscellaneous tech debts/cleanups #286

merged 25 commits into from
Aug 27, 2021

Conversation

viclafargue
Copy link
Contributor

@viclafargue viclafargue commented Jul 1, 2021 •
edited
Loading

Miscellaneous updates to solve tech debts in RAFT :

  • Removal of handle host and device allocators
  • Addition of a get_thrust_policy method to the handle
  • Usage of get_thrust_policy where handle is available
  • Removal of rmm::device_vector
  • Use of RMM device allocator in the raft::allocate function
  • Creation of an allocation + deallocation helper system
  • Usage of rmm::exec_policy instead of thrust::cuda::par.on when no handle is available

@viclafargue viclafargue requested review from a team as code owners July 1, 2021 10:18
@viclafargue viclafargue requested a review from divyegala July 1, 2021 10:19
@divyegala
Copy link
Member

@viclafargue @dantegd should we keep the host allocator on the handle? RMM does not provide a singleton instance for a host allocator, just wrapper classes around two types of allocators: new-delete and pinned memory. Alternatively, I have not yet seen the host allocator being used anywhere, and since pinned memory isn't really our use-case, we can remove it and let the developer allocate host memory as they see fit

@viclafargue viclafargue force-pushed the miscellaneous-raft-updates branch from a3c9392 to f6fe37a Compare July 5, 2021 10:04
divyegala
divyegala requested changes Jul 6, 2021
View reviewed changes
Copy link
Member

@divyegala divyegala left a comment

Choose a reason for hiding this comment

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

I have, on a first pass, gone through all the changes addressed by this PR and they look great. Only have comments on the new get_thrust_policy method. Thanks for going through these so quickly, Victor!

cpp/include/raft/handle.hpp Outdated Show resolved Hide resolved
cpp/include/raft/handle.hpp Outdated Show resolved Hide resolved
cpp/include/raft/handle.hpp Outdated Show resolved Hide resolved
cpp/include/raft/handle.hpp Outdated Show resolved Hide resolved
cpp/include/raft/handle.hpp Outdated Show resolved Hide resolved
@viclafargue viclafargue force-pushed the miscellaneous-raft-updates branch from ac79888 to e571293 Compare July 8, 2021 17:00
@viclafargue viclafargue mentioned this pull request Jul 13, 2021
Merged
@divyegala divyegala linked an issue Jul 16, 2021 that may be closed by this pull request
[BUG] Replace thrust/rmm::device_vector in MST with rmm::device_uvector #184
Closed
@viclafargue viclafargue added breaking Breaking change improvement Improvement / enhancement to an existing function labels Jul 19, 2021
@viclafargue viclafargue changed the base branch from branch-21.08 to branch-21.10 July 19, 2021 16:30
@viclafargue viclafargue mentioned this pull request Jul 21, 2021
Merged
@github-actions github-actions bot added the CMake label Aug 4, 2021
seunghwak
seunghwak reviewed Aug 16, 2021
View reviewed changes
cpp/include/raft/cudart_utils.h Outdated Show resolved Hide resolved
cpp/include/raft/cudart_utils.h Outdated Show resolved Hide resolved
cpp/include/raft/cudart_utils.h Outdated Show resolved Hide resolved
cpp/include/raft/cudart_utils.h Outdated Show resolved Hide resolved
cpp/include/raft/cudart_utils.h Outdated Show resolved Hide resolved
@viclafargue
Copy link
Contributor Author

rerun tests

@dantegd
Copy link
Member

dantegd commented Aug 24, 2021

rerun tests

1 similar comment
@viclafargue
Copy link
Contributor Author

rerun tests

@dantegd dantegd dismissed divyegala’s stale review August 27, 2021 15:26

stale review, comments have been addressed, need to merge at the soonest

@dantegd
Copy link
Member

dantegd commented Aug 27, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 1c4e1e6 into rapidsai:branch-21.10 Aug 27, 2021
This was referenced Aug 27, 2021
Closed
Closed
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this pull request Aug 27, 2021
@viclafargue
Apply modifications to account for RAFT changes (#1707)
65ca876 
This PR apply modifications to the cuGraph codebase to account for changes in RAFT and RMM :
- rapidsai/raft#283
- rapidsai/raft#285
- rapidsai/raft#286
- rapidsai/rmm#816

This PR requires some changes in the cuHornet dependency : rapidsai/cuhornet#52

Authors:
  - Victor Lafargue (https://github.com/viclafargue)

Approvers:
  - Brad Rees (https://github.com/BradReesWork)
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Seunghwa Kang (https://github.com/seunghwak)
  - Chuck Hastings (https://github.com/ChuckHastings)

URL: #1707
rapids-bot bot pushed a commit to rapidsai/cuml that referenced this pull request Aug 30, 2021
@viclafargue
Apply modifications to account for RAFT changes (#4077)
903d576 
This PR apply modifications to the cuML codebase to account for changes in RAFT and RMM :
- rapidsai/raft#283
- rapidsai/raft#285
- rapidsai/raft#286
- rapidsai/rmm#816

Authors:
  - Victor Lafargue (https://github.com/viclafargue)
  - Dante Gama Dessavre (https://github.com/dantegd)

Approvers:
  - William Hicks (https://github.com/wphicks)
  - Micka (https://github.com/lowener)
  - Dante Gama Dessavre (https://github.com/dantegd)
  - Divye Gala (https://github.com/divyegala)

URL: #4077
@dantegd dantegd mentioned this pull request Sep 7, 2021
Closed
@viclafargue viclafargue mentioned this pull request Nov 24, 2021
Closed
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@viclafargue
Apply modifications to account for RAFT changes (rapidsai#4077)
269b7b1 
This PR apply modifications to the cuML codebase to account for changes in RAFT and RMM :
- rapidsai/raft#283
- rapidsai/raft#285
- rapidsai/raft#286
- rapidsai/rmm#816

Authors:
  - Victor Lafargue (https://github.com/viclafargue)
  - Dante Gama Dessavre (https://github.com/dantegd)

Approvers:
  - William Hicks (https://github.com/wphicks)
  - Micka (https://github.com/lowener)
  - Dante Gama Dessavre (https://github.com/dantegd)
  - Divye Gala (https://github.com/divyegala)

URL: rapidsai#4077
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seunghwak seunghwak seunghwak left review comments

@dantegd dantegd dantegd approved these changes

@divyegala divyegala divyegala left review comments

Assignees
No one assigned
Labels
breaking Breaking change CMake cpp improvement Improvement / enhancement to an existing function python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Replace thrust/rmm::device_vector in MST with rmm::device_uvector
4 participants
@viclafargue @divyegala @dantegd @seunghwak