wjakob / nanobind

nanobind: tiny and efficient C++/Python bindings
BSD 3-Clause "New" or "Revised" License
2.33k stars 195 forks source link

[BUG]: Calling `from_dlpack` with a DLPack tensor is deprecated. #729

Closed hpkfft closed 1 month ago

hpkfft commented 1 month ago

Problem description

An extension that returns an ndarray<nb::jax> will give the user the following DeprecationWarning:

Calling from_dlpack with a DLPack tensor is deprecated. The argument to from_dlpack should be an array from another framework that implements the __dlpack__ protocol.

This is seen using test_ndarray.py::test19_return_jax in PR #728

Reproducible example code

python -m pip install -U "jax[cpu]"
pytest tests/test_ndarray.py
wjakob commented 1 month ago

Thank you for the report, this is now fixed on master.

hpkfft commented 1 month ago

Thanks!

When a framework is not specified, would it be a good idea to implement the __dlpack__ protocol as you did for JAX? For example, looking at the ndarray test test15_consume_numpy, the python code does the following:

    class wrapper:
        def __init__(self, value):
            self.value = value
        def __dlpack__(self):
            return self.value
    a = t.return_dlpack()
    x = np.from_dlpack(wrapper(a))

Would it be good if a already had the two member functions __dlpack__ and __dlpack_device__ so the wrapper above was not needed?

wjakob commented 1 month ago

That would be a break in documented behavior, so I don't think this could be changed in a minor version bump.

wjakob commented 1 month ago

@hpkfft: FYI, your project's website does not work from Japan: https://www.dropbox.com/scl/fi/7wugf992co256963ltf5e/Screenshot-2024-09-21-at-14.44.19.png?rlkey=ji9zuy859okffac63wl8nv3k5&dl=0

wjakob commented 1 month ago

@hpkfft: see commit 8dc834c5fdaf412251732e7dde21b305ce5756be for a pattern that is probably more broadly useful: directly implementing these methods on the underlying array types.

hpkfft commented 1 month ago

Thank you for letting me know about my website problem! It should be fixed now.

Yes, I've already realized I could implement these two methods in my extension. (Though, I did not think to provide kwargs; I will look into this and see if it might be useful to me.) If you're curious, see the example I have at the bottom of this section: https://hpkfft.com/python.html#data-types

It was nice of you to add this advice to the nanobind documentation, both for me and for others.