Ensure allocation stream is used for buffer deallocation if no explicit stream is provided #1032
Conversation
leofang
added
P0
|
/ok to test 738eed4 |
This comment has been minimized.
This comment has been minimized.
leofang
force-pushed
the
dealloc_stream
branch
from
738eed4 to
605241e
Compare
|
(This PR is now based entirely on #1070 except for the last commit, so that PR needs to be merged first.) |
|
/ok to test 605241e |
kkraus14
left a comment
kkraus14
left a comment
There was a problem hiding this comment.
Is there any way we could add some testing for stream ordered allocations / deallocations?
|
Changes LGTM on top of #1070 |
|
Merged with main! |
|
/ok to test 33345d6 |
| If the buffer is deallocated without an explicit stream, the allocation stream | ||
| is used. | ||
| """ | ||
| if stream is None: |
There was a problem hiding this comment.
Did we intend to remove this? Based on the deallocate API signature a user would deem it valid to pass None as a stream argument. That None is not checked in the _deallocate function above when the stream object is called with it.
There was a problem hiding this comment.
This is a very interesting question that's worth expanding.. From my perspective mr.deallocate() is not meant to be called by end users because
- it was designed to be called by
Buffer.close()or its destructor when out of scope - it has such an ugly, not pythonic signature (passing pointer and size)
I don't think currently we have any test that actually exercise calling mr.deallocate() explicitly.
If this can be called by the end users, the contract that we deallocate using the allocation stream would not be upheld, because this API does not take any Buffer argument and so the stream is not available to us.
|
Thanks, all! I noticed I forgot to prepare a release note for this PR. Working on it now... |
|
|
4 participants
Description
closes #779.
Checklist