[FEA] Rng: expose host-rng-state in host-only API #609
Conversation
MatthiasKohl
added
3 - Ready for Review
feature request
MatthiasKohl
added
2 - In Progress
MatthiasKohl
added
3 - Ready for Review
and removed
2 - In Progress
|
This PR fixes #406 |
…deprecated inclusion of rng_device.cuh through rng.cuh
|
@cjnolet I believe I addressed your comments. I haven't fully tested all the possible deprecation messages, which are much more tricky since we want to keep the |
| DI void custom_next( | ||
| GenType& gen, OutType* val, SamplingParams<OutType, LenType> params, LenType idx, LenType stride) | ||
| template <typename DataT, typename WeightsT, typename IdxT = int> | ||
| void sampleWithoutReplacement(RngState& rng_state, |
There was a problem hiding this comment.
One very small nitpick I have here is that the handle should always be the first argument for primitives that accept one.
There was a problem hiding this comment.
I removed the handle here since it wasn't being used in the implementation at all (the stream is already given as another parameter).
This also keeps the API in line with other RNG APIs
There was a problem hiding this comment.
I found a few more trivial things in my second pass that I missed in the first pass but overall I think it looks great. Have you tried building cuml/cugraph with these changes yet just to make sure they build successfully? I don't know that we need to open full PRs to test this anymore unless there are direct changes to the public API, but it would be a good idea to check whether things like "warnings as errors" flags cause builds to fail.
|
@cjnolet I haven't tried the cuml/cugraph builds yet but I can do that once I've addressed your latest comments |
|
@cjnolet I've addressed the issues you mentioned, and I've been able to build cuml. I'm currently building cugraph as well, and the files requiring RNG seem to compile fine at least. |
|
@gpucibot merge |
After merging PR #609 , I realized that there were still a few bugs (all minor): 1. The deprecated `Rng` class had the `type` attribute removed. I added it back. This was used in cugraph-ops but not in other projects (cugraph, cuml). 2. The template function to call another function with the device state was actually unusable since you wouldn't be able to instantiate a function to call it with. So I removed that. 3. The macro used to call functions with a device state wasn't using fully namespace-qualified names, which made it unusable outside of RAFT. 4. Removed the deprecation warning of the new `rng_device.cuh` since it has to be included anyway through `rng.cuh`, so those deprecations didn't make sense. Authors: - Matt Joux (https://github.com/MatthiasKohl) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #630
Changes:
RngStateandGeneratorTypeto own header so they can be used in public APIs without a CUDA-compatible compiler (useful for python bindings etc.).advancemethod to the state object as well, allowing libraries to useRngStatefor host-only coderng.hppheader: if we want to expose theRngobject itself as a host-only object, the implementation/definition would have to be separated from the declaration, and this means it would be hard to keep a header-only library for these objects. Otherwise, there is simply no way that thisrng.hppactually fulfills its contract of a host-only header, so we remove it.This is not the only way of creating a host-only API, but it's most likely the one which needs the least amount of changes in dependent libraries, and these libraries can basically "opt-in" to the host-only API.
@cjnolet Do you agree with this approach? I'd like to get this PR into 22.06 fairly soon, so we can finally go back to sane APIs and compile commands in cugraph-ops and cugraph.