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

Use raft::host_span instead of const reference to std::vector #4931

Open
wants to merge 6 commits into
base: branch-25.04
Choose a base branch
from

Conversation

seunghwak
Copy link
Contributor

@seunghwak seunghwak commented Feb 10, 2025

This PR updates functions to consistently take raft::host_span instead of std::vector const& (we have been mixing the two); except for public functions in graph_functions.hpp.

Marked as breaking as this PR updates functions under include/cugraph/utilities/device_comm.hpp,shuffle_comm.cuh, but we don't expect public users to directly call these utility functions.

@seunghwak seunghwak added improvement Improvement / enhancement to an existing function breaking Breaking change labels Feb 10, 2025
@seunghwak seunghwak added this to the 25.04 milestone Feb 10, 2025
@seunghwak seunghwak self-assigned this Feb 10, 2025
@seunghwak seunghwak requested a review from a team as a code owner February 10, 2025 17:48
@ChuckHastings
Copy link
Collaborator

/merge

@miscco
Copy link
Contributor

miscco commented Feb 14, 2025

Question: why dont we use cuda::std::span it is available in all supported standard modes

@seunghwak
Copy link
Contributor Author

Question: why dont we use cuda::std::span it is available in all supported standard modes

Oh, is it available now? I was thinking about using raft::host_span/device_span first and switch to cuda::std::span (and std::span which is from C++20) once they become available.

I will replace raft span with cuda::std::span in a separate PR. How do we distinguish host_span and device_span in cuda::std::span?

@miscco
Copy link
Contributor

miscco commented Feb 14, 2025

I will replace raft span with cuda::std::span in a separate PR. How do we distinguish host_span and device_span in cuda::std::span?

There is currently no way, but AFAIK raft also only has type aliases which boil down to the same type

@seunghwak
Copy link
Contributor Author

I will replace raft span with cuda::std::span in a separate PR. How do we distinguish host_span and device_span in cuda::std::span?

There is currently no way, but AFAIK raft also only has type aliases which boil down to the same type

https://github.com/rapidsai/raft/blob/branch-25.04/cpp/include/raft/core/span.hpp#L50

In raft, bool is_device is a non-type template parameter, and host_span and device_span are different types. I feel like we need to distinguish host span and device span (unless GPUs can directly access host_span data; like GH200 or GB200).

I think we will stay little longer with raft::host_span/device_span till we get a solution for this (maybe cuda::std::span and std::span).

@ChuckHastings
Copy link
Collaborator

/merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change cuGraph improvement Improvement / enhancement to an existing function
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants