-
Notifications
You must be signed in to change notification settings - Fork 6k
Add cachetools key-value store #18883
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
Conversation
Thanks a lot @Florian-BACHO! |
readme = "README.md" | ||
license = "MIT" | ||
dependencies = [ | ||
"cachetools>=6.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on cachetools-utils? https://github.com/zx80/cachetools-utils
Its the same, but extended for supporting redis and memcache (which feels like it would make this class more useful)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, apart from the use of cachetools-utils
instead of cachetools
as already Logan pointed out, this is a good PR!
They only thing I would add is a README, at least to give an idea of how to initialize this :))
Thanks for your feedbacks @AstraBert and @logan-markewich ! |
Hey there @Florian-BACHO, My sense is that this would require discarding the BaseKVStore, or refactoring it as the MutableMapping store that you propose: it might be an idea for future releases, but for now I'd keep things as they are :)) |
This is something that crossed my mind but so many kv stores already inherit |
@AstraBert @logan-markewich I opened a new PR (#18893) with an alternative approach that is more generic. |
Description
I propose a new
CachetoolsKVStore
class, a key-value store that wraps cachetoolsCache
objects. This will allow creating different implementations of cache stores, mainly in the context of theBaseEmbedding
's new caching system being implemented in PR #18864.New Package?
Did I fill in the
tool.llamahub
section in thepyproject.toml
and provide a detailed README.md for my new integration or package?Version Bump?
Did I bump the version in the
pyproject.toml
file of the package I am updating? (Except for thellama-index-core
package)Type of Change
How Has This Been Tested?
Your pull-request will likely not be merged unless it is covered by some form of impactful unit testing.
Suggested Checklist:
uv run make format; uv run make lint
to appease the lint gods