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

[BUG]: ndarray<> fails on read-only buffer object #206

Closed
zingdle opened this issue May 8, 2023 · 6 comments
Closed

[BUG]: ndarray<> fails on read-only buffer object #206

zingdle opened this issue May 8, 2023 · 6 comments

Comments

@zingdle
Copy link

zingdle commented May 8, 2023

Problem description

when accessing numpy array with ndarray<>, nanobind fails when binding on a read-only object.

seems that

if (PyObject_GetBuffer(o, view.get(), PyBUF_RECORDS)) {

PyBUF_RECORDS requires write permission. errors ValueError: buffer source array is read-only

Reproducible example code

#include <nanobind/nanobind.h>
#include <nanobind/ndarray.h>

namespace nb = nanobind;

NB_MODULE(myext, m) {
  m.def("foo", [](nb::object o) {
    auto a = nb::cast<nb::ndarray<>>(o);
    printf("%ld\n", a.ndim());
  });
}
>>> import myext
>>> import numpy as np
>>> a = np.broadcast_to(1, (2, 3))
>>> b = np.arange(16)
>>> myext.foo(a)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: nanobind::cast_error
>>> myext.foo(b)
1
@wjakob
Copy link
Owner

wjakob commented May 9, 2023

nanobind currently doesn't have the concept of a read-only tensor and so refusing this unsafe cast seems reasonable. I might consider adding a special nb::ndarray<nb::ro, ..> annotation (perhaps with alias nb::ndarray_ro<..> ) that is able to accept such arrays. Would that fix your problem?

I think that pybind11 simply did not perform checks on whether an array is read only (and shouldn't be written to) requiring the user to do the right thing, which is of course also a solution though arguably not the best one.

@eirrgang
Copy link

eirrgang commented May 9, 2023

I might consider adding a special nb::ndarray<nb::ro, ..> annotation

At least one other user would definitely appreciate that! :-)

edit: I could probably offer a PR, if you like. If there is more discussion about such a feature, I could probably contribute after May 15.

@zingdle
Copy link
Author

zingdle commented May 10, 2023

Yes, a special annotation fixed my problem and make semantics clear. Looking forward to this feature!

@wjakob
Copy link
Owner

wjakob commented May 25, 2023

@eirrgang @zingdle can you take a look at PR #217 and let me know if this addresses your needs?

@eirrgang
Copy link

@eirrgang @zingdle can you take a look at PR #217 and let me know if this addresses your needs?

Ah, okay. The C++ accessors on the ndarray object will respect the const-ness, and the dltensor member is private anyway. The dlpack interface necessarily loses the const-ness, but there's no way around that until dlpack grows "read-only" semantics.

I'm not seeing any modification to the o.__dlpack__ support. Shouldn't it be disabled or force a copy when re-sharing a buffer that was provided read-only, or from a const object? (How does numpy handle that?) Sorry, I'm less familiar with the dlpack use cases. If this is within the scope of the comment about dlpack ignoring ro, maybe there should be a stronger admonition or warning.

For numpy and buffer protocol users, though, it looks great to me! Thanks!

@wjakob
Copy link
Owner

wjakob commented May 30, 2023

The dlpack interface necessarily loses the const-ness, but there's no way around that until dlpack grows "read-only" semantics.

DLPack supports a read-only flag as of dmlc/dlpack#113. But the details of the Python interface aren't worked out, and in any case nobody is using this new versioned flavor of DLPack yet. I plan to update nanobind once there are some implementations of the interface to test against..

I'm not seeing any modification to the o.dlpack support. Shouldn't it be disabled or force a copy when re-sharing a buffer that was provided read-only, or from a const object? (How does numpy handle that?) Sorry, I'm less familiar with the dlpack use cases. If this is within the scope of the comment about dlpack ignoring ro, maybe there should be a stronger admonition or warning.

nanobind generally ignores constness in function parameters and return values. Ignoring the read-only flag in the DLPack interface seems to me in line with this overall behavior. PR #217 implements a limited form of const tracking for nd-arrays because we need this flag to be able to fully interface with NumPy (which is far more strict).

@wjakob wjakob closed this as completed May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants