BUG: Set writeable flag for writeable dlpacks. #28600
Conversation
There was a problem hiding this comment.
Thanks, this looks right to me, sorry about that!
I am actually not sure we want to change bahvior for dlpack <v1 (i.e. unversioned path). Could you check if older NumPy versions returned read-only arrays (I think this did)? I think the old path should maybe set readonly=1/True just to keep behavior (by now the new path should be taken anyway).
I suspect it's not tested, as you would have to do add a helper like:
class myarr:
def __dlpack__(self):
return np.array([1]).__dlpack__(max_version=None)
to avoid getting the new (versioned) protocol.
(The error comes from writeable being default for new allocations, but not for wrapped ones data!=NULL, I suspect it was always slightly off, but before the new path it didn't matter.)
|
Thanks for your rapid comment! This is likely the first time from_dlpack has returned something writeable, but I did test this as you recommend.
It doesn't look like these flags have been changed since they were written in 3163d57#diff-b6927246f0fd28501d169c22cbbdd27e319b8c4659f23bb6b9423f336c2818ebR4349 . fb60521 added the readonly state but only cleared the writeable flag in response to it (it was already unset). I've changed the readonly state to always be readonly for older unversioned dlpacks, respecting the old behavior. I'll squash these changes. |
5156275 to
a36442b
Compare
|
Perfect, thanks! I think the old behavior may have unset it twice effectively and I didn't notice when adding the new one. |
Explicitly set the writeable flag in from_dlpack as the inverse of the dlpack read_only flag. Previously it was not actually being set. Additionally, update the readonly logic such that legacy unversioned DLPacks are never writeable, for compatibility with old behavior. Fixes numpy#28599
a36442b to
155f32b
Compare
|
Thanks for your comments. I removed the release note, could also add it back shorter. |
|
Thanks @karl3wm . |
Explicitly set the writeable flag in from_dlpack as the inverse of the dlpack readable flag. Previously it was not actually being set.
Fixes #28599