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

Add support for from_dlpack with numpy 1.23.0 #284

Merged
merged 3 commits into from
Apr 18, 2023
Merged

Add support for from_dlpack with numpy 1.23.0 #284

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
775a354
Add support for dlpack with numpy 1.23.0
oliverholworthy Apr 13, 2023
143d6cf
Merge branch 'main' into numpy-dlpack-public
karlhigley Apr 14, 2023
88fc18d
Merge branch 'main' into numpy-dlpack-public
oliverholworthy Apr 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions merlin/table/numpy_column.py
View file
Open in desktop
Expand Up @@ -97,7 +97,13 @@ def _register_from_dlpack_cpu_to_numpy():
@_from_dlpack_cpu.register(np.ndarray)
def _from_dlpack_cpu_to_numpy(to, array):
try:
# private `_from_dlpack` method added in 1.22.0
return np._from_dlpack(array)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a vague recollection that the semantics of these methods are actually not quite the same. Do we have a way to test against different versions of NumPy that would allow us to validate that both of these methods work as expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

np._from_dlpack(obj) function, where obj supports __dlpack__, and returns an ndarray
NumPy 1.22.0 Release Notes

numpy.from_dlpack has been added to allow easy exchange of data using the DLPack protocol. It accepts Python objects that implement the __dlpack__ and __dlpack_device__ methods and returns a ndarray object which is generally the view of the data of the input object.
NumPy 1.23.0 Release Notes

from the release notes the first private function in 1.22 mentions it supports objects that implement __dlpack__, while the notes of 1.23 mention support for objects with __dlpack__ and __dlpack_device__. There was a bugfix for __dlpack_device__ in numpy/numpy#21138. Which maybe indicates we shouldn't be using this until we have a min version of numpy 1.23 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we have a way to test against different versions of NumPy that would allow us to validate that both of these methods work as expected?

For automated tests we could potentially add a new workflow that has a test matrix for supported numpy versions? Combined with some pytest marks to run a subset of the tests? (the core tests run pretty quickly so that might not be strictly needed here)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah, that's the difference. I don't think we do much with __dlpack_device__ since it matters less on CPU. This is probably fine then.

except AttributeError:
pass
try:
# public `from_dlpack` method added in 1.23.0
return np.from_dlpack(array)
except AttributeError as exc:
raise NotImplementedError(
"NumPy does not implement the DLPack Standard until version 1.22.0, "
Expand Down