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

[FEA] Delete default constructor for device_buffer and device_uvector #422

Closed
jrhemstad opened this issue Jun 23, 2020 · 10 comments
Closed
Labels
feature request New feature or request

Comments

@jrhemstad
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Similar to #418

The default constructor for device_buffer and device_uvector are problematic as they do not allow specifying a stream. This can lead to situations where the user isn't aware an operation is stream ordered:

device_buffer b{};
b.resize(100); // Defaults to using the default CUDA stream

Describe the solution you'd like

Delete the default ctor and force users to explicit specify a zero size and a stream.

@jrhemstad jrhemstad added the feature request New feature or request label Jun 23, 2020
@jakirkham
Copy link
Member

I thought device_buffer used the default stream in this case. Or did that change?

@jrhemstad
Copy link
Contributor Author

I thought device_buffer used the default stream in this case. Or did that change?

It does, and that's precisely the problem. Due to the widespread misuse of the defaulted stream arguments with device_buffer in libcudf and elsewhere, we've decided to make stream arguments explicit and force users to specify a stream to reduce the chance of doing the wrong thing.

@jakirkham
Copy link
Member

I think this will become challenging in Python as we don't have a good way to handle stream objects today. It will also requires other Python libraries that don't really know what streams are in use/available to think about them, which will make things challenging from an ecosystem perspective.

@jrhemstad
Copy link
Contributor Author

I think this will become challenging in Python as we don't have a good way to handle stream objects today. It will also requires other Python libraries that don't really know what streams are in use/available to think about them, which will make things challenging from an ecosystem perspective.

Nothing is stopping the Python side from still plugging in the default stream. The point is to make it explicit and force that behavior rather than defaulting the value.

@jakirkham
Copy link
Member

Is this still an issue if we use PTDS ( #416 )?

@jrhemstad
Copy link
Contributor Author

Is this still an issue if we use PTDS ( #416 )?

It's orthogonal to PTDS. The point is, to use a device_buffer you always need to be explicit about what stream it's being used on, even if it's the CUDA default stream.

@github-actions
Copy link

This issue has been marked rotten due to no recent activity in the past 90d. 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.

@github-actions
Copy link

This issue has been marked stale due to no recent activity in the past 30d. 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. This issue will be marked rotten if there is no activity in the next 60d.

@harrism
Copy link
Member

harrism commented Feb 16, 2021

@jrhemstad still relevant?

@jrhemstad
Copy link
Contributor Author

#424 (comment)

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants