classification
Title: Cached method implementation no longer works on Python 3.7.3
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.9, Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: jaraco, josh.r, mcepl, rhettinger, serhiy.storchaka
Priority: normal Keywords: 3.7regression, patch

Created on 2019-04-17 18:45 by jaraco, last changed 2019-04-20 22:42 by mcepl. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 12881 merged rhettinger, 2019-04-20 01:34
PR 12888 merged miss-islington, 2019-04-20 17:21
Messages (10)
msg340429 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2019-04-17 18:45
In [this ticket](https://github.com/jaraco/jaraco.functools/issues/12), I learned that [jaraco.functools.method_cache](https://github.com/jaraco/jaraco.functools/blob/6b32ee0dfd3e7c88f99e88cd87c35fa9b76f261f/jaraco/functools.py#L109-L180) no longer works on Python 3.7.3.

A distilled version of what's not working is this example:

```
>>> import jaraco.functools
>>> class MyClass:
...   calls = 0
...   @jaraco.functools.method_cache
...   def call_me_maybe(self, val):
...     self.calls += 1
...     return val
... 
>>> a = MyClass()
>>> a.call_me_maybe(0)
0
>>> a.call_me_maybe(0)
0
>>> a.calls
2
```

The second call to the cached function is missing the cache even though the parameters to the function are the same.


```
>>> a.call_me_maybe
<functools._lru_cache_wrapper object at 0x107eb2df0>
>>> a.call_me_maybe.cache_info()
CacheInfo(hits=0, misses=2, maxsize=128, currsize=2)
```

Here's a further distilled example not relying on any code from jaraco.functools:

```
>>> def method_cache(method):
...     def wrapper(self, *args, **kwargs):
...             # it's the first call, replace the method with a cached, bound method
...             bound_method = functools.partial(method, self)
...             cached_method = functools.lru_cache()(bound_method)
...             setattr(self, method.__name__, cached_method)
...             return cached_method(*args, **kwargs)
...     return wrapper
... 
>>> import functools
>>> class MyClass:
...   calls = 0
...   @method_cache
...   def call_me_maybe(self, val):
...     self.calls += 1
...     return val
... 
>>> a = MyClass()
>>> a.call_me_maybe(0)
0
>>> a.call_me_maybe(0)
0
>>> a.calls
2
```

I was not able to replicate the issue with a simple lru_cache on a partial object:

```
>>> def func(a, b):
...   global calls
...   calls += 1
... 
>>> import functools
>>> cached = functools.lru_cache()(functools.partial(func, 'a'))
>>> calls = 0
>>> cached(0)
>>> cached(0)
>>> calls
1
```

Suggesting that there's some interaction with the instance attribute and the caching functionality.

I suspect the issue arose as a result of changes in issue35780.
msg340433 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2019-04-17 18:52
I've put together this full reproducer script:

```
import functools


def method_cache(method):
	def wrapper(self, *args, **kwargs):
		# it's the first call, replace the method with a cached, bound method
		bound_method = functools.partial(method, self)
		cached_method = functools.lru_cache()(bound_method)
		setattr(self, method.__name__, cached_method)
		return cached_method(*args, **kwargs)
	return wrapper


class MyClass:
	calls = 0

	@method_cache
	def call_me_maybe(self, number):
		self.calls += 1
		return number


ob = MyClass()
ob.call_me_maybe(0)
ob.call_me_maybe(0)
assert ob.calls == 1
```

That code fails on Python 3.7.3. If I bypass the `from _functools import _lru_cache_wrapper` in functools, the test no longer fails, so the issue seems to be only with the C implementation.
msg340438 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-04-17 19:28
It seems highly likely this is related to #26110 which optimizes method calls by avoiding the creation of bound methods in certain circumstances, and/or the follow-up #29263.

Side-note:

bound_method = functools.partial(method, self)

is not how you create real bound methods. The correct approach (that makes a bound method identical to what the interpreter makes when necessary) for your use case is:

bound_method = types.MethodType(method, self)
msg340439 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-04-17 19:34
Hmm... Although I can't seem to reproduce on 3.7.2 (ob.calls is 1), so assuming it is really happening in 3.7.3, it wouldn't be either of those issues (which were fully in place long before 3.7.2 I believe).
msg340440 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2019-04-17 19:37
Hi Josh. Thanks for the tip on types.MethodType. I've updated the code to use that and the behavior seems to be the same, MethodType does seem a more appropriate way to create a bound method.

Regarding the referenced tickets, I suspect they're not pertinent, as the behavior did work in Python 3.7.1 (confirmed) and 3.7.2 (highly likely), so the regression appears to be in the changes for 3.7.3 (https://docs.python.org/3.7/whatsnew/changelog.html#python-3-7-3-final).
msg340554 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-04-19 18:22
Thanks for the reproducer code.  I've bisected this back to b2b023c657ba8c3f4a24d0c847d10fe8e2a73d44 which fixes other known bugs in the lru_cache in issue 35780.  The problem is due to the lines that use a scalar instead of an args tuple for exact ints and strs.  I'll work-up a PR to fix it soon (I'm on vacation and have limited connectivity so it may take a few days).
msg340555 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2019-04-19 18:34
Nice work. Thanks Raymond.
msg340560 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-04-20 00:55
For the record, here's what is going on.  The method_cache() code uses a slightly different invocation for the first call than for subsequent calls. In particular, the wrapper() uses **kwargs with an empty dict whereas the first call didn't use keyword arguments at all.   The C version of the lru_cache() is treating that first call as distinct from the second call, resulting in a cache miss for the both the first and second invocation but not in subsequent invocations.

The pure python lru_cache() has a memory saving fast path taken when kwds is an empty dict.  The C version is out-of-sync with because it runs the that path only when kwds==NULL and it doesn't check for the case where kwds is an empty dict.  Here's a minimal reproducer:

    @lru_cache()
    def f(x):
        pass

    f(0)
    f(0, **{})
    assert f.cache_info().hits == 1

Here's a possible fix:

diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c
index 3f1c01651d..f118119479 100644
--- a/Modules/_functoolsmodule.c
+++ b/Modules/_functoolsmodule.c
@@ -751,7 +751,7 @@ lru_cache_make_key(PyObject *args, PyObject *kwds, int typed)
     Py_ssize_t key_size, pos, key_pos, kwds_size;

     /* short path, key will match args anyway, which is a tuple */
-    if (!typed && !kwds) {
+    if (!typed && (!kwds || PyDict_GET_SIZE(kwds) == 0)) {
         if (PyTuple_GET_SIZE(args) == 1) {
             key = PyTuple_GET_ITEM(args, 0);
             if (PyUnicode_CheckExact(key) || PyLong_CheckExact(key)) {
msg340586 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-04-20 17:20
New changeset 14adbd45980f705cb6554ca17b8a66b56e105296 by Raymond Hettinger in branch 'master':
bpo-36650: Fix handling of empty keyword args in C version of lru_cache. (GH-12881)
https://github.com/python/cpython/commit/14adbd45980f705cb6554ca17b8a66b56e105296
msg340587 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-04-20 17:50
New changeset 8b30ee843528d0f0e2bfc3307d86658915579c21 by Raymond Hettinger (Miss Islington (bot)) in branch '3.7':
bpo-36650: Fix handling of empty keyword args in C version of lru_cache. (GH-12881) (GH-12888)
https://github.com/python/cpython/commit/8b30ee843528d0f0e2bfc3307d86658915579c21
History
Date User Action Args
2019-04-20 22:42:57mceplsetnosy: + mcepl
2019-04-20 17:51:10rhettingersetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-04-20 17:50:35rhettingersetmessages: + msg340587
2019-04-20 17:21:08miss-islingtonsetpull_requests: + pull_request12814
2019-04-20 17:20:48rhettingersetmessages: + msg340586
2019-04-20 01:34:47rhettingersetkeywords: + patch
stage: patch review
pull_requests: + pull_request12806
2019-04-20 00:55:12rhettingersetmessages: + msg340560
2019-04-19 18:34:38jaracosetmessages: + msg340555
2019-04-19 18:22:04rhettingersetmessages: + msg340554
2019-04-17 20:19:49serhiy.storchakasetnosy: + serhiy.storchaka
2019-04-17 19:37:37jaracosetmessages: + msg340440
2019-04-17 19:34:30josh.rsetmessages: + msg340439
2019-04-17 19:28:40josh.rsetnosy: + josh.r
messages: + msg340438
2019-04-17 18:52:55jaracosetmessages: + msg340433
2019-04-17 18:47:35jaracosettype: behavior
components: + Library (Lib)
versions: + Python 3.7, Python 3.8, Python 3.9
2019-04-17 18:45:43jaracocreate