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

TypeError invoking deepcopy on lru_cache #69633

Closed
jaraco opened this issue Oct 20, 2015 · 17 comments
Closed

TypeError invoking deepcopy on lru_cache #69633

jaraco opened this issue Oct 20, 2015 · 17 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@jaraco
Copy link
Member

jaraco commented Oct 20, 2015

BPO 25447
Nosy @rhettinger, @jaraco, @serhiy-storchaka
Files
  • lru_cache_simple_pickling.patch
  • lru_cache_simple_pickling_2.patch
  • lru_cache_simple_copy.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 2015-12-28.22:03:29.301>
    created_at = <Date 2015-10-20.16:32:09.067>
    labels = ['type-bug', 'library']
    title = 'TypeError invoking deepcopy on lru_cache'
    updated_at = <Date 2015-12-28.22:03:29.300>
    user = 'https://github.com/jaraco'

    bugs.python.org fields:

    activity = <Date 2015-12-28.22:03:29.300>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2015-12-28.22:03:29.301>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2015-10-20.16:32:09.067>
    creator = 'jaraco'
    dependencies = []
    files = ['40845', '40848', '41435']
    hgrepos = []
    issue_num = 25447
    keywords = ['patch', '3.5regression']
    message_count = 17.0
    messages = ['253233', '253234', '253239', '253255', '253366', '253367', '253373', '253388', '253389', '253390', '253397', '253398', '257084', '257086', '257103', '257104', '257132']
    nosy_count = 4.0
    nosy_names = ['rhettinger', 'jaraco', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue25447'
    versions = ['Python 3.5', 'Python 3.6']

    @jaraco
    Copy link
    Member Author

    jaraco commented Oct 20, 2015

    Beginning with Python 3.5, an lru_cache no longer survives a deepcopy.

    $ cat >> cache_copy.py
    import copy
    from functools import lru_cache
    foo = lru_cache()(lambda: None)
    copy.deepcopy(foo)
    $ python3.5 cache_copy.py
    Traceback (most recent call last):
      File "cache_copy.py", line 4, in <module>
        copy.deepcopy(foo)
      File "/usr/lib/python3.5/copy.py", line 182, in deepcopy
        y = _reconstruct(x, rv, 1, memo)
      File "/usr/lib/python3.5/copy.py", line 293, in _reconstruct
        y = callable(*args)
      File "/usr/lib/python3.5/copyreg.py", line 88, in __newobj__
        return cls.__new__(cls, *args)
    TypeError: Required argument 'user_function' (pos 1) not found
    

    I suspect this is due to the C implementation per bpo-14373.

    I realize one's first reaction might be "why are you deep copying a function," so here's the downstream bug and offended code. That decorator is designed to wrap a method and to store the cache on the instance object, an object which is liable to be deep copied. As a result, production code using this technique is failing on Python 3.5.

    Although it was apparently an artifact of previous implementations that the cache happened to be deep-copyable, I hope this expectation can be restored with minimal fuss.

    @jaraco jaraco added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Oct 20, 2015
    @jaraco
    Copy link
    Member Author

    jaraco commented Oct 20, 2015

    For some context into the error:

    $ python3.5 -m pdb cachecopy.py 
    > /home/jaraco/cachecopy.py(1)<module>()
    -> import copy
    (Pdb) c
    Traceback (most recent call last):
      File "/usr/lib/python3.5/pdb.py", line 1661, in main
        pdb._runscript(mainpyfile)
      File "/usr/lib/python3.5/pdb.py", line 1542, in _runscript
        self.run(statement)
      File "/usr/lib/python3.5/bdb.py", line 431, in run
        exec(cmd, globals, locals)
      File "<string>", line 1, in <module>
      File "/home/jaraco/cachecopy.py", line 1, in <module>
        import copy
      File "/usr/lib/python3.5/copy.py", line 182, in deepcopy
        y = _reconstruct(x, rv, 1, memo)
      File "/usr/lib/python3.5/copy.py", line 293, in _reconstruct
        y = callable(*args)
      File "/usr/lib/python3.5/copyreg.py", line 88, in __newobj__
        return cls.__new__(cls, *args)
    TypeError: Required argument 'user_function' (pos 1) not found
    Uncaught exception. Entering post mortem debugging
    Running 'cont' or 'step' will restart the program
    > /usr/lib/python3.5/copyreg.py(88)__newobj__()
    -> return cls.__new__(cls, *args)
    (Pdb) cls
    <class 'functools._lru_cache_wrapper'>
    (Pdb) args
    cls = <class 'functools._lru_cache_wrapper'>
    args = ()

    @serhiy-storchaka
    Copy link
    Member

    I'm not sure that we can say that deepcopying was not supported for lru_cache. copy.deepcopy() returned the same object, as for immutable objects, but can we consider decorated with lru_cache() function immutable?

    The question is: if made functools._lru_cache_wrapper to support deepcopy protocol, should the result of copy.deepcopy() share the cache with original?

    @jaraco
    Copy link
    Member Author

    jaraco commented Oct 20, 2015

    Indeed. I guess my point about "supported" related to not having tests capturing this requirement or docs stipulating it.

    My instinct on the latter question is this - an lru_cache-decorated function is a stateful function. It is mutable, much like a dict or list. If simply copied, the resulting function should have references to the same state. If _deep_ copied, the state (cache) should be similarly deep copied.

    Focusing on the deep copy operation, if a cached function is copied, the copy could have additional operations invoked on it and its cache would hit or miss on those calls independently from the original function, and likewise subsequent calls on the original function should not hit on calls unique to the copied function.

    I don't have a heavy investment in this expectation. It's also reasonable to me that a deepcopy operation could reuse the same cache or result in an uninitialized one.

    @rhettinger
    Copy link
    Contributor

    Although it was apparently an artifact of previous
    implementations that the cache happened to be deep-copyable,
    I hope this expectation can be restored with minimal fuss.

    Hmm, I'm surprised that a deepcopy ever worked. The LRU cache certainly wasn't designed for that.

    If Serhiy has a straight-forward way of restoring the behavior (and adding a test for it), that would be reasonable for 3.5.1. However, if it involves twisting the code into knots or risking breakage, it would be better to either close this as "won't fix" or reclassify as a feature request for 3.6.

    @serhiy-storchaka
    Copy link
    Member

    Using lru_cache() is an implementation detail. I think that lru_cache wrapper should be perfect wrapper, it should support all protocols that are supported by wrapped function (at the same extent as original Python implementation).

    Proposed simple patch adds support for pickling, copying and deep-copying lru_cache wrappers. lru_cache wrapper is serialized as global object, i.e. deepcopy() returns the same object, as was in 3.4. Implementing true deep copying (with any semantic) is not so easy, especially for Python implementation.

    @serhiy-storchaka
    Copy link
    Member

    Added test cases for nested functions (qualname != __name__).

    @jaraco
    Copy link
    Member Author

    jaraco commented Oct 23, 2015

    Simple and elegant. I like it.

    @jaraco
    Copy link
    Member Author

    jaraco commented Oct 23, 2015

    Can you speak to why 'update_wrapper' should be removed? Is that indicative of a deeper issue with update_wrapper also not supporting pickle/deepcopy?

    @serhiy-storchaka
    Copy link
    Member

    It is redundant. update_wrapper() is called just after calling _lru_cache_wrapper() in decorating_function().

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 24, 2015

    New changeset 73226f0d0f89 by Serhiy Storchaka in branch '3.5':
    Issue bpo-25447: The lru_cache() wrapper objects now can be copied and pickled
    https://hg.python.org/cpython/rev/73226f0d0f89

    New changeset d3e048c7b65a by Serhiy Storchaka in branch 'default':
    Issue bpo-25447: The lru_cache() wrapper objects now can be copied and pickled
    https://hg.python.org/cpython/rev/d3e048c7b65a

    @serhiy-storchaka
    Copy link
    Member

    Thank you for your report and review Jason.

    @jaraco
    Copy link
    Member Author

    jaraco commented Dec 27, 2015

    I've since moved the jaraco.functools project to Github, so here are updated links for those found in the original post to the downstream bug and the offended code.

    I've since realized, thanks to the test suite on jaraco.functools, a follow-on issue where it seems that if the deepcopy occurs on a wrapped partial, it will still fail, because the lru cache wrapper has no qualname:

    $ cat > cache_copy.py
    import copy
    from functools import lru_cache, partial
    foo = lru_cache()(partial(print, 'out'))
    copy.deepcopy(foo)
    $ python3.5 cache_copy.py
    Traceback (most recent call last):
      File "cache_copy.py", line 5, in <module>
        copy.deepcopy(foo)
      File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/copy.py", line 174, in deepcopy
        rv = reductor(4)
    AttributeError: 'functools._lru_cache_wrapper' object has no attribute '__qualname__'

    I realize now that when I reviewed the patch, I did not take the time to test it against my full use case, but only relied on the tests as committed, which did not cover this more nuanced usage.

    On further consideration, this error highlights a potential subtle difference in the reduce support between the C implementation and the Python implementation.

    In digging into the implementation, I don't yet fully understand how it is that the Python implementation survives a deepcopy, but I'd like to revisit this issue to see if it may be possible to make it so.

    @jaraco jaraco reopened this Dec 27, 2015
    @serhiy-storchaka
    Copy link
    Member

    The Python implementation survives a deepcopy because the Python implementation has a function type, and functions functions are copied as atoms.

    Proposed patch just implements __copy__ and __deepcopy__ methods that returns the object itself for the lru_cache object.

    @jaraco
    Copy link
    Member Author

    jaraco commented Dec 28, 2015

    Wow. That's great. I'll take a look soon... and verify it works with the downstream lib.

    @jaraco
    Copy link
    Member Author

    jaraco commented Dec 28, 2015

    I've confirmed this change corrects the failing test and expectation in the downstream project. The tests look good to me as far as covering this more nuanced expectation. Well done, and huge thanks.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 28, 2015

    New changeset 33b428ef34b9 by Serhiy Storchaka in branch '3.5':
    Issue bpo-25447: Copying the lru_cache() wrapper object now always works,
    https://hg.python.org/cpython/rev/33b428ef34b9

    New changeset a0f9a8bee85f by Serhiy Storchaka in branch 'default':
    Issue bpo-25447: Copying the lru_cache() wrapper object now always works,
    https://hg.python.org/cpython/rev/a0f9a8bee85f

    @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

    3 participants