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.

Sign up for GitHub

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

Jump to bottom

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

Open
dkales opened this issue Aug 27, 2024 · 3 comments · May be fixed by #292
Open

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

dkales opened this issue Aug 27, 2024 · 3 comments · May be fixed by #292

Comments

@dkales
Copy link
Contributor

dkales commented Aug 27, 2024 •
edited
Loading

In

cudarc/src/nccl/safe.rs

Lines 242 to 245 in 886d6d2

pub fn broadcast<S: DevicePtr<T>, R: DevicePtrMut<T>, T: NcclType>(
&self,
sendbuff: &Option<S>,
recvbuff: &mut R,
, 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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

dkales commented Sep 5, 2024

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

coreylowman added a commit that referenced this issue Sep 6, 2024
@coreylowman
#290 Fixing Option usage in nccl::Comm::broadcast/reduce
9d5906b
@coreylowman coreylowman linked a pull request Sep 6, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@coreylowman @dkales