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

Add support for from_dlpack with numpy 1.23.0 #284

Merged
merged 3 commits into from Apr 18, 2023

Conversation

oliverholworthy
Copy link
Member

Add support for from_dlpack with numpy 1.23.0

Check np.from_dlpack (added in 1.22.0, renamed from private _from_dlpack), while keeping check of np._from_dlpack (added in 1.22.0) for backwards compatibility with numpy 1.22.0

@oliverholworthy oliverholworthy added the chore Maintenance for the repository label Apr 13, 2023
@oliverholworthy oliverholworthy self-assigned this Apr 13, 2023
@oliverholworthy oliverholworthy marked this pull request as ready for review April 13, 2023 21:15
@@ -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.

@karlhigley karlhigley merged commit 6ab05ce into NVIDIA-Merlin:main Apr 18, 2023
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Maintenance for the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants