| msg253233 - (view) |
Author: Jason R. Coombs (jaraco) *  |
Date: 2015-10-20 16:32 |
| 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 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 . I realize one's first reaction might be "why are you deep copying a function," so here's the [downstream bug](https://bitbucket.org/jaraco/jaraco.functools/issues/1/method_cache-causes-typeerror-when) and [offended code](https://bitbucket.org/jaraco/jaraco.functools/src/68d1fda21af7e7b4c36577f953f382270bdc1e05/jaraco/functools.py?at=default&fileviewer=file-view-default#functools.py-72:138). 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. |
|
|
| msg253234 - (view) |
Author: Jason R. Coombs (jaraco) * |
Date: 2015-10-20 16:35 |
| For some context into the error: $ python3.5 -m pdb cachecopy.py > /home/jaraco/cachecopy.py(1)() -> 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 "", line 1, in File "/home/jaraco/cachecopy.py", line 1, in 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 = () |
|
|
| msg253239 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-10-20 18:08 |
| 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? |
|
|
| msg253255 - (view) |
Author: Jason R. Coombs (jaraco) * |
Date: 2015-10-20 21:14 |
| 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. |
|
|
| msg253366 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2015-10-23 05:18 |
| > 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. |
|
|
| msg253367 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-10-23 08:05 |
| 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. |
|
|
| msg253373 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-10-23 09:50 |
| Added test cases for nested functions (__qualname__ != __name__). |
|
|
| msg253388 - (view) |
Author: Jason R. Coombs (jaraco) * |
Date: 2015-10-23 16:29 |
| Simple and elegant. I like it. |
|
|
| msg253389 - (view) |
Author: Jason R. Coombs (jaraco) * |
Date: 2015-10-23 16:34 |
| 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? |
|
|
| msg253390 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-10-23 17:01 |
| It is redundant. update_wrapper() is called just after calling _lru_cache_wrapper() in decorating_function(). |
|
|
| msg253397 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2015-10-24 06:52 |
| New changeset 73226f0d0f89 by Serhiy Storchaka in branch '3.5': Issue #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 #25447: The lru_cache() wrapper objects now can be copied and pickled https://hg.python.org/cpython/rev/d3e048c7b65a |
|
|
| msg253398 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-10-24 06:55 |
| Thank you for your report and review Jason. |
|
|
| msg257084 - (view) |
Author: Jason R. Coombs (jaraco) * |
Date: 2015-12-27 17:09 |
| 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](https://github.com/jaraco/jaraco.functools/issues/1) and the [offended code](https://github.com/jaraco/jaraco.functools/blob/56d1daf4b88239a137f03da87d312f2522df1078/jaraco/functools.py#L79-L145). I've since realized, thanks to the test suite on jaraco.functools, a [follow-on issue](https://github.com/jaraco/jaraco.functools/issues/3) 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 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. |
|
|
| msg257086 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-12-27 18:19 |
| 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. |
|
|
| msg257103 - (view) |
Author: Jason R. Coombs (jaraco) * |
Date: 2015-12-28 00:16 |
| Wow. That's great. I'll take a look soon... and verify it works with the downstream lib. |
|
|
| msg257104 - (view) |
Author: Jason R. Coombs (jaraco) * |
Date: 2015-12-28 01:37 |
| 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. |
|
|
| msg257132 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2015-12-28 22:01 |
| New changeset 33b428ef34b9 by Serhiy Storchaka in branch '3.5': Issue #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 #25447: Copying the lru_cache() wrapper object now always works, https://hg.python.org/cpython/rev/a0f9a8bee85f |
|
|