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

doctest for method_cache fails on Python 3.7.3 #12

Closed
mcepl opened this issue Apr 17, 2019 · 6 comments
Closed

doctest for method_cache fails on Python 3.7.3 #12

mcepl opened this issue Apr 17, 2019 · 6 comments

Comments

@mcepl
Copy link

@mcepl mcepl commented Apr 17, 2019

Looking at:

>>> class MyClass:
... calls = 0
...
... @method_cache
... def method(self, value):
... self.calls += 1
... return value
>>> a = MyClass()
>>> a.method(3)
3
>>> for x in range(75):
... res = a.method(x)
>>> a.calls
75

I just cannot wrap my head around it. Given the method method is called once on line 126 and 75-times on line 129, I would think that a.calls on line 131 should return 76. And it does (on python 3.7.3). However, it returns just 75 for python 2.7.16. But the craziness goes even further, with the following text:
>>> b = MyClass()
>>> for x in range(35):
... res = b.method(x)
>>> b.calls
35
>>> a.method(0)
0
>>> a.calls
75

the very same a.method is called once more on line 143, so a.calls on line 145 should return 77, but you expect 75. And I have to say, the explanation on lines 148-150 doesn't help much. If the value of property calls was overridden by b, then the call on line 145 should return 36 (b.calls+1), right?

@jaraco

This comment has been minimized.

Copy link
Owner

@jaraco jaraco commented Apr 17, 2019

I would think that a.calls on line 131 should return 76.

It should only return 75 because a.method(3) and a.method(x) when x == 3 are the same call so the second one is cached. Just like with functools.lru_cache, the parameters are used as keys into the cache of results.

And it does [return 76] (on python 3.7.3).

Oh, you're right. That's a bug. The tests passed on Python 3.7.1 (and I'm pretty sure 3.7.2 also). It seems something changed between Python 3.7.2 and 3.7.3 that breaks this implementation.

And I have to say, the explanation on lines 148-150 doesn't help much.

What this is trying to say is that method_cache is seeking to create a per-instance cache... but a simple wrapper of functools.lru_cache, which has a default max-size of 100, would cache values across all instances (on the class method itself). So if you'd done this:

class MyClass:
    calls = 0
    @functools.lru_cache(max_size=100)
    def method(self, value):
        self.calls += 1
        return value

Then, when you do

a = MyClass()
for x in range(75):
    a.method(x)

The cache found on MyClass.method has entries for calls that looks something like:

{
    (a, 0): 0,
    (a, 1): 1,
    ...
    (a, 74): 74,
}

If you then instantiate b and loop over it 35 times, here's what happens:

The first 25 calls add to the cache on MyClass.method so it looks like this:

{
    (a, 0): 0,
    (a, 1): 1,
    ...
    (a, 74): 74,
    (b, 0): 0,
    ...
    (b, 24): 24,
}

But now the cache is full. It has 100 items in it. So calling b.method(25) causes the least recently used item to be flushed from the cache, so it looks like this:

{
    (a, 1): 1,
    ...
    (a, 74): 74,
    (b, 0): 0,
    ...
    (b, 24): 24,
    (b, 25): 25,
}

There's no longer an entry for (a, 0), so the next time a.method(0) is called, it will miss the cache, and it will add to the .calls attribute.

If the value of property calls was overridden by b, ...

It's not that calls gets overridden by b. Both a and b have their own .calls instance variable (attribute)... but they share a cache on their method.

@jaraco

This comment has been minimized.

Copy link
Owner

@jaraco jaraco commented Apr 17, 2019

I see the following entry in the Python 3.7.3 news:

Fix lru_cache() errors arising in recursive, reentrant, or multi-threaded code. These errors could result in orphan links and in the cache being trapped in a state with fewer than the specified maximum number of links. Fix handling of negative maxsize which should have been treated as zero. Fix errors in toggling the "full" status flag. Fix misordering of links when errors are encountered. Sync-up the C code and pure Python code for the space saving path in functions with a single positional argument. In this common case, the space overhead of an lru cache entry is reduced by almost half. Fix counting of cache misses. In error cases, the miss count was out of sync with the actual number of times the underlying user function was called.

Those changes (traced to here) are likely implicated.

@mcepl

This comment has been minimized.

Copy link
Author

@mcepl mcepl commented Apr 17, 2019

It should only return 75 because a.method(3) and a.method(x) when x == 3 are the same call so the second one is cached. Just like with functools.lru_cache, the parameters are used as keys into the cache of results.

Thanks, that is my mistake obviously. I kind of forgot that this package actually does something useful ;).

@jaraco jaraco changed the title doctest for method_cache doesn't make any sense to me doctest for method_cache fails on Python 3.7.3 Apr 17, 2019
@jaraco

This comment has been minimized.

Copy link
Owner

@jaraco jaraco commented Apr 17, 2019

I've filed bpo-36650 to track the regression upstream.

@jaraco

This comment has been minimized.

Copy link
Owner

@jaraco jaraco commented Apr 20, 2019

My first instinct was that this was a serious flaw with serious performance consequences for this library (cache not having an effect). After Raymond's analysis, I see now it only causes a single cache miss on the first call (an artifact of the cache being created). The long-term performance should be unaffected. Therefore, the only remedy needed here is to bypass the test failure.

@jaraco jaraco closed this in cc97209 Apr 20, 2019
jaraco added a commit that referenced this issue Apr 20, 2019
@jaraco

This comment has been minimized.

Copy link
Owner

@jaraco jaraco commented Apr 20, 2019

I considered two possible fixes for this issue, one in da55ea3 and another in cc97209. I opted for the latter because it's less intrusive on the documentation, even though it's a lot more code for the codebase. I intend to keep that workaround in place until Python 3.7.3 is unlikely to be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.