-
Notifications
You must be signed in to change notification settings - Fork 16.1k
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
langchain[minor], community[minor], core[minor]: Async Cache support and AsyncRedisCache #15817
Changes from 5 commits
18150e4
83dff04
58256da
3244533
cbba991
c113166
d125f76
589c813
3da8707
088f6a1
6bfb3bc
168cf10
5c5a857
a7b4af4
68bf6db
91d4ed7
6c52107
51fecab
bde6216
8bcec4d
96f8067
8afbc75
9cd1d7c
66f4e49
7e8f604
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,3 +22,21 @@ def update(self, prompt: str, llm_string: str, return_val: RETURN_VAL_TYPE) -> N | |
@abstractmethod | ||
def clear(self, **kwargs: Any) -> None: | ||
"""Clear cache that can take additional keyword arguments.""" | ||
|
||
|
||
class AsyncBaseCache(ABC): | ||
"""Base interface for async cache.""" | ||
|
||
@abstractmethod | ||
async def alookup(self, prompt: str, llm_string: str) -> Optional[RETURN_VAL_TYPE]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An alternative would be to add these methods to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dzmitry-kankalovich We generally prefer keeping async version of methods in the same class as the sync versions, and have them provide a default async implementation that uses Would you mind merging into existing abstractions? Ideally a single PR that just modifies the core interface first, and then separately we can do a PR for any implementations There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As for the smaller PRs - yes, I can move slower and split up into several PRs, if you are happy with current direction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally better to split PRs by package to minimize potential dependency conflicts since the packages may have different release schedules |
||
"""Look up based on prompt and llm_string.""" | ||
|
||
@abstractmethod | ||
async def aupdate( | ||
self, prompt: str, llm_string: str, return_val: RETURN_VAL_TYPE | ||
) -> None: | ||
"""Update cache based on prompt and llm_string.""" | ||
|
||
@abstractmethod | ||
async def aclear(self, **kwargs: Any) -> None: | ||
"""Clear cache that can take additional keyword arguments.""" |
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.
Maybe for this you could have a
RedisCache
that uses aredis.Redis
client and anAsyncRedisCache
that uses aredis.asyncio.Redis
? (I know this is the opposite move as for BaseCache 😄 )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 actually did it this way in the very first variant, but I didn't like it was literally 99% of the copy from the original RedisCache. The remaining 1% were async/await keywords and client check in constructor. Feels like a potential future maintenance problem?
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.
Also, I am not sure what I suppose to do for
AsyncRedisCache
sync methodslookup/update/clear
. Like I can raise error on those, or do the opposite trick for async in sync execution, but doesn't it feel like too much of duplication of effort?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.
You could put this common behavior in a parent abstract class ?
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.
Alright, I can do it. But before that, can you tell me whats your rationale here? Just so I know for future :)
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.
RedisBaseCache is abstract and has methods _key, __ensure_generation_type __get_generations __configure_pipeline_for_update
RedisCache extends RedisBaseCache and implements methods lookup, update, clear using redis.Redis
AsyncRedisCache extends RedisBaseCache and implements methods alookup, aupdate, aclear using redis.asyncio.Redis
AsyncRedisCache raises NotImplementedError() for lookup, update, clear
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.
@cbornet I mean I was asking whats the motivation for such split.
I've pushed the implementation which is basically what you said, however I went a bit further with sync-in-async and actually made it so you can run it in certain context (weird case though, I log warning for that). I can rework to
NotImplementedError
if you think its cleaner.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.
At the moment, we generally try to avoid that. Only one event loop is allowed in a given thread, so there's some additional logic that's required to determine if there's an event loop running already and if yes, kick off another thread etc