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

Using raft::resources across raft::random #1420

Merged

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Apr 14, 2023

Eventually we need to do this across all the headers in the codebase so that users have a choice as to whether they want to use raft::device_resources (which implicitly depends on the cuda math libs and thrust) or whether they just want to use raft::resources (which is agnostic of the resources it contains and allows the primitives themselves to levvy the dependency requirements).

cc @MatthiasKohl this should allow cugraph-ops to completely remove the math libs dependency (though the conda recipes will also need to be changed to depend on libraft-headers-only and the cmake changed to turn off the CTK math libs dependency).

NOTE: Before this PR is merged, it's important that it be tested w/ cugraph/cuml at the very least to spot any cases where the device_resources.hpp include was being assumed transitively from the RAFT functions.

…sources` instead of `raft::device_resources`
@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 14, 2023
@cjnolet cjnolet self-assigned this Apr 14, 2023
@cjnolet cjnolet requested a review from a team as a code owner April 14, 2023 23:25
@github-actions github-actions bot added the cpp label Apr 14, 2023
@MatthiasKohl
Copy link
Contributor

Thanks @cjnolet I'll try this out soon in cugraph-ops.
Do you have a small guide on how to move from the actual implementations (in detail) to these higher-level APIs with the handle and the views?
Also, will the pointer-based APIs be deprecated (they aren't yet in rng.cuh)?

Copy link
Member

@benfred benfred left a comment

Choose a reason for hiding this comment

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

lgtm!

@cjnolet
Copy link
Member Author

cjnolet commented Apr 17, 2023

Also, will the pointer-based APIs be deprecated (they aren't yet in rng.cuh)?

I'll make sure I give a good heads up when these are finally removed. It's hard to explicitly deprecate everything individually but the general state of RAFT is that we're shifting the public APIs to the mdspan-based variants and raft::resources. It shouldn't be too complicated to shift over to the new APIs- mdspan is just a lightweight wrapper around a pointer w/ some shape information.

I might submit a PR to cugraph-ops to attempt this change.

@cjnolet cjnolet added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Apr 17, 2023
@cjnolet cjnolet removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label May 4, 2023
@cjnolet
Copy link
Member Author

cjnolet commented May 5, 2023

/merge

@rapids-bot rapids-bot bot merged commit aa9d686 into rapidsai:branch-23.06 May 5, 2023
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants