BUG: Fix SystemError when pickling datetime64 array with pickle5 #12748

pitrou · 2019-01-15T13:58:35Z

Check for error when calling the PickleBuffer constructor in ndarray.__reduce_ex__,
and fall back on the regular __reduce__ implementation for arrays which don't
support taking a buffer.

Also fix some reference leaks.

Fixes gh-12745, gh-12793

pitrou · 2019-01-15T16:04:44Z

@pierreglaser What are you doing on _pickle.c?

pierreglaser · 2019-01-15T16:13:06Z

Dynamic/nested scope objects pickling (See this python-dev thread, been working on this for a little while already), feel free to reach out if you are interested!

pitrou · 2019-01-19T21:56:12Z

I'll note that I get this error after running numpy/core/tests/test_multiarray.py:

*** Reference count error detected: 
an attempt was made to deallocate 12 (d) ***

The error is still there if I disable all pickling tests, though, so it's probably not pickling-related.

eric-wieser · 2019-01-19T21:57:24Z

numpy/core/src/multiarray/methods.c

 #if PY_VERSION_HEX >= 0x03080000
-            pickle_module = PyImport_ImportModule("pickle");
+    pickle_module = PyImport_ImportModule("pickle");
 #elif PY_VERSION_HEX < 0x03080000 && PY_VERSION_HEX >= 0x03060000


A nit: you could drop the PY_VERSION_HEX < 0x03080000 here, it's implied by the condition above.

You're right.

eric-wieser · 2019-01-19T22:00:16Z

numpy/core/src/multiarray/methods.c

+         ((PyObject*)self)->ob_type != &PyArray_Type) ||
+        PyDataType_ISUNSIZED(descr)) {
+        /* The PickleBuffer class from version 5 of the pickle protocol
+         * can only be used for arrays backed by a contiguous data buffer.


The PEP says "PickleBuffer can wrap any kind of buffer, including non-contiguous buffers.". Am I missing something?

Only contiguous buffers are correctly handled by the pickle module, though.
(this is something that should probably be improved in the future, but is non-trivial)

Is that mentioned in the PEP, pickle docs, or cpython bug tracker?

No, I have to update the PEP.

eric-wieser · 2019-01-19T22:13:33Z

numpy/core/tests/test_multiarray.py

+            del a, DATA, carray
+            # check for reference leaks (gh-12793)
+            for ref in refs:
+                assert ref() is None


@mattip, this type of test will fail on pypy, right?

Hmm, yes, I should call gc.collect.

Could skip for pypy. These sorts of tests do tend to be fragile.

seberg · 2019-01-19T22:25:15Z

@pitrou I am curious, what do I need to reproduce that error when running the tests? It seems plausible that is new, or was hidden behind some reference leaks before?

pitrou · 2019-01-19T22:26:53Z

python runtests.py -t numpy/core/tests/test_multiarray.py reproduces it here.

seberg · 2019-01-19T22:28:25Z

Ah, was wondering if I need a debug build at the least or so, because I cannot reproduce it (am on python 3.7.2).

pitrou · 2019-01-19T22:29:43Z

I have Python 3.7.2 from Anaconda. Weird.

seberg · 2019-01-19T22:34:24Z

Am on arch linux right now, but not quite sure why it should make a difference... I guess that error message does mean the actual integer 12. Maybe there are just too many things potentially touching it to make it easy to reproduce :/.

pitrou · 2019-01-19T22:37:14Z

Ok, I bisected the tests manually. The following test is sufficient to reproduce here:

def test_array_interface():
    # Test scalar coercion within the array interface
    class Foo(object):
        def __init__(self, value):
            self.value = value
            self.iface = {'typestr': '=f8'}

        def __float__(self):
            return float(self.value)

        @property
        def __array_interface__(self):
            return self.iface

    f = Foo(0.5)
    f.iface['shape'] = (2,)
    assert_raises(ValueError, np.array, f)

seberg · 2019-01-19T22:39:07Z

@pitrou thanks a lot! I can see the refcount decrase, I guess I just don't reach 0 on my system.

EDIT: Or dunno if its that, but I see the error if I run the test often enough anyway.

pitrou · 2019-01-19T22:47:14Z

The following change seems to solve the issue:

diff --git a/numpy/core/src/multiarray/ctors.c b/numpy/core/src/multiarray/ctors.c
index 63bf5377a..4724b8472 100644
--- a/numpy/core/src/multiarray/ctors.c
+++ b/numpy/core/src/multiarray/ctors.c
@@ -2503,6 +2503,7 @@ PyArray_FromInterface(PyObject *origin)
     if (ret == NULL) {
         goto fail;
     }
+    dtype = NULL;  /* ref was stolen by PyArray_NewFromDescrAndBase */
     if (data == NULL) {
         if (PyArray_SIZE(ret) > 1) {
             PyErr_SetString(PyExc_ValueError,

I'll let someone else handle it, as I don't really know what the surrounding code is doing.

seberg · 2019-01-19T23:02:54Z

Dang, should have checked here earlier, just found that as well :).

seberg · 2019-01-19T23:06:53Z

Thanks @pitrou. I doubt many know that code well, but I figured the same fix, so that seems like a good sign.

charris · 2019-01-22T03:22:51Z

Is this PR waiting on completion?

pitrou · 2019-01-22T06:07:27Z

No, it should be ready.

numpy/core/src/multiarray/methods.c

mattip · 2019-01-22T06:17:26Z

one very minor nit. We can check after merge if the new test is flaky on PyPy and skip it in a new PR. LGTM.

Fixes numpygh-12745

charris · 2019-01-23T18:17:28Z

Thanks @pitrou .

…py#12748) * BUG: Fix SystemError when pickling datetime64 array with pickle5 Fixes numpygh-12745 * Fix reference and error handling * Add a test for the fixed reference leak * Fix for PyPy + simplify #if condition * Fix comment

pierreglaser · 2019-01-25T11:14:02Z

thanks for working on that @pitrou!

jakirkham · 2020-05-08T00:14:24Z

numpy/core/src/multiarray/methods.c

    }
    else {
+        PyErr_Format(PyExc_ValueError,
+                     "__reduce_ex__ called with protocol > 5");


Is there a reason we expect this not to work with a later Python pickle protocol?

charris added 00 - Bug component: numpy._core 09 - Backport-Candidate PRs tagged should be backported labels Jan 15, 2019

charris added this to the 1.16.1 release milestone Jan 15, 2019

pitrou mentioned this pull request Jan 19, 2019

Memory leak when pickling ndarrays with NumPy 1.16.0 #12793

Closed

eric-wieser reviewed Jan 19, 2019

View reviewed changes

mattip mentioned this pull request Jan 20, 2019

BUG: double decref of dtype in failure codepath. Test and fix #12805

Merged

charris mentioned this pull request Jan 20, 2019

BUG: double decref of dtype in failure codepath. Test and fix #12812

Merged

mattip reviewed Jan 22, 2019

View reviewed changes

numpy/core/src/multiarray/methods.c Outdated Show resolved Hide resolved

pitrou added 3 commits January 22, 2019 10:13

BUG: Fix SystemError when pickling datetime64 array with pickle5

48ac84a

Fixes numpygh-12745

Fix reference and error handling

c93cdb2

Add a test for the fixed reference leak

ceabdf6

pitrou added 2 commits January 22, 2019 10:13

Fix for PyPy + simplify #if condition

7e5318c

Fix comment

7801e07

pitrou force-pushed the gh-12745-pickle5-systemerror branch from 9884804 to 7801e07 Compare January 22, 2019 09:17

charris merged commit a5cace4 into numpy:master Jan 23, 2019

pitrou deleted the gh-12745-pickle5-systemerror branch January 23, 2019 19:44

charris mentioned this pull request Jan 24, 2019

BUG: Fix SystemError when pickling datetime64 array with pickle5 #12844

Merged

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jan 24, 2019

charris removed this from the 1.16.1 release milestone Jan 24, 2019

jakirkham reviewed May 8, 2020

View reviewed changes

eric-wieser mentioned this pull request May 8, 2020

ENH: Allow pickle with protocol 5 when higher is requested #16185

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix SystemError when pickling datetime64 array with pickle5 #12748

BUG: Fix SystemError when pickling datetime64 array with pickle5 #12748

pitrou commented Jan 15, 2019 •

edited

pitrou commented Jan 15, 2019

pierreglaser commented Jan 15, 2019

pitrou commented Jan 19, 2019 •

edited

eric-wieser Jan 19, 2019

pitrou Jan 19, 2019

eric-wieser Jan 19, 2019

pitrou Jan 19, 2019

eric-wieser Jan 19, 2019

pitrou Jan 19, 2019

eric-wieser Jan 19, 2019

pitrou Jan 19, 2019

charris Jan 22, 2019

seberg commented Jan 19, 2019

pitrou commented Jan 19, 2019

seberg commented Jan 19, 2019

pitrou commented Jan 19, 2019

seberg commented Jan 19, 2019

pitrou commented Jan 19, 2019

seberg commented Jan 19, 2019 •

edited

pitrou commented Jan 19, 2019

seberg commented Jan 19, 2019

seberg commented Jan 19, 2019

charris commented Jan 22, 2019

pitrou commented Jan 22, 2019

mattip commented Jan 22, 2019

charris commented Jan 23, 2019

pierreglaser commented Jan 25, 2019

jakirkham May 8, 2020

BUG: Fix SystemError when pickling datetime64 array with pickle5 #12748

BUG: Fix SystemError when pickling datetime64 array with pickle5 #12748

Conversation

pitrou commented Jan 15, 2019 • edited

pitrou commented Jan 15, 2019

pierreglaser commented Jan 15, 2019

pitrou commented Jan 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg commented Jan 19, 2019

pitrou commented Jan 19, 2019

seberg commented Jan 19, 2019

pitrou commented Jan 19, 2019

seberg commented Jan 19, 2019

pitrou commented Jan 19, 2019

seberg commented Jan 19, 2019 • edited

pitrou commented Jan 19, 2019

seberg commented Jan 19, 2019

seberg commented Jan 19, 2019

charris commented Jan 22, 2019

pitrou commented Jan 22, 2019

mattip commented Jan 22, 2019

charris commented Jan 23, 2019

pierreglaser commented Jan 25, 2019

Choose a reason for hiding this comment

pitrou commented Jan 15, 2019 •

edited

pitrou commented Jan 19, 2019 •

edited

seberg commented Jan 19, 2019 •

edited