Cached method implementation no longer works on Python 3.7.3

[jaraco]

BPO	36650
Nosy	@rhettinger, @jaraco, @mcepl, @serhiy-storchaka, @MojoVampire
PRs	bpo-36650: Fix handling of empty keyword args in C version of lru_cache. #12881 [3.7] bpo-36650: Fix handling of empty keyword args in C version of lru_cache. (GH-12881) #12888

^{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/rhettinger'
closed_at = <Date 2019-04-20.17:51:10.064>
created_at = <Date 2019-04-17.18:45:43.778>
labels = ['3.7', '3.8', 'type-bug', 'library', '3.9']
title = 'Cached method implementation no longer works on Python 3.7.3'
updated_at = <Date 2019-04-20.22:42:57.288>
user = 'https://github.com/jaraco'

bugs.python.org fields:

activity = <Date 2019-04-20.22:42:57.288>
actor = 'mcepl'
assignee = 'rhettinger'
closed = True
closed_date = <Date 2019-04-20.17:51:10.064>
closer = 'rhettinger'
components = ['Library (Lib)']
creation = <Date 2019-04-17.18:45:43.778>
creator = 'jaraco'
dependencies = []
files = []
hgrepos = []
issue_num = 36650
keywords = ['patch', '3.7regression']
message_count = 10.0
messages = ['340429', '340433', '340438', '340439', '340440', '340554', '340555', '340560', '340586', '340587']
nosy_count = 5.0
nosy_names = ['rhettinger', 'jaraco', 'mcepl', 'serhiy.storchaka', 'josh.r']
pr_nums = ['12881', '12888']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue36650'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

[jaraco]

In this ticket, I learned that jaraco.functools.method_cache 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 bpo-35780.

[jaraco]

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.

[MojoVampire]

It seems highly likely this is related to bpo-26110 which optimizes method calls by avoiding the creation of bound methods in certain circumstances, and/or the follow-up bpo-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)

[MojoVampire]

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).

[jaraco]

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).

[rhettinger]

Thanks for the reproducer code. I've bisected this back to b2b023c which fixes other known bugs in the lru_cache in bpo-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).

[jaraco]

Nice work. Thanks Raymond.

[rhettinger]

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)) {

[rhettinger]

New changeset 14adbd4 by Raymond Hettinger in branch 'master':
bpo-36650: Fix handling of empty keyword args in C version of lru_cache. (GH-12881)
14adbd4

[rhettinger]

New changeset 8b30ee8 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)
8b30ee8