Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Recursive OrderedDict pickling #62100

Closed
serhiy-storchaka opened this issue May 3, 2013 · 11 comments
Closed

Recursive OrderedDict pickling #62100

serhiy-storchaka opened this issue May 3, 2013 · 11 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 17900
Nosy @rhettinger, @pitrou, @avassalotti, @serhiy-storchaka
Files
  • OrderedDict_pickle_recursive.patch
  • test_yaml.py: Show the YAML breakage.
  • OrderedDict_pickle_PyYAML.patch
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2016-06-12.08:27:22.492>
created_at = <Date 2013-05-03.19:40:16.705>
labels = ['type-bug', 'library']
title = 'Recursive OrderedDict pickling'
updated_at = <Date 2016-06-12.08:27:22.490>
user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2016-06-12.08:27:22.490>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2016-06-12.08:27:22.492>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2013-05-03.19:40:16.705>
creator = 'serhiy.storchaka'
dependencies = []
files = ['30116', '37103', '37106']
hgrepos = []
issue_num = 17900
keywords = ['patch']
message_count = 11.0
messages = ['188325', '188344', '189662', '189688', '189737', '230482', '230484', '230487', '267778', '268314', '268347']
nosy_count = 5.0
nosy_names = ['rhettinger', 'pitrou', 'alexandre.vassalotti', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue17900'
versions = ['Python 3.4', 'Python 3.5']
    @serhiy-storchaka
    Copy link
    Member Author

    Here is a patch which allows pickling of recursive OrderedDicts. Actually it simplifies __reduce__() code.

    @serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement stdlib Python modules in the Lib dir labels May 3, 2013
    @serhiy-storchaka
    Copy link
    Member Author

    In additional the patch decreases the size of the pickled data, speeds up pickling and unpickling.

    $ ./python -c "import collections, pickle; od = collections.OrderedDict((i, i) for i in range(100000)); print(len(pickle.dumps(od)))"

    Without patch: 1536827.
    With patch: 737578.

    $ ./python -m timeit -s "from pickle import dumps; import collections; od = collections.OrderedDict((i, i) for i in range(100000))"  "dumps(od)"

    Without patch: 454 msec.
    With patch: 361 msec.

    $ ./python -m timeit -s "from pickle import dumps, loads; import collections; od = collections.OrderedDict((i, i) for i in range(100000)); x = dumps(od)"  "loads(x)"

    Without patch: 1.41 sec.
    With patch: 1.15 sec.

    @serhiy-storchaka
    Copy link
    Member Author

    Ping.

    @avassalotti
    Copy link
    Member

    The patch looks good to me. So go ahead and submit it. :)

    @serhiy-storchaka serhiy-storchaka self-assigned this May 20, 2013
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 21, 2013

    New changeset 56f25569ba86 by Serhiy Storchaka in branch 'default':
    Issue bpo-17900: Allowed pickling of recursive OrderedDicts. Decreased pickled
    http://hg.python.org/cpython/rev/56f25569ba86

    @rhettinger
    Copy link
    Contributor

    This patch caused a regression by breaking designed-in support for pickling using PyYAML (note, there was a test for yaml support, but this patch defeated the test). As a result of the patch, ordering now gets lost during a dump/load roundtrip.

    This change should not have been committed without my review (I would have caught the problem instantly).

    @rhettinger rhettinger reopened this Nov 2, 2014
    @rhettinger rhettinger added type-bug An unexpected behavior, bug, or error and removed type-feature A feature request or enhancement labels Nov 2, 2014
    @pitrou
    Copy link
    Member

    pitrou commented Nov 2, 2014

    Can you explain what is broken independently of PyYAML?

    @serhiy-storchaka
    Copy link
    Member Author

    Oh, sorry, I did not notice test_yaml_linkage. And in any case this test was broken (should be rv[1][0] instead of rv[1]).

    I don't like to revert these changes, and the reversion can break a code written for 3.4 which pickles recursive OrderedDicts. I suggest following solution. PyYAML calls __reduce_ex_(2) or __reduce__(). Therefore we can return to old behavior with protocol < 3 and left current behavior with protocol >= 3. Recursive OrderedDicts will be pickleable with default protocol and with highest protocol, and PyYAML will be happy.

    Does this patch look good to your?

    @serhiy-storchaka
    Copy link
    Member Author

    I think it is too late to revert this change. Is it worth to add a workaround for PyYAML if it still hasn't fixed serialization of OrderedDicts?

    @rhettinger
    Copy link
    Contributor

    At this point, we can drop it. PyYAML will just have to deal with it.

    @rhettinger rhettinger removed their assignment Jun 12, 2016
    @serhiy-storchaka
    Copy link
    Member Author

    Reported PyYAML issue: https://bitbucket.org/xi/pyyaml/issues/61/ordereddict-doesnt-round-trip.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @avassalotti @rhettinger @pitrou @serhiy-storchaka