Use cachetools's LRUCache to cache manifest list #1187
Conversation
| ] | ||
|
|
||
|
|
||
| @cached(cache=LRUCache(maxsize=128), key=lambda io, manifest_list: hashkey(manifest_list)) |
There was a problem hiding this comment.
Did the use of cachetools LRUCache, instead of functools.lru_cache solve the issues with the FileSystem on M1 Macs we were observing before?
There was a problem hiding this comment.
yep! the difference is that functools.lru_cache uses all of the function's args as the cache key, including the io arg.
Using cachetools, I can specify which argument to use as the cache key. In this case, only using manifest_list as the cache key
key=lambda io, manifest_list: hashkey(manifest_list)
There was a problem hiding this comment.
That's so clever - so the issue was with the function arguments being 'held up' by the cache preventing them from being GCed 🤯 🧠
This is next level @kevinjqliu 💯
There was a problem hiding this comment.
That's my best theory, haha. Regardless of the underlying cause, we should not be using io as the cache key!
kevinjqliu
force-pushed
the
kevinjqliu/manifest-list-cachetools
branch
from
13fe7d6 to
ade0fc5
Compare
|
When is the release (0.8.0) planned for - would it be possible to release this fix on a 0.7.2 hotfix release? This solves #1162 bug that is blocking usage, unless there is a workaround. Thanks! |
* use cachetools * use LRU cache * return tuple * comment * clear global cache for tests * move _manifests to manifest.py * rebase poetry.lock
* use cachetools * use LRU cache * return tuple * comment * clear global cache for tests * move _manifests to manifest.py * rebase poetry.lock
3 participants
Closes #595, Reverts #787, Closes #1162
Reimplement manifest list cache using the
cachetoolslibrary. This keeps the manifest list cache as a global cache and introduces the ability to specify the cache key as onlymanifest_list.The cache result is stored as
Tupleinstead ofListto prevent modificationMove the
_manifestsfunction fromsnapshots.pytomanifest.pysince its a global manifest cache and not specific to any 1 snapshot