search

coreylowman / cudarc

Safe rust wrapper around CUDA toolkit
Apache License 2.0
626 stars 77 forks source link

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

Open dkales opened 2 months ago

dkales commented 2 months ago

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?

coreylowman commented 2 months ago

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>."

coreylowman commented 2 months ago

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.

dkales commented 2 months ago

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