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

itertools.accumulate __reduce__/__setstate__ bug #69904

Closed
arigo mannequin opened this issue Nov 24, 2015 · 14 comments
Closed

itertools.accumulate __reduce__/__setstate__ bug #69904

arigo mannequin opened this issue Nov 24, 2015 · 14 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@arigo
Copy link
Mannequin

arigo mannequin commented Nov 24, 2015

BPO 25718
Nosy @arigo, @rhettinger, @kristjanvalur, @avassalotti, @serhiy-storchaka
Files
  • copy_state_is_false.patch
  • itertools_accumulate_state_is_none.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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2016-03-06.12:34:39.711>
    created_at = <Date 2015-11-24.08:07:23.506>
    labels = ['type-bug', 'library']
    title = 'itertools.accumulate __reduce__/__setstate__ bug'
    updated_at = <Date 2016-03-06.12:34:39.709>
    user = 'https://github.com/arigo'

    bugs.python.org fields:

    activity = <Date 2016-03-06.12:34:39.709>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2016-03-06.12:34:39.711>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2015-11-24.08:07:23.506>
    creator = 'arigo'
    dependencies = []
    files = ['41151', '41261']
    hgrepos = []
    issue_num = 25718
    keywords = ['patch']
    message_count = 14.0
    messages = ['255252', '255280', '255329', '255336', '255622', '255625', '256058', '256060', '256695', '258577', '260997', '261237', '261242', '261247']
    nosy_count = 6.0
    nosy_names = ['arigo', 'rhettinger', 'kristjan.jonsson', '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/issue25718'
    versions = ['Python 3.5', 'Python 3.6']

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Nov 24, 2015

    itertools.accumulate() has got methods __reduce__() and __setstate__() which fail at saving/restoring the state in all cases. Example:

       >>> a = itertools.accumulate([0.0, 42])
       >>> next(a)
       0.0
       >>> b = copy.deepcopy(a)
       >>> next(a)
       42.0
       >>> next(b)
       42           # should have been the same as the previous result

    More precisely, the problem occurs when the C-level "total" field has some value whose truth-value is false. Then __reduce__/setstate will not restore the value of this field, but keep it NULL. That's why in the example above the intermediate total of 0.0 is forgotten, and the next(b) call will again just pick the next value without doing any addition. (The problem can hurt more than in this example if we use a custom function instead of addition.)

    A fix is easy but, as far as I can tell, it requires breaking the pickled format of accumulate() iterators that have already started.

    @arigo arigo mannequin added extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels Nov 24, 2015
    @serhiy-storchaka
    Copy link
    Member

    __reduce__() and __setstate__() methods of itertools.accumulate() look correct. The problem is not in itertools.accumulate(), but in the copy module. It uses the same __reduce__ protocol as pickle, but in different way. In the pickle module (in both Python and C implementations) state value is ignored only if it is None. In the copy module any state with boolean value is False is ignored.

    Proposed patch fixes the copy module.

    @serhiy-storchaka serhiy-storchaka added stdlib Python modules in the Lib dir and removed extension-modules C modules in the Modules dir labels Nov 24, 2015
    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Nov 25, 2015

    Ok, then with pickle you can have the same problem but only with None-vs-no-total. Here is an artificial example:

    import itertools, pickle
    
    def foo(a, b):
        print(a, b)
    
    a = itertools.accumulate([3, 4, 5], foo)
    next(a)
    next(a)   # prints: 3, 4
    
    b = pickle.loads(pickle.dumps(a))
    
    next(a)   # prints: None, 5
    next(b)   # foo() is not called at all

    @serhiy-storchaka
    Copy link
    Member

    In this case yes, you are unlucky. But this is an artificial example, and I don't think this happens in any real code.

    However specific meaning of None should be documented (there are other wrong words about __getstate__).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 30, 2015

    New changeset 095d21df9374 by Serhiy Storchaka in branch '3.4':
    Issue bpo-25718: Fixed copying object with state with boolean value is false.
    https://hg.python.org/cpython/rev/095d21df9374

    New changeset 3d39e1e2dcb3 by Serhiy Storchaka in branch '2.7':
    Issue bpo-25718: Fixed copying object with state with boolean value is false.
    https://hg.python.org/cpython/rev/3d39e1e2dcb3

    New changeset fecb07050aae by Serhiy Storchaka in branch '3.5':
    Issue bpo-25718: Fixed copying object with state with boolean value is false.
    https://hg.python.org/cpython/rev/fecb07050aae

    New changeset 962086677306 by Serhiy Storchaka in branch 'default':
    Issue bpo-25718: Fixed copying object with state with boolean value is false.
    https://hg.python.org/cpython/rev/962086677306

    @serhiy-storchaka
    Copy link
    Member

    The bug in the copy module is fixed. As for accumulate's __reduce__, I don't think we can do something.

    @kristjanvalur
    Copy link
    Mannequin

    kristjanvalur mannequin commented Dec 7, 2015

    This could be fixed by saving the accumulate state in a tuple. It would break protocol, though. I don't recall the rules for backwards compatibility of pickles.
    I've argued before that the state of runtime structures such as generators is so intimately tied with the currently executing python that we shouldn't worry to much about it. This sort of stuff is used for caches, IPC, and so on. But it's not my call.

    @serhiy-storchaka
    Copy link
    Member

    Well, we can fix this without breaking the protocol. The accumulate() object with state is None can be reconstructed as islice(accumulate(chain([None], iterator)), 1, None).

    Proposed patch implements this solution.

    @serhiy-storchaka
    Copy link
    Member

    What would you say about the patch Kristján?

    @serhiy-storchaka
    Copy link
    Member

    Ping.

    @serhiy-storchaka
    Copy link
    Member

    Ping again.

    @serhiy-storchaka
    Copy link
    Member

    Raymond, could you please look at the patch?

    @rhettinger
    Copy link
    Contributor

    +1

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 6, 2016

    New changeset f3c54cbac3de by Serhiy Storchaka in branch '3.5':
    Issue bpo-25718: Fixed pickling and copying the accumulate() iterator with total is None.
    https://hg.python.org/cpython/rev/f3c54cbac3de

    New changeset 49c035b30e18 by Serhiy Storchaka in branch 'default':
    Issue bpo-25718: Fixed pickling and copying the accumulate() iterator with total is None.
    https://hg.python.org/cpython/rev/49c035b30e18

    @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
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants