search

static-frame / arraykit

Python C Extensions for StaticFrame
Other
8 stars 2 forks source link

ArrayGO_getnewargs #24

Closed flexatone closed 3 years ago

flexatone commented 3 years ago

I think I wired up the ArrayGO_getnewargs function correctly in ArrayGO_methods; it is getting called as expected, and I can have it return None (then fail with a normal exception). However, when I try to use PyTuple_New and PyTuple_SET_ITEM, something goes wrong and I get a segfault. I assume I am not handling reference counting right for either the array or the tuple?

brandtbucher commented 3 years ago

Heh, I see the problem. It has nothing to do with your implementation.

__getnewargs__ requires that a class does its initialization logic in __new__... but we do it in __init__. So both self->array and self->list were NULL after unpickling, which breaks assumptions that most of our methods make about at least one of them existing.

Realistically, the current design is a bit flawed in this regard. I'll make a patch tomorrow to move this logic from __init__ to __new__!

brandtbucher commented 3 years ago

I moved all of the setup for ArrayGO instances from __init__ to __new__ in 971498e47070e3851e5e390189da773a3c1d7078. __getnewargs__ should start working correctly if you pull in the latest changes from master.

flexatone commented 3 years ago

So I pulled in from master and indeed now ArrayGO_getnewargs is no longer giving us a segfault. So I added a test to explicitly pickle ArrayGO and found the following. Ideas?

_________________________ TestUnit.test_array_pickle_a _________________________
Traceback (most recent call last):
  File "/home/ariza/src/arraykit/test/test_array_go.py", line 109, in test_array_pickle_a
    msg = pickle.dumps(ag1)
_pickle.PicklingError: Can't pickle <class 'ArrayGO'>: attribute lookup ArrayGO on builtins failed
brandtbucher commented 3 years ago

I added a test to explicitly pickle ArrayGO and found the following. Ideas?

I have a hunch that changing .tp_name to "arraykit.ArrayGO" will fix this (it's currently just "ArrayGO"). Do you mind trying?

flexatone commented 3 years ago

That was it! I was just looking at automap last night and noticed that tp_name had the module name.

brandtbucher commented 3 years ago

Land it!