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

[WIP] Remove RAFT memory management #400

Merged

Conversation

viclafargue
Copy link
Contributor

Answers #308.
Requires the appropriate changes in cuML and cuGraph before merging.

@viclafargue viclafargue requested review from a team as code owners November 24, 2021 10:12
@cjnolet cjnolet added the 2 - In Progress Currenty a work in progress label Dec 6, 2021
rapids-bot bot pushed a commit to rapidsai/cuml that referenced this pull request Jan 18, 2022
@cjnolet
Copy link
Member

cjnolet commented Jan 25, 2022

@viclafargue, it looks like there's just a couple includes for allocator remaining in the code and this is ready to go. Do we want to get this into 22.02?

@cjnolet cjnolet changed the base branch from branch-22.02 to branch-22.04 January 25, 2022 16:24
@viclafargue
Copy link
Contributor Author

Postponing to 22.04, as discussed offline.

@github-actions github-actions bot added the CMake label Jan 25, 2022
@viclafargue viclafargue requested review from a team as code owners February 14, 2022 13:35
#include <raft/mr/buffer_base.hpp>
#include <raft/mr/device/buffer.hpp>

namespace raft {
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is going to require some updates to cugraph as well.

cpp/test/spatial/fused_l2_knn.cu Outdated Show resolved Hide resolved
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this pull request Feb 15, 2022
@viclafargue viclafargue added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change breaking Breaking change and removed non-breaking Non-breaking change labels Feb 24, 2022
@cjnolet
Copy link
Member

cjnolet commented Feb 25, 2022

rerun tests

@viclafargue viclafargue force-pushed the remove-raft-memory-management branch from cf19910 to 0243630 Compare March 15, 2022 18:36
/** Default cudaMallocHost/cudaFreeHost based host allocator */
class default_allocator : public allocator {
public:
void* allocate(std::size_t n, cudaStream_t stream) override
Copy link
Member

Choose a reason for hiding this comment

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

I had tried to remove this at one point and realized cugraph is still using it. We should also open a PR in cugraph to move the use of host allocator/buffer to something equivalent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just searched cugraph/cpp and could not find code calling this. We need to double check but we may have removed the code using this function.

Copy link
Member

Choose a reason for hiding this comment

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

@seunghwak I think you are right, I'm no longer seeing it in cugraph either.

@cjnolet
Copy link
Member

cjnolet commented Mar 18, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 613c722 into rapidsai:branch-22.04 Mar 18, 2022
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In Progress Currenty a work in progress breaking Breaking change CMake cpp improvement Improvement / enhancement to an existing function python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants