Thank you very much for the new benchmarks. They look good: there is no more spurious memory copies when pickling and loading contiguous numpy arrays when using protocol 5 either with in-band or out-of-band buffers.

Next steps:

• install pickle5 under python 3.7 in https://github.com/numpy/numpy/blob/master/tools/travis-before-install.sh
• update the tests in the existing numpy test suite to also test with protocol=5 when possible (that is if pickle5 is installed or when hasattr(pickle, 'PickleBuffer').

I don't think we can modify the signature of PyArray_FromBuffer: It is part of the numpy C-api which we are very reluctant to modify, so it's better to go back to the wrapper function you had before.

Otherwise looks fine so far, at inital skim.

By playing this PR I noticed the following:

>>> import numpy as np
>>> import pickle5
>>> a = np.arange(12)

Let's mark the array read-only and then pickle it:

>>> a.flags.writeable = False
>>> pickle5.loads(a.dumps(protocol=2)).flags.writeable
True

The writeable flag is lost: it means that currently (in numpy master and all past versions), a readonly array would be loaded back as a read-write array. I think this is a bug of numpy. (I actually checked that I could reproduce the behavior with the last stable release of numpy).

On the contrary, the PickleBuffer class of the protocol 5 propagates the readonly/writeable state and therefore:

>>> pickle5.loads(a.dumps(protocol=5)).flags.writeable
False

@ahaldane @charris do you confirm that the fact that a dumps/loads cycle does not preserve the writeable flag can be considered a bug?

My initial impression is it's not a bug: We don't preserve the writeable flag by default when we construct a new array from another one by b = np.array(a), even though we retain other properties like the memory order. To me doing a save + load is like constructing a new buffer from an old one, like the np.array call.

@ahaldane thanks for the feedback. However I hope that you don't consider a bug that this PR would preserve the writeable flag for contiguous array with the protocol 5?

The fact that this behavior is not consistent if the array is contiguous or not might be surprising to the users though.

OK, I just reread the PEP and I see what you're getting at: Now that an unpickled ndarray might be just a view of the original pickled ndarray, we have to maintain the writeable flags to avoid creating writeable views of arrays which have writeable=False.

That seems like a strong argument for maintaining the writeable flag.

One might object that it's a bit strange that whether or not an unpickled ndarray is writeable depends on the pickle protocol and whether it was saved to disk or not. But on the other hand, maybe it's easy to explain to people: Protocol <5 always returns a copy, so it is always writeable, while protocol 5 can return a view in which case users should expect it to sometimes be unwriteable. That's the same as many other numpy operations where copies are writeable while views might not be, eg fancy indexing vs slice indexing.

In summary, the way you have it sounds fine.

So a little clarification on the error handling:
I think we need personalized messages for python 3.6 and 3.7, alerting users that the highest protocol possible is 5, and not 4, as pickle would do. Using numpy.ndarray.dumps, numpy.ndarray.dump, and numpy.ndarray.__reduce_ex__

• if protocol > 5, a ValueError will be raised saying'pickle protocol must be <=5'
• if protocol == 5, and pickle5 is not installed, an ImportError will be raised telling the user that pickle5 is needed

For __reduce_ex__, we raise a ValueError if protocol > 5, and if ( protocol == 5 and sys.version_info[:2] < (3, 6)

Otherwise, we let pickle raise its own error messages, that tell the right information. With the code used right now, there will be no SystemError anymore, only pickle error messages, or the messages stated above.

This is all being tested by the way, yielding a drop in the coverage because some tests are mutually exclusive on the python version, but the coverage report is specified using only python 3.6

This is all being tested by the way, yielding a drop in the coverage because some tests are mutually exclusive on the python version, but the coverage report is specified using only python 3.6

We might want to enable coverage collection on Python 3.5 for this reason, no?

codecov aggregates all the coverage stats and reports the union of coverage reports.

We'd just have to be a little cautious since some projects (i.e., SciPy) appear to have reached the codecov data limit, and then nothing works.

Now that #12090, #12091 have been merged, this should be rebased off master and squashed to a smaller number (1?) of relevant commits.

Also the changes in the ndarray.dump(s) should be removed from this PR.

UPDATE:
Just rebased the PR. I made 2 commits though (and not one), because there still were some cleanups to do about pickle that were not included in #12090. Most notably:

• Some pickle.dumps tests that I did not catch in TST, MAINT: Update pickling tests by making them loop over all protocols #12090 that are now looped over for every pickle protocol
• Some refactoring about how pickle is imported internally: since we don't want a big conditional block such as:

if sys.version_info[0] >= 3:
    if sys.version_info[1] in (6, 7):
        try:
            import pickle5 as pickle
        except ImportError:
            import pickle
    else:
        import pickle
else:
    import cPickle as pickle

each time we import pickle in python, we import it once this way, in numpy.core.numeric, and then each time pickle is used in other files, we import it from numpy.core.numeric. The only time we import pickle another way is in numpy/core/setup.py

Now, numpy should be clean pickle-wise, meaning:

• there is no more np.ndarray.dump(s) call in the test suite.
• there is a consistent pickle importing behavior
• each time pickle.dumps is tested, it is tested for every protocol.

This is what the first commit is about.

The second commit implements array_reduce_ex, as well as some tests to check the good behavior of pickle protocol 5 on numpy arrays. This PR can thus be easily broken out in two smaller PRs, one for each commit.
Preventively, I created the PR (#12133) for the first commit only, on which this branch can be furtherly rebased on.

Happy to hear your suggestions,
Pierre

Needs a release note.

Any thoughts about adding this to the NumPy API as some point?

Great work. Thanks everyone!

@pitrou are you confident that PEP 574 will be accepted and your protocol 5 branch be merged in CPython before the release of Python 3.8? Otherwise some assumptions made in the code of this PR might be slightly off although they should not cause any breakage (I believe).

Any thoughts about adding this to the NumPy API as some point?

Can you be more specific? I don't think I understand what you are suggesting.

@pitrou are you confident that PEP 574 will be accepted and your protocol 5 branch be merged in CPython before the release of Python 3.8?

I don't think the PEP will be controversial. The main uncertainty at this point is around the schedule for choosing and putting the new governance model in place. But I don't expect 3.8 to be feature-frozen before that is done anyway :-)

For reference, using the current numpy master and the latest stable release of pandas, I confirm that pickle protocol 5 makes it possible to have no-copy semantics for pandas dataframes without changing anything in pandas:

>>> import pandas as pd
>>> import numpy as np
>>> import pickle5 as pickle
>>> df = pd.DataFrame({'a': np.random.randn(3), 'b': np.random.randint(0, 2, 3)})
>>> buffers = []
>>> df_pkl = pickle.dumps(df, protocol=5, buffer_callback=buffers.append)
>>> buffers
[<pickle.PickleBuffer object at 0x7f32f7eea5c8>, <pickle.PickleBuffer object at 0x7f32f7e5f848>]
>>> pickle.loads(df_pkl, buffers=buffers)
          a  b
0 -0.577441  1
1  0.168650  1
2  0.783261  0

@ogrisel This is great :-) I suspect @mrocklin will be interested.