[BUG] `cachedmethod` is not instance-specific, leading to incorrect results

[liblaf]

Hello,

I believe I've found a bug in how @cachedmethod handles different class instances. The cache appears to be shared across all instances of a class, which leads to incorrect results when the method's output depends on instance attributes.

Description

When a method decorated with @cachedmethod is called on a second instance of a class with the same arguments as a previous call on a different instance, it incorrectly returns the cached result from the first instance.

This happens because the cache key for cachedmethod does not seem to factor in the specific object instance (self). According to the documentation, this is the intended behavior as it "ignores self parameters in hashing and key making." However, this can lead to unexpected and incorrect behavior, as demonstrated below.

Minimal Reproducible Example

import cachebox

class A:
    a: int

    def __init__(self, a: int = 1) -> None:
        self.a = a

    @cachebox.cachedmethod(None)
    def method(self, b: int) -> int:
        return self.a + b

# First instance
a1 = A(a=1)
print(f"a1.method(1): {a1.method(1)}")

# Second instance with different internal state
a2 = A(a=2)
print(f"a2.method(1): {a2.method(1)}")

Actual Behavior

a1.method(1): 2
a2.method(1): 2

Expected Behavior

a1.method(1): 2
a2.method(1): 3

The second call, a2.method(1), should have returned 3 (since a2.a is 2), but it returned 2, which was the cached result from the a1 instance.

This makes cachedmethod unsuitable for most instance methods, as their behavior is typically tied to the instance's state. I suggest that the cache should be instance-specific by default.

Thank you for your work on this library

[awolverp]

Hi, thank you ❤️

So I have to deprecate the cachedmethod decorator, since its only difference is what's causing the bug. Do you have any other ideas?

[liblaf]

@awolverp Perhaps we can refer to the design of cachetools. @cachetools.cachedmethod requires user to specify a function that returns a per-instance cache, which is usually a member of the instance and is created when the instance is initialized. We can also allow the user to pass a function like that, or, alternatively, a variable name str.

For example, instead of @cachebox.cachedmethod(cachebox.FIFOCache(128)), we can use @cachebox.cachedmethod("_cache") and create self._cache in __init__.

Another concern is that current implementation of is_cached() won't work for cachedmethod with a per-instance cache.

[liblaf]

@awolverp In fact, @cachetools.cachemethod can accept a cachebox cache:

from collections.abc import Hashable

import cachebox
import cachetools


class A:
    a: int
    _cache: cachebox.BaseCacheImpl[Hashable, int]

    def __init__(self, a: int) -> None:
        self.a = a
        self._cache = cachebox.FIFOCache(128)

    @cachetools.cachedmethod(lambda self: self._cache)
    def method(self, b: int) -> int:
        return self.a + b


a1 = A(1)
assert a1.method(1) == 2  # success
a2 = A(2)
assert a2.method(1) == 3  # success

[awolverp]

Great 🔥 , that was a nice idea. If I want to apply this change, I should first deprecate the cachedmethod and create a method with a different name for this. This way, we prevent incompatible changes.

I will do it ASAP

[awolverp]

I've deprecated the cachedmethod function. The cached function should be used instead. This solution avoids incompatible changes while providing a simpler interface: "a single function for all use cases".

Thanks you a lot.

Example:

from collections.abc import Hashable
import cachebox


class A:
    a: int
    _cache: cachebox.BaseCacheImpl[Hashable, int]

    def __init__(self, a: int) -> None:
        self.a = a
        self._cache = cachebox.FIFOCache(128)

    @cachebox.cached(lambda self: self._cache)
    def method(self, b: int) -> int:
        return self.a + b


a1 = A(1)
assert a1.method(1) == 2  # success
a2 = A(2)
assert a2.method(1) == 3  # success