search

rapidsai / cuml

cuML - RAPIDS Machine Learning Library
https://docs.rapids.ai/api/cuml/stable/
Apache License 2.0
4.18k stars 527 forks source link

[BUG] Use `rmm::exec_policy` instead of `thrust::cuda::par` #3976

Open trxcllnt opened 3 years ago

trxcllnt commented 3 years ago

We should use the new rmm::exec_policy instead of the default thrust::cuda::par policy, because the RMM execution policy uses the current device_memory_resource for temporary Thrust and cub allocations. In the process, we should update APIs that currently use raw cudaStream_t to use rmm::cuda_stream_view.

divyegala commented 3 years ago

@trxcllnt could you tell why rmm::cuda_stream_view was needed instead of raw cudaStream_t?

trxcllnt commented 3 years ago

@divyegala because rmm::exec_policy(stream)->on(stream) is both more verbose and deprecated :sweat_smile:

divyegala commented 3 years ago

@trxcllnt yep, I have been consistently using the new API. I'm actually asking why is there a new wrapper around cuda streams?

trxcllnt commented 3 years ago

IIUC rmm::cuda_stream is an owning type, whereas the ownership of an arbitrary cudaStream_t isn't clear. I'm sure @jrhemstad would have more to say on the topic.

github-actions[bot] commented 2 years ago

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

jrhemstad commented 2 years ago

IIUC rmm::cuda_stream is an owning type, whereas the ownership of an arbitrary cudaStream_t isn't clear. I'm sure @jrhemstad would have more to say on the topic.

Thanks stale-bot for reminding me about this.

cuda_stream is an owning type. cuda_stream_view is non-owning, but is strongly typed such that it cannot be confused with an int and disallows construction from nullptr.

Without cuda_stream_view, these two overloads would be ambiguous:

foo(cudaStream_t)
foo(int)
github-actions[bot] commented 2 years ago

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.