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

Construct pylibcudf columns from objects supporting __cuda_array_interface__ #15615

Conversation

brandon-b-miller
Copy link
Contributor

This PR allows zero copy construction of pylibcudf columns from device arrays via the gpumemoryview class. cc @mroeschke

@brandon-b-miller brandon-b-miller added feature request New feature or request Python Affects Python cuDF API. non-breaking Non-breaking change labels Apr 30, 2024
@brandon-b-miller brandon-b-miller self-assigned this Apr 30, 2024
@brandon-b-miller brandon-b-miller requested a review from a team as a code owner April 30, 2024 01:42
python/cudf/cudf/_lib/pylibcudf/column.pyx Show resolved Hide resolved
python/cudf/cudf/_lib/pylibcudf/column.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/pylibcudf/column.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/pylibcudf/column.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/pylibcudf/column.pyx Outdated Show resolved Hide resolved
data = gpumemoryview(obj)
iface = data.__cuda_array_interface__()
data_type = _data_type_from_iface(iface)
size = iface['shape'][0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Someone needs to check that the object being described is one-dimensional and contiguous. So we should probably:

Suggested change
size = iface['shape'][0]
(size,) = iface['shape']

And also assert that strides are appropriate (either None or (1,) I think?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We implemented something similar for buffer here, but it's in the cuDF python namespace. Would it be overkill to move the impl to a cython utility we can consume from both places?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because we need to check here too, otherwise we're potentially producing a nonsense result.

@brandon-b-miller
Copy link
Contributor Author

Some failing tests locally seem to be because our datetime column supports __cuda_array_interface__, but our timedelta column seems not to. Anyone know if this is as intended?

@mroeschke
Copy link
Contributor

Anyone know if this is as intended?

Not sure if it's intended but appears like an oversight. The interface for numeric and datetime columns are exactly the same, so I suppose timedelta should have support since it's a supported type code

rapids-bot bot pushed a commit that referenced this pull request May 1, 2024
…15622)

Column types that support CAI already have custom `NotImplementedError`s, and since the implementation is the same for datetime and numeric columns, moving their implementation to `ColumnBase`

Should help address timedelta support in #15615 cc @brandon-b-miller

Authors:
  - Matthew Roeschke (https://github.com/mroeschke)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

URL: #15622
@mroeschke
Copy link
Contributor

Timedelta columns should work now with #15622 in

@brandon-b-miller
Copy link
Contributor Author

Timedelta columns should work now with #15622 in

Thanks @mroeschke tests pass locally for me now.

Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wence-
Copy link
Contributor

wence- commented May 2, 2024

/merge

@rapids-bot rapids-bot bot merged commit 4494991 into rapidsai:branch-24.06 May 2, 2024
70 checks passed
@vyasr vyasr added the pylibcudf Issues specific to the pylibcudf package label May 28, 2024
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 non-breaking Non-breaking change pylibcudf Issues specific to the pylibcudf package Python Affects Python cuDF API.
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants