Repository navigation
MAINT,TST: np._from_dlpack: add more test + small memory optimization - #21121
Conversation
| y = np._from_dlpack(capsule, _testing=True) | ||
| with pytest.raises(ValueError, match=r"PyCapsule_GetPointer called " | ||
| r"with incorrect name"): | ||
| np._from_dlpack(capsule, _testing=True) |
There was a problem hiding this comment.
I don't think I like a "private" kwarg here, and I don't quite understand why it is necessary. It seems to do nothing except an INCREF and that incref seems odd?!
The allocation change looks nice on first sight though!
There was a problem hiding this comment.
I don't think I like a "private" kwarg here, and I don't quite understand why it is necessary.
To check that consumed capsules raise and that the array_dlpack_deleter skips deleting a consumed capsule. But it doesn't have to be tested explicitly so I can revert that part. I added it to address "testing calling _from_dlpack twice" semantics.
There was a problem hiding this comment.
You can keep the assert if you like, its gone anyway +in release mode (but I am not sure it is useful).
When it comes to checking that we clean up correctly, valgrind or pytest-leaks (sys.gettotalrefcount with a debug python) will find that for us. If that is what you mean?
Getting the line/code paths coverage up is always good, fortunatley, while cumbersome, checking that the code paths clean up correctly is mostly reliable.
There was a problem hiding this comment.
I have reverted. I am not yet aware of how NumPy uses valgrind and pytest-leaks in its tests/CI so I will add the tests back later if needed. Now the PR is stripped down to testing with ndims=0 and memory optimization. Let me know if that looks good!
There was a problem hiding this comment.
I have reverted. I am not yet aware of how NumPy uses valgrind and pytest-leaks
It doesn't by default, there is a bit of docs on it on the dev guide, but I am not expecting you to do it. It is more of a: don't worry about it too much, because we would probably catch it eventually.
I see you removed the test again? But the test seems right, the capsule will be renamed so an error should be given, how come you can't keep the test?
There was a problem hiding this comment.
But the test seems right, the capsule will be renamed so an error should be given, how come you can't keep the test?
_from_dlpack doesn't accept capsules; even the one named "dltensor". (it needs an object that has __dlpack__ attribute). So, I had to remove the test (I added the _testing kwargs to enable passing capsule so I could write the test.).
There was a problem hiding this comment.
Well, you could create a dummy class with a __dlpack__ dunder to do it. But, we can just focus on the other change in this PR, and then create another (or not, not like it is super vital).
There was a problem hiding this comment.
you could create a dummy class with a
__dlpack__dunder to do it
Good idea! Thanks!
But, we can just focus on the other change in this PR, and then create another
OK, sounds good!
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
One nit, otherwise I think it is ready. Thanks. (I see that one line is indeed not covered by the test right now, so the followup would be cool.)
Co-authored-by: Sebastian Berg <[email protected]>
|
Thanks for these cleanups, Tirth! |
…numpy#21121) * MAINT,TST: np._from_dlpack: add more test + small memory optimization * MAINT: np._from_dlpack: avoid arithmetic on void * * MAINT: np._from_dlpack: use unsigned char for _testing * MAINT: remove an unnecessary NULL check * MAINT: np._from_dlack: revert passing private kwarg * MAINT: np._from_dlpack: don't add padding at the end Co-authored-by: Sebastian Berg <[email protected]> Co-authored-by: Sebastian Berg <[email protected]>
fixes gh-20339
Adds tests for:
ndim=0arrays_from_dlpackcalled twice with the same capsule (i.e. renaming mechanism)_from_dlpackis called twice.Also, merged the allocation of DLManagedTensor, shape, and strides. To test if the renaming and deallocating mechanism works as expected, I have added a keyword-only private argument
_testingthat allows capsules to be passed to_from_dlpack. I don't know how often NumPy does this so let me know if it's fine to keep it or should I revert that change.cc @seberg @mattip