classification
Title: Pure Python pickle module should not depend on _pickle.PickleBuffer
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Alex.Willmer, christian.heimes, miss-islington, pitrou, vstinner
Priority: normal Keywords: 3.8regression, patch

Created on 2019-06-09 20:23 by christian.heimes, last changed 2019-06-13 12:28 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
bp0-37201_make_test_20190610.txt Alex.Willmer, 2019-06-11 07:31 `make test` output for pickle._PickleBuffer attempt
Pull Requests
URL Status Linked Edit
PR 14016 merged vstinner, 2019-06-12 14:12
PR 14055 merged miss-islington, 2019-06-13 11:59
Messages (15)
msg345088 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2019-06-09 20:23
In https://twitter.com/moreati/status/1137815804530049028 Alex Willmer wrote:

> In CPython 3.8dev the pure http://pickle.py  module now depends on the C _pickle module, but only for one class https://github.com/python/cpython/blob/c879ff247ae1b67a790ff98d2d59145302cd4e4e/Lib/pickle.py#L39

Antoine, would it make sense turn _pickle.PickleBuffer into an optional dependency? The class is only used for pickle 5 as described in PEP 574, https://www.python.org/dev/peps/pep-0574/
msg345089 - (view) Author: Alex Willmer (Alex.Willmer) * Date: 2019-06-09 20:40
I noticed this because I was experimenting with pickle.py from the 3.8 branch to support protocol 5 in https://github.com/moreati/pikl (and later to https://pypi.org/project/zodbpickle/).

However I want to make it clear, if CPython maintainers wish to keep the code as it is I will deal with it. Maximizing our convenience, and minimizing your maintenance workload is paramount.
msg345090 - (view) Author: Alex Willmer (Alex.Willmer) * Date: 2019-06-09 20:43
Arggh, typo. I mean maximizing *your* convenience is paramount.
msg345093 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2019-06-09 20:58
Note that PickleBuffer is really a built-in type (it's defined in Objects/picklebufobject.c). However it's exposed from _pickle.c because it doesn't make sense to expose it in the `builtins` module.
msg345094 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2019-06-09 21:01
In short I'm not against making PickleBuffer optional but I'm not sure it makes a lot of sense.  Would you make memoryview optional because it's written in C?  Is anyone annoyed by the fact that something comes from _pickle.c.

(note that the pure Python Pickler class *does* work with PickleBuffer; so you don't lose any extensability or customizability by using PickleBuffer)

In any case, I won't do the work myself, so feel free to draft a PR so that we see the magnitude of the changes required.
msg345095 - (view) Author: Alex Willmer (Alex.Willmer) * Date: 2019-06-09 21:27
Attempting a PR
msg345197 - (view) Author: Alex Willmer (Alex.Willmer) * Date: 2019-06-11 07:31
I don't think I can do this. My WIP code is in https://github.com/moreati/cpython/pull/new/bpo-37210, and associated make test output is attached.

Principal blockers

-  `_pickle.PickleBuffer.raw()` can return a contiguous buffer from either a c_contiguous, or f_contiguous buffer. memoryview() (and hence pickle._PickleBuffer.raw()) requires a c_contiguous buffer.
- I'm unsure how to allow _PickleBuffer -> bytearray conversion

The most common cause of test failures is `TypeError: cannot pickle 'memoryview' object`. I guess Python is seeing a lack of __reduce__(), and failing back to walking the object attributes. Implementing __reduce__() and or __reduce_ex__() might fix this, but seems moot given the other blockers.
msg345198 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2019-06-11 08:07
Right, it is probably not possible to write a pure Python PickleBuffer. There's a reason it's written in C...

When you said "make PickleBuffer an optional dependency", I thought you intended to have a usable pure Python Pickler, but without support for the PickleBuffer class.
msg345261 - (view) Author: Alex Willmer (Alex.Willmer) * Date: 2019-06-11 18:59
> it is probably not possible to write a pure Python PickleBuffer

Fair enough

> a usable pure Python Pickler, but without support for the PickleBuffer class.

That makes sense. However, for third party packages (e.g. zodbpickle, pikl) wanting a pure Python pickle module to use as a basis, I think it would make more sense to use the CPython 3.7 branch as a basis. Adding a pickle._Pickler variant to the 3.8 stdlib that can't do protocol 5, just for third-parties imposes a maintenance burden on the core devs for zero benefit to anyone.

I propose closing this ticket as WONTFIX.
msg345262 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2019-06-11 19:36
Ok, closing then.
msg345360 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-12 14:02
FYI pyperformance is affected by this issue:

vstinner@apu$ env/bin/python ~/prog/python/pyperformance/pyperformance/benchmarks/bm_pickle.py unpickle --pure-python -v -p3  
Traceback (most recent call last):
  File "/home/vstinner/prog/python/pyperformance/pyperformance/benchmarks/bm_pickle.py", line 287, in <module>
    import pickle
  File "/home/vstinner/prog/python/master/Lib/pickle.py", line 39, in <module>
    from _pickle import PickleBuffer
ModuleNotFoundError: import of _pickle halted; None in sys.modules

It is no longer possible to benchmark the pure Python implement of pickle.

--pure-python uses this code path:

def is_module_accelerated(module):
    return getattr(pickle.Pickler, '__module__', '<jython>') == 'pickle'

(...)

        if six.PY3:
            sys.modules['_pickle'] = None
        import pickle
        if not is_module_accelerated(pickle):
            raise RuntimeError("Unexpected C accelerators for pickle")

Should I remove the benchmark?
msg345362 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-12 14:13
I wrote a simple PR, PR 14016, to fix the pure Python implementation of pickle: simply restrict pickle to protocol 4 if PickleBuffer is missing.
msg345503 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-13 11:59
New changeset 63ab4ba07b492448844940c347787ba30735b7f2 by Victor Stinner in branch 'master':
bpo-37210: Fix pure Python pickle when _pickle is unavailable (GH-14016)
https://github.com/python/cpython/commit/63ab4ba07b492448844940c347787ba30735b7f2
msg345511 - (view) Author: miss-islington (miss-islington) Date: 2019-06-13 12:28
New changeset cbda40db7b604b377acfd3f04e19407ca33748a7 by Miss Islington (bot) in branch '3.8':
bpo-37210: Fix pure Python pickle when _pickle is unavailable (GH-14016)
https://github.com/python/cpython/commit/cbda40db7b604b377acfd3f04e19407ca33748a7
msg345512 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-13 12:28
I pushed my change to 3.8 and master branches. I close the issue.
History
Date User Action Args
2019-06-13 12:28:44vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg345512

stage: patch review -> resolved
2019-06-13 12:28:21miss-islingtonsetnosy: + miss-islington
messages: + msg345511
2019-06-13 11:59:03miss-islingtonsetkeywords: + patch
stage: resolved -> patch review
pull_requests: + pull_request13917
2019-06-13 11:59:02vstinnersetmessages: + msg345503
2019-06-12 14:13:10vstinnersetstatus: closed -> open
resolution: wont fix -> (no value)
2019-06-12 14:13:04vstinnersetmessages: + msg345362
2019-06-12 14:12:12vstinnersetpull_requests: + pull_request13881
2019-06-12 14:02:18vstinnersetnosy: + vstinner
messages: + msg345360
2019-06-11 19:36:39pitrousetstatus: open -> closed
resolution: wont fix
messages: + msg345262

stage: resolved
2019-06-11 18:59:52Alex.Willmersetmessages: + msg345261
2019-06-11 08:07:56pitrousetmessages: + msg345198
2019-06-11 07:31:54Alex.Willmersetfiles: + bp0-37201_make_test_20190610.txt

messages: + msg345197
2019-06-09 21:27:25Alex.Willmersetmessages: + msg345095
2019-06-09 21:01:07pitrousetmessages: + msg345094
2019-06-09 20:58:53pitrousetmessages: + msg345093
2019-06-09 20:43:47Alex.Willmersetmessages: + msg345090
2019-06-09 20:40:44Alex.Willmersetnosy: + Alex.Willmer
messages: + msg345089
2019-06-09 20:23:32christian.heimescreate