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 inhttps://github.com/numpy/numpy/blob/master/tools/travis-before-install.sh
• update the tests in the existing numpy test suite to also test withprotocol=5 when possible (that is if pickle5 is installed or whenhasattr(pickle, 'PickleBuffer').

I don't think we can modify the signature ofPyArray_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:

>>>importnumpyasnp>>>importpickle5>>>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.writeableTrue

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, thePickleBuffer class of the protocol 5 propagates the readonly/writeable state and therefore:

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

@ahaldane@charris do you confirm that the fact that adumps/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 byb = np.array(a), even though we retain other properties like the memory order. To me doing asave + load is like constructing a new buffer from an old one, like thenp.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. Usingnumpy.ndarray.dumps,numpy.ndarray.dump, andnumpy.ndarray.__reduce_ex__

• ifprotocol > 5, aValueError will be raised saying'pickle protocol must be <=5'
• ifprotocol == 5, andpickle5 is not installed, anImportError will be raised telling the user thatpickle5 is needed

For__reduce_ex__, we raise aValueError ifprotocol > 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 noSystemError 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 thendarray.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:

• Somepickle.dumps tests that I did not catch inTST, 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:

ifsys.version_info[0]>=3:ifsys.version_info[1]in (6,7):try:importpickle5aspickleexceptImportError:importpickleelse:importpickleelse:importcPickleaspickle

each time we import pickle in python, we import itonce this way, innumpy.core.numeric, and then each timepickle is used in other files, we import it fromnumpy.core.numeric. The only time we importpickle another way is innumpy/core/setup.py

Now,numpy should be cleanpickle-wise, meaning:

• there is no morenp.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 implementsarray_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:

>>>importpandasaspd>>>importnumpyasnp>>>importpickle5aspickle>>>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.PickleBufferobjectat0x7f32f7eea5c8>,<pickle.PickleBufferobjectat0x7f32f7e5f848>]>>>pickle.loads(df_pkl,buffers=buffers)ab0-0.577441110.168650120.7832610

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