Fix property_cache_once_per_frame

[MythicManiac]

Issue

Currently, running multiple bots at the same time within a single process will sometimes result in either of the bot having their own units within the self.known_enemy_units or self.known_enemy_structures variables.

Cause

Current implementation of the "property_cache_once_per_frame" decorator doesn't work with multiple instances of the same class. This is due to the cache being stored to the function itself, rather than to the individual object it's being called on.

Address this issue by adding a parameter to the decorator, which can be used to define where the cache should be stored on the object, and store the cache there.

This allows multiple bots to co-exist in a single process without conflicting caches.

Demonstration

from functools import wraps

def property_cache_not_working(f):
    f.cache = None

    @wraps(f)
    def inner(self):
        if f.cache is None:
            f.cache = f(self)
        return f.cache

    return property(inner)

def property_cache_working(cache_target):
    def cacher(f):
        @wraps(f)
        def inner(self):
            if getattr(self, cache_target, None) is None:
                setattr(self, cache_target, f(self))
            return getattr(self, cache_target)
        return property(inner)
    return cacher

class Base(object):

    @property_cache_working("_cache_target")
    def working_cache(self):
        return self.get_value()

    @property_cache_not_working
    def broken_cache(self):
        return self.get_value()

    def get_value(self):
        raise NotImplementedError

class SubclassA(Base):

    def get_value(self):
        return 1

class SubclassB(Base):

    def get_value(self):
        return 2

def main():
    instanceA = SubclassA()
    instanceB = SubclassB()
    print("Broken cache result:")
    print(f"instanceA: {instanceA.broken_cache}")
    print(f"instanceB: {instanceB.broken_cache}")
    print("Working cache result:")
    print(f"instanceA: {instanceA.working_cache}")
    print(f"instanceB: {instanceB.working_cache}")
    print("Contents of _cache_target")
    print(f"instanceA: {instanceA._cache_target}")
    print(f"instanceB: {instanceB._cache_target}")

if __name__ == "__main__":
    main()

This script yields the following output:

Broken cache result:
instanceA: 1
instanceB: 1
Working cache result:
instanceA: 1
instanceB: 2
Contents of _cache_target
instanceA: 1
instanceB: 2

[BurnySc2]

Thanks for the find and detailled explanation!

Would the following also work?

def property_cache_once_per_frame(f):
    """ This decorator caches the return value for one game loop, then clears it if it is accessed in a different game loop
    Only works on properties of the bot object because it requires access to self.state.game_loop """

    @wraps(f)
    def inner(self):
        cache_name = "_cache_" + f.__name__
        frame_name = "_frame_" + f.__name__
        if not hasattr(self, cache_name) or not hasattr(self, frame_name) or getattr(self, frame_name) != self.state.game_loop:
            setattr(self, cache_name, f(self))
            setattr(self, frame_name, self.state.game_loop)

        cached_data = getattr(self, cache_name)
        return cached_data

    return property(inner)

[MythicManiac]

That looks like it would work as well. 👍

I personally prefer explicitly naming where the cache should be stored, just for the sake of avoiding seemingly random variables appearing on the class. 😄 Your proposed solution is of course easier to use and requries less code overall

Up for you guys to decide which approach to use, just let me know if you want me to update the PR

[BurnySc2]

I'd prefer my solution because it is shorter (and IMO more readable this way). But mine implicitly uses _cache_ + function_name and _frame_ + function_name variables to store these values, while yours explicitly uses the given string as (cache) variable name.

I'd leave it up for @Dentosal to decide which variation he wants to accept, since he seems to be more actively again lately.

[Dentosal]

Good catch, and thanks for the PR. I agree with @BurnySc2, as I like the implicit naming because it makes usage a bit more cleanier. After that I'll be happy to merge this.

[MythicManiac]

Alright, I'll update this 👍

[MythicManiac]

@Dentosal @BurnySc2 updated the PR

[Dentosal]

@MythicManiac I changed this to be merged to develop first. Could you resolve the conflicts, I'm not sure about all of them.

[MythicManiac]

Looks like GitHub lost track of my fork when I rebased and closed this one, oh well. Refer to https://github.com/Dentosal/python-sc2/pull/240 for the updated version