Skip to content
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

Add caching support to BaseChatModel #1644

Closed
asadovsky opened this issue Mar 13, 2023 · 9 comments · Fixed by #5089
Closed

Add caching support to BaseChatModel #1644

asadovsky opened this issue Mar 13, 2023 · 9 comments · Fixed by #5089

Comments

@asadovsky
Copy link

Looks like BaseLLM supports caching (via langchain.llm_cache), but BaseChatModel does not.

@asadovsky
Copy link
Author

Here is a suboptimal workaround that I came across: https://gist.github.com/BlackHC/49a37aaa6e9f3e31928596ce477897ad

Note that it only works for generate, not agenerate, and only for ChatOpenAI.

@UmerHA
Copy link
Contributor

UmerHA commented May 18, 2023

Working on it - will be done today or tomorrow

@kaikun213
Copy link
Contributor

Amazing! Any news on this? @UmerHA

@UmerHA
Copy link
Contributor

UmerHA commented May 22, 2023

Yes: I'm working on it (https://github.com/UmerHA/langchain/tree/1644-BaseChatModel-Caching), but it got delayed as I had to fix another PR first.

@UmerHA
Copy link
Contributor

UmerHA commented May 22, 2023

Amazing! Any news on this? @UmerHA

aaaand-its-done-r7d5cl

#5089 :)

hwchase17 added a commit that referenced this issue Jun 24, 2023
#  Add caching to BaseChatModel
Fixes #1644

(Sidenote: While testing, I noticed we have multiple implementations of
Fake LLMs, used for testing. I consolidated them.)

## Who can review?
Community members can review the PR once tests pass. Tag
maintainers/contributors who might be interested:
Models
- @hwchase17
- @agola11

Twitter: [@UmerHAdil](https://twitter.com/@UmerHAdil) | Discord:
RicChilligerDude#7589

---------

Co-authored-by: Harrison Chase <hw.chase.17@gmail.com>
@gururise
Copy link
Contributor

gururise commented Jun 25, 2023

@UmerHA
Does this work for agenerate? Still getting:
ValueError: GPTCache only supports caching of normal LLM generations, got <class 'langchain.schema.ChatGeneration'>

This was referenced Jun 25, 2023
@UmerHA
Copy link
Contributor

UmerHA commented Jun 27, 2023

@gururise Hey, the lc team adapted the pr and now it doesn't, but it was a conscious decision from their pov. I assume the reasoning is that some caches only make sense with single-message inputs. GPTCache would be one of them.

@pors
Copy link
Contributor

pors commented Jun 27, 2023

@gururise Hey, the lc team adapted the pr and now it doesn't, but it was a conscious decision from their pov. I assume the reasoning is that some caches only make sense with single-message inputs. GPTCache would be one of them.

Could you please elaborate a bit on that? Why won't caching make sense for a chat model?

@jakobsa
Copy link

jakobsa commented Jul 4, 2023

Thanks for the merge! I have been looking into RedisCache and
noticed the cache throwing KeyError 'message' when cache value would be available in the cache.

The reason seems to be, that it is not intended for use with ChatGenerations. There are ValueErrors in the code making statements along those lines. The limitation is fine, I will subclass and override. However I wanted to come back to the PR as the intended ValueError is never thrown. ChatGeneration is a subclass of Generation and therefor not hitting:

            if not isinstance(gen, Generation):
                raise ValueError(
                    "RedisCache only supports caching of normal LLM generations, "
                    f"got {type(gen)}"
                )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants