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

[REVIEW] Develop new mdspan-ified multi_variable_gaussian interface #845

Merged

Conversation

mhoemmen
Copy link
Contributor

@mhoemmen mhoemmen commented Sep 23, 2022

Develop a new multi_variable_gaussian interface that uses mdspan. The new interface uses free functions, rather than a class.

TODO:

@mhoemmen mhoemmen requested a review from a team as a code owner September 23, 2022 19:33
@github-actions github-actions bot added the cpp label Sep 23, 2022
@mhoemmen mhoemmen added feature request New feature or request non-breaking Non-breaking change labels Sep 23, 2022
@cjnolet
Copy link
Member

cjnolet commented Sep 23, 2022

Do not expose workspaces to the public API (see e.g., Implement stats API with mdspan #802 (comment)); use RMM instead.

If you have time to support it, adding an optional rmm::mr::device_memory_resource (or even better, add our own interface that can wrap rmm's device and host memory resources) to the handle could be super helpful to get the workspace out of the public API. We can also use it on the ivf flat search API.

@mhoemmen
Copy link
Contributor Author

@cjnolet wrote:

If you have time to support it, adding an optional rmm::mr::device_memory_resource (or even better, add our own interface that can wrap rmm's device and host memory resources) to the handle could be super helpful to get the workspace out of the public API. We can also use it on the ivf flat search API.

I'll be happy to add an optional rmm::mr::device_memory_resource parameter. For adding it to the handle instead, do you have a sense for what that interface should look like? I was thinking of a free function like this.

device_memory_resource get_default_memory_resource(const handle_t&);

The const reference is necessary because of all the interfaces that take const handle&, but storing the memory resource in the handle would call for the same mutable pattern as before.

@mhoemmen mhoemmen force-pushed the mdspanify-multi_variable_gaussian-1 branch 2 times, most recently from 3a74368 to 8711316 Compare September 27, 2022 17:05
@cjnolet
Copy link
Member

cjnolet commented Sep 27, 2022

The const reference is necessary because of all the interfaces that take const handle&, but storing the memory resource in the handle would call for the same mutable pattern as before.

I think that could work. To make this compatible w/ some of the ideas we've been tossing around for the more library agnostic raft handle, what do you think about sticking this in a file called rmm_resource_handler.hpp? Just thinking out loud here, but it would be nice if the handle ultimately just became a key/value store and primitives could request what they need from the handle. Eventually, we could have includes like cublas_resource_handler.hpp, which would lazy-load a cublas handle and store it on the raft handle when it's needed. Of course, this disregards the immutability of the const qualifier and might require some care (locks?) to avoid conflicts in concurrent updates. The handle is coupled to a single "user" stream, but what if these resources are updated in a worker stream?

The basic idea here would be that if no primitives are invoked that need cublas, there's no reason for the user to ever have to interact with it, and thus no reason to even bother creating a cublas handle at all.

@mhoemmen mhoemmen force-pushed the mdspanify-multi_variable_gaussian-1 branch 2 times, most recently from fb949f9 to 206f889 Compare September 28, 2022 18:15
@mhoemmen mhoemmen changed the title [WIP] Develop new mdspan-ified multi_variable_gaussian interface [REVIEW] Develop new mdspan-ified multi_variable_gaussian interface Sep 28, 2022
@mhoemmen mhoemmen changed the title [REVIEW] Develop new mdspan-ified multi_variable_gaussian interface [WIP] Develop new mdspan-ified multi_variable_gaussian interface Sep 28, 2022
@mhoemmen mhoemmen force-pushed the mdspanify-multi_variable_gaussian-1 branch from 206f889 to d866f64 Compare September 28, 2022 19:46
@mhoemmen mhoemmen changed the title [WIP] Develop new mdspan-ified multi_variable_gaussian interface [REVIEW] Develop new mdspan-ified multi_variable_gaussian interface Sep 28, 2022
@mhoemmen
Copy link
Contributor Author

@cjnolet wrote:

Just thinking out loud here, but it would be nice if the handle ultimately just became a key/value store and primitives could request what they need from the handle.

I've filed this as a feature request: #856

Thanks! : - )

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Thanks for these wrappers, Mark! This looks great overall. Just minor things and I think it's ready to go.

cpp/include/raft/random/detail/multi_variable_gaussian.cuh Outdated Show resolved Hide resolved
cpp/include/raft/random/multi_variable_gaussian.cuh Outdated Show resolved Hide resolved
cpp/include/raft/random/multi_variable_gaussian.cuh Outdated Show resolved Hide resolved
cpp/include/raft/random/multi_variable_gaussian.cuh Outdated Show resolved Hide resolved
It's not ready for public consumption yet,
as it still lives in the detail namespace.
However, it builds and passes tests.
New mdspan-based compute_multi_variable_gaussian interface
is a one-pass interface.  It uses device_memory_manager
so that the user no longer needs to allocate workspace by hand.
Move multi_variable_gaussian_decomposition_method to a new
random_types.hpp header file.
@mhoemmen mhoemmen force-pushed the mdspanify-multi_variable_gaussian-1 branch from d866f64 to b28b93a Compare October 5, 2022 15:55
* Rename setup_multi_variable_gaussian to
  build_multi_variable_gaussian_token

* Finish documentation of the supported methods
  that compute_multi_variable_gaussian can use.

* Remove the separate setup phase from the public
  multivariable gaussian interface.
  (Taking a memory resource for the workspace makes
  separating setup from computation no longer needed.)
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks mark! The new API looks really clean.

@cjnolet
Copy link
Member

cjnolet commented Oct 5, 2022

@gpucibot merge

1 similar comment
@cjnolet
Copy link
Member

cjnolet commented Oct 5, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b3d5103 into rapidsai:branch-22.10 Oct 5, 2022
@mhoemmen mhoemmen deleted the mdspanify-multi_variable_gaussian-1 branch October 5, 2022 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp feature request New feature or request non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants