BUG: Fix pickle and memoryview for datetime64, timedelta64 scalars by mattip · Pull Request #11694 · numpy/numpy

mattip · 2018-08-09T05:56:15Z

Fixes #11656, which is a regression after #10564.

Adds tests for pickling datetime64 and timedelta64 scalars. Pickling arrays of these dtypes works but pickling scalars fails.

The problem seems to be in the new code for gentype_getbuffer, which creates a PEP 3118 compliant buffer. The old code was:

int gentype_getbuffer(PyObject *self, Py_buffer *view, int flags)
{
    Py_ssize_t len;
    void *buf;
    /* FIXME: XXX: the format is not implemented! -- this needs more work */
    len = gentype_getreadbuf(self, 0, &buf);
    return PyBuffer_FillInfo(view, self, buf, len, 1, flags);
}

so now I need to find what is happening in that code that is not happening in the new code.

So far I only found where we override an original exception with a new one. ~~Now memoryview(np.datetime64('2016-01-01')) raises an exception with the same message as `memoryview(np.array(100, 'M8[D]').~~

Edit: python2 - python3 confusion

eric-wieser · 2018-08-09T06:08:11Z

    info = _buffer_get_info(self);
    if (info == NULL) {
-        PyErr_SetString(PyExc_BufferError,
+        if (!PyErr_Occurred()) {


I don't think this if can ever be hit (at least, after gh-11695)

Try setting a breakpoint there and calling memoryview(np.datetime64('2016-01-01')) or more to the point of the issue pickle.dumps(np.datetime64('2016-01-01')). The call chain is gettype_getbuffer -> _buffer_get_info -> _buffer_info_new -> _buffer_format_string -> switch statement -> missing case marked with XXX -> raising error with "cannot include dtype '%c' in a buffer".

To be clear - I think _buffer_get_info is well behaved, and will always set an exception if it returns NULL.

eric-wieser · 2018-08-09T07:26:29Z

In case it helps, this was the old behavior

def as_dict(m):
	return dict(shape=m.shape, itemsize=m.itemsize, ndim=m.ndim, format=m.format)

>>> as_dict(memoryview(np.datetime64('2016-01-01')))
{'itemsize': 1, 'ndim': 1, 'shape': (8,), 'format': 'B'}

mattip · 2018-08-09T23:11:31Z

The memoryview info for datetime64 and timedelta64 uses B which is a single byte, so the code needs to special case the memoryview buffer generation. The dtype.itemsize == 8, dtype.shape == () becomes the memoryview mv.itemsize == 1, mv.shape == (8,0). If the dtype used `L' or 'l', the code would be cleaner.

mattip · 2018-08-09T23:54:10Z

I cleared the WIP in the title, this seems to work now.

eric-wieser · 2018-08-10T02:48:51Z

+        dt1 = np.datetime64('2016-01-01')
+        dt2 = np.datetime64('2017-01-01')
+        expected = {'strides': (1,), 'itemsize': 1, 'ndim': 1,
+                    'shape': (8,), 'format': 'B'}


One interesting idea would be to set the format to T{8B:value:T{}:__numpy_dtype_m8[N]:} so that we can actually round-trip this

eric-wieser · 2018-08-10T07:27:57Z

+        assert_equal(res, expected)
+        assert_equal(v.tolist(), lst)
+
+        lst = [110, 1, 0, 0, 0, 0, 0, 0]


I think this might be making assumptions about endianness

eric-wieser · 2018-08-10T07:28:46Z

                assert_(base <= sys.getrefcount(s))
+
+    @pytest.mark.skipif(sys.version_info < (3, 0), reason="Python3 only")
+    def test_datetime_memoryview(self):


I think this test might be better alongside wherever the other memoryview tests are

eric-wieser · 2018-08-10T16:55:45Z

NameError: lst

eric-wieser · 2018-08-11T02:40:50Z

-        case NPY_UBYTE:        if (_append_char(str, 'B')) return -1; break;
+        case NPY_UBYTE:
+        case NPY_DATETIME:
+        case NPY_TIMEDELTA:    if (_append_char(str, 'B')) return -1; break;


Would it be legal to set the format to 8B instead, and leave the shape as ()?

Why not 'Q' then, since we call these datetime64 and timedelta64, the 64 bit is explicit. This would mean that d=np.ones((3), 'M8[D]'); np.array(memoryview(d)) would return a uint64 array, rather that a 3 by 8 'uint8' one.

eric-wieser · 2018-08-11T02:41:54Z

Can you add a test for memoryview of datetime arrays?

mattip · 2018-08-13T23:55:28Z

Can you add a test for memoryview of datetime arrays?

Added tests for both scalars and arrays. After changing the format string to 'Q' I could remove the special casing in memoryview(scalar) added in the earlier commits. Also now np.asarray(memoryview(d)) works where it used to raise an error (also improved the error message).

I tried to hit the "Sanity check" errors in tests. Should I go to C to create non-compliant buffers?

mattip · 2018-08-14T23:57:18Z

Would this be easier to review if I move the last commit 63e44e3 to a separate PR? Then we could get this into 1.15.1 without changing the buffer format from B. Note various things would fail, but they failed previously (all on python3) where

m=memoryview(np.array((3), 'M8[D]'))
d = memoryview(np.datetime64('2001-01-01'))
a = np.array(m)
b = np.array(d)

version	m succeeds?	d succeeds?	a succeeds?	b succeeds?
1.13.0	no	yes	(not relevant)	yes
1.14.0	no	yes	(not relevant)	yes
1.15.0	no	no	(not relevant)	(not relevant)
`895a2a8` (format=`B`)	yes	yes	no	yes
`63e44e3` (format=`Q`)	yes	yes	yes	yes

charris · 2018-08-15T16:34:19Z

Does the 'Q' format need a prefix, '=' perhaps, to indicate standard size (8) and avoid compiler dependence? I'm thinking of https://docs.python.org/2/library/struct.html.

eric-wieser · 2018-08-15T16:37:32Z

Then we could get this into 1.15.1 without changing the buffer format from B

I agree - let's leave the buffer format as it was in 1.14.x for the 1.15.x series - we can change it in 1.16

What do you mean by "a doesn't succeed"?

eric-wieser · 2018-08-15T16:40:09Z

-                    "Item size computed from the PEP 3118 buffer format "
-                    "string does not match the actual item size.");
+                    "Item size %d for PEP 3118 buffer format "
+                    "string %s does not match the dtype %c item size %d.",


Better to use the INTP format macro here (or perhaps %z) for itemsize, rather than casting.

Edit: %zd

mattip · 2018-08-15T23:32:37Z

What do you mean by "a doesn't succeed"?

I mean that executing the line a = np.array(m) raises an exception. Also, even though np.array(memoryview) may not raise, it will not create a datetime dtype, so I should have qualified the "yes" in a and b columns in that table.

mattip · 2018-08-15T23:56:02Z

I backed out the last commit to fix only the regression on pyhon 3 between 1.14 and 1.15

eric-wieser · 2018-08-16T00:57:35Z

What is the exception that's thrown?

mattip · 2018-08-17T07:18:59Z

added test to gentype_getbuffer for PEP 3118 compliant buffer creation, test now raises instead of passing (as it should)

charris · 2018-08-17T23:11:11Z

@eric-wieser Look good?

eric-wieser · 2018-08-18T04:22:54Z

I'm not really a fan of this approach - what we've ended up doing is:

Changing an internal function to return an illegal result
Added a bunch of special cases to the caller to try and fix it
Calling an expensive python-side parser to determine whether our special cases were sufficient

This does at least meet the requirement of not giving broken behavior to the user. I'm not sure if there's an easier way to achieve this.

eric-wieser · 2018-08-18T04:24:19Z

Can't you just set the format directly here and return early, rather than modifying _buffer_format_string to do something incorrect?

eric-wieser · 2018-08-18T04:26:30Z

This comment should be explaining that the format is being exposed as B - scalars are always 1 is false - sizeof(B) == 1 is what's relevant here.

removing code, comment expanded above.

Whoops, this is needed. Comment fixed.

eric-wieser · 2018-08-18T04:27:08Z

I assume that in the tests this does occur? If so, perhaps say when it occurs in the comment.

eric-wieser · 2018-08-18T20:09:41Z

I think shape and strides are uninitialized here - they're malloc'd below for some cases.

fixed to be compliant with _buffer_info_t allocation strategy

mattip · 2018-08-18T22:00:11Z

Special-cased memoryview(d) where d is a datetime64 scalar. This now fixes the original issue, adds tests for memoryview and pickle ing datetime64 objects, and never creates non-compliant buffers.

eric-wieser · 2018-08-19T02:45:41Z

malloc can fail - this needs to call PyErr_NoMemory() if so

eric-wieser · 2018-08-19T02:46:00Z

I'd prefer this appear after the comment, alongside all the other assigments

eric-wieser

Looks good now, thanks for the persistence. Will merge and squash if you don't want to squash yourself.

eric-wieser · 2018-08-19T05:53:08Z

Something for another PR: We ought to be checking the return value of _append_char, but we don't

…ception

mattip · 2018-08-19T06:42:06Z

sqaushed to two commits.

eric-wieser · 2018-08-19T07:23:39Z

+        assert_raises((ValueError, BufferError), memoryview, a)
+        assert_raises((ValueError, BufferError), memoryview, np.array((3), 'M8[D]'))
+
+


super-nit: too many blank lines. Not worth fixing though - something to pick up if / when we apply a PEP8-checker

eric-wieser · 2018-08-19T07:24:26Z

Thanks @mattip!

eric-wieser mentioned this pull request Aug 9, 2018

BUG: Add missing PyErr_NoMemory after failing malloc #11695

Merged

eric-wieser reviewed Aug 9, 2018

View reviewed changes

charris added 00 - Bug 06 - Regression component: numpy._core 09 - Backport-Candidate PRs tagged should be backported labels Aug 9, 2018

charris added this to the 1.15.1 release milestone Aug 9, 2018

mattip changed the title ~~WIP: BUG: pickle and memoryview for datetime64, timedelta64 scalars~~ BUG: pickle and memoryview for datetime64, timedelta64 scalars Aug 9, 2018

eric-wieser reviewed Aug 10, 2018

View reviewed changes

mattip force-pushed the master branch from c10609e to 895a2a8 Compare August 10, 2018 18:29

eric-wieser reviewed Aug 11, 2018

View reviewed changes

mattip force-pushed the master branch 2 times, most recently from c38cd2b to 63e44e3 Compare August 13, 2018 23:50

eric-wieser reviewed Aug 15, 2018

View reviewed changes

mattip force-pushed the master branch from 63e44e3 to 895a2a8 Compare August 15, 2018 23:52

mattip mentioned this pull request Aug 16, 2018

ENH: change buffer format string for datetime64 to '=Q' #11749

Closed

mattip force-pushed the master branch 2 times, most recently from 69f279d to 27cd5a0 Compare August 17, 2018 10:08

eric-wieser reviewed Aug 18, 2018

View reviewed changes

mattip force-pushed the master branch from 27cd5a0 to d66b04e Compare August 18, 2018 19:22

eric-wieser reviewed Aug 18, 2018

View reviewed changes

mattip force-pushed the master branch 2 times, most recently from ce53937 to eb1b209 Compare August 18, 2018 20:25

eric-wieser reviewed Aug 19, 2018

View reviewed changes

eric-wieser approved these changes Aug 19, 2018

View reviewed changes

eric-wieser reviewed Aug 19, 2018

View reviewed changes

mattip added 2 commits August 19, 2018 09:39

MAINT: test memoryview, improve error message, don't swallow first ex…

83cd724

…ception

BUG: add tests, special case memoryview(datetime64-scalar)

c94d1c7

mattip force-pushed the master branch from c56423a to c94d1c7 Compare August 19, 2018 06:40

eric-wieser reviewed Aug 19, 2018

View reviewed changes

eric-wieser merged commit e8d177f into numpy:master Aug 19, 2018

charris changed the title ~~BUG: pickle and memoryview for datetime64, timedelta64 scalars~~ BUG: Fix pickle and memoryview for datetime64, timedelta64 scalars Aug 19, 2018

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Aug 19, 2018

charris mentioned this pull request Aug 19, 2018

BUG: Fix pickle and memoryview for datetime64, timedelta64 scalars #11785

Merged

charris removed this from the 1.15.1 release milestone Aug 19, 2018

vanossj mentioned this pull request Sep 27, 2018

BUG: fix memory leak of buffer format string #12045

Merged

jakirkham mentioned this pull request Nov 24, 2018

datetime64 arrays don't support buffer protocol #4983

Open

		assert_raises((ValueError, BufferError), memoryview, a)
		assert_raises((ValueError, BufferError), memoryview, np.array((3), 'M8[D]'))

Conversation

mattip commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip commented Aug 9, 2018

Uh oh!

mattip commented Aug 9, 2018

Uh oh!

eric-wieser Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Aug 11, 2018

Uh oh!

mattip commented Aug 13, 2018

Uh oh!

mattip commented Aug 14, 2018

Uh oh!

charris commented Aug 15, 2018

Uh oh!

eric-wieser commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Aug 15, 2018

Uh oh!

mattip commented Aug 15, 2018

Uh oh!

eric-wieser commented Aug 16, 2018

Uh oh!

mattip commented Aug 17, 2018

Uh oh!

charris commented Aug 17, 2018

Uh oh!

eric-wieser commented Aug 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Aug 9, 2018 •

edited

Loading

eric-wieser Aug 9, 2018 •

edited

Loading

eric-wieser commented Aug 9, 2018 •

edited

Loading

eric-wieser Aug 10, 2018 •

edited

Loading

eric-wieser commented Aug 15, 2018 •

edited

Loading

eric-wieser Aug 15, 2018 •

edited

Loading

eric-wieser Aug 18, 2018 •

edited

Loading