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

[ENH] Support more input data layouts in cudf.from_dlpack #10849

Open
wence- opened this issue May 13, 2022 · 3 comments
Open

[ENH] Support more input data layouts in cudf.from_dlpack #10849

wence- opened this issue May 13, 2022 · 3 comments
Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code.

Comments

@wence-
Copy link
Contributor

wence- commented May 13, 2022

Related to #10754, the current implementation of from_dlpack requires unit-stride fortran order, and produces appropriate error messages in the unsupported cases

Consider

import cudf
import cupy
a = cupy.arange(10)
b = a[::2]
c = cudf.from_dlpack(b.__dlpack__())
=> RuntimeError: from_dlpack of 1D DLTensor only for unit-stride data
b = cupy.broadcast_to(a[1], (10,)) # b is stride-0
=> RuntimeError: from_dlpack of 1D DLTensor only for unit-stride data

a = cupy.arange(12).reshape(3, 4).copy(order="F")
b = a[::2, :]
c = cudf.from_dlpack(b.__dlpack__())
=> RuntimeError: from_dlpack of 2D DLTensor only for column-major unit-stride data

Since from_dlpack copies in all cases right now, I think that things can be handled like so:

  1. Non-fortran-order: useful error
  2. unit-stride: current cudaMemcpyAsync one column at a time
  3. fastest-dimension is stride-0 (broadcasted arrays): std::fill for the 1D case, just getting the strides right for the 2D case
  4. fastest-dimension is stride-N (sliced arrays): cudaMemcpy2DAsync with appropriate choices of pitch and stride for the source array

However, I'm not really sure of the performance implications of these choices, and if the current approach of producing an error and requiring that the caller copy to contiguous fortran-order first before calling from_dlpack is not better. For example, for case 4 is it faster to copy to a contiguous buffer first rather than copying column by column?

@wence- wence- added bug Something isn't working Needs Triage Need team to review and classify libcudf Affects libcudf (C++/CUDA) code. labels May 13, 2022
@wence- wence- changed the title [BUG] cudf.from_dlpack memory errors for non-unit-stride data [BUG] cudf.from_dlpack incorrect results for non-unit-stride data May 13, 2022
@wence-
Copy link
Contributor Author

wence- commented May 17, 2022

The silently bad behaviour was turned into actual logic_errors in #10850, it would still be possible to support more fortran-order or broadcasted inputs if desired.

@github-actions
Copy link

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

@GregoryKimball GregoryKimball added feature request New feature or request and removed Needs Triage Need team to review and classify bug Something isn't working labels Jun 28, 2022
@wence- wence- changed the title [BUG] cudf.from_dlpack incorrect results for non-unit-stride data [ENH] Support more input data layouts in cudf.from_dlpack Jul 8, 2022
@github-actions
Copy link

github-actions bot commented Oct 6, 2022

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.

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 libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

No branches or pull requests

3 participants