cudarc icon indicating copy to clipboard operation
cudarc copied to clipboard

Should `nccl::broadcast` take a `Option<&T>` instead of an `&Option<T>`?

Open dkales opened this issue 1 year ago • 3 comments

In https://github.com/coreylowman/cudarc/blob/886d6d27cd68da4f81ce30a98bdf1940a895f813/src/nccl/safe.rs#L242-L245, the sendbuf is given as an &Option<T>, which involves wrapping the T, which is something like a CudaSlice in an Option to pass it in there, making re-use of the sendbuffer later on less ergonomic, since you need to get it back from the Option. The other way around would make this easier, but IDK if there are some other drawbacks I'm not seeing ATM?

dkales avatar Aug 27 '24 11:08 dkales

Hmm yeah actually I agree with you. At first I thought Option provided a way to do this, but its the reverse - Option::as_ref "Converts from &Option<T> to Option<&T>."

chelsea0x3b avatar Sep 05 '24 16:09 chelsea0x3b

I think using the CudaViews is a fairly okay work around for this (probably why it's usable in the first place). Creating a view is very cheap and you can reuse them as many times as you want. Unless I'm missing something.

chelsea0x3b avatar Sep 05 '24 16:09 chelsea0x3b

I think using the CudaViews is a fairly okay work around for this (probably why it's usable in the first place). Creating a view is very cheap and you can reuse them as many times as you want. Unless I'm missing something.

yeah this is a workaround, but only possible since the last version while this interface was there much longer, just wanted to raise this as a suggestion

dkales avatar Sep 05 '24 19:09 dkales