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

[FEA] Raft RNG updated API #2260

Merged
merged 6 commits into from
May 11, 2022

Conversation

MatthiasKohl
Copy link
Contributor

This PR updates usage of RAFT RNG functionality throughout the cugraph code-base, including using the new RAFT RngState to pass state between cugraph and cugraph-ops.
Finally, I removed unnecessary includes from raft/random where applicable.

@MatthiasKohl MatthiasKohl requested a review from a team as a code owner May 4, 2022 17:06
@kaatish kaatish added 3 - Ready for Review improvement Improvement / enhancement to an existing function labels May 4, 2022
@kaatish kaatish added this to the 22.06 milestone May 4, 2022
namespace cugraph {

template <typename graph_t>
std::tuple<rmm::device_uvector<typename graph_t::edge_type>,
rmm::device_uvector<typename graph_t::vertex_type>>
sample_neighbors_adjacency_list(raft::handle_t const& handle,
ops::gnn::graph::Rng& rng,
raft::random::RngState& rng_state,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You will have to make the corresponding changes to cpp/include/cugraph/algorithms.hpp file. The functions sample_neighbors* do not have a python binding yet so CI should not break because of this.
@rlratzel thoghts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. I've updated the public API in algorithms.
I don't see any uses in python or binding code, so I think this should be good enough for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

You will have to make the corresponding changes to cpp/include/cugraph/algorithms.hpp file. The functions sample_neighbors* do not have a python binding yet so CI should not break because of this. @rlratzel thoghts?

This should be okay since the python binding for neighborhood sampling will use the C API, which does not expose this function signature to the binding code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will merge this change (once it is merged into the code base) into my refactor branch for uniform neighborhood sampling. I've been talking with Corey about wanting to make this change, so I'm ready.

@kaatish kaatish added the non-breaking Non-breaking change label May 4, 2022
Copy link
Collaborator

@ChuckHastings ChuckHastings left a comment

Choose a reason for hiding this comment

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

Do we have an issue somewhere to track the issue we were seeing with the PC Generator (the default for raft)?

@MatthiasKohl
Copy link
Contributor Author

I'll open an issue

@MatthiasKohl
Copy link
Contributor Author

I've opened issue #2266 for this and the tests here seem to be passing.
@kaatish can we merge this PR?

@ChuckHastings
Copy link
Collaborator

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 403797f into rapidsai:branch-22.06 May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants