ARROW-1695: [Serialization] Fix reference counting of numpy arrays created in custom serializer #1220
Conversation
pcmoritz
changed the title
pcmoritz
force-pushed
the
fix-serialize-tensors
branch
from
9d87906 to
7e23bb5
Compare
|
Yes, this is correct! It is decref'd explicitly here: https://github.com/pcmoritz/arrow/blob/7e23bb5e7cd666e311595f76d60dbd08bf71920e/cpp/src/arrow/python/python_to_arrow.cc#L674 |
|
The test failure is in the go bindings and unrelated to this PR (cc @kou are you aware of this problem?). I'll merge this since I'd like to do some follow up work that needs this fix. |
| custom_serializer=serialize_dummy_class, | ||
| custom_deserializer=deserialize_dummy_class) | ||
|
|
||
| pa.serialize(DummyClass()) |
There was a problem hiding this comment.
Should also deserialize the object and assert that it is the correct value.
There was a problem hiding this comment.
Oops too late. This is a regression test to catch the segfault which used to already occur in the serialize call, so only testing that is fine; the actual codepath of serializing a numpy array via custom serializer will be tested in the PR I'm about to create, which is about serializing pytorch Tensors.
There was a problem hiding this comment.
You want to handle pytorch tensors in Arrow? As opposed to in Ray?
There was a problem hiding this comment.
Yes, I'd like to handle them in Arrow conditionally on pytorch being installed, much in the same way as we handle pandas. There is precedent for that, the glib bindings do it for raw lua torch tensors I think.
I created #1234 to fix it. |
|
Awesome, thanks :) |
This uses the NumPyBuffer built into Arrow's Tensor facility to protect the numpy arrays holding the Tensors to be serialized. See also the problem description in https://issues.apache.org/jira/browse/ARROW-1695.