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

Core: Handle Failed Cache Reads For Non-Serializable Objects #27989

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

FarhanChowdhury248
Copy link

This PR fixes the issue mentioned in #27264.

Description

Bug

Many caching implementations for chat model results (as defined in langchain_core/caches.py) require cached objects to be Serializable in order to properly perform the cache read and write. In cases where they are not, the lookup may fail when the cache attempts to de-serialize the cached data into a list of ChatGeneration objects. On such failure, many caches in langchain_core\caches.py fail over to returning a list of Generation objects that contain the serialized data strings.

When these are read by BaseChatModel in _generate_with_cache or _agenerate_with_cache, the cached result is fed into a ChatGeneration object, which does not accept Generation objects, leading to the error.

Fix

Add a check in _generate_with_cache and _agenerate_with_cache to verify the cached results are of type ChatGeneration[]. If not, skip the lookup and treat the case as an uncached one.

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Nov 8, 2024
Copy link

vercel bot commented Nov 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Nov 8, 2024 2:24pm

@dosubot dosubot bot added Ɑ: core Related to langchain-core 🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature labels Nov 8, 2024
@FarhanChowdhury248
Copy link
Author

Where do tests for BaseChatModel go? I was hoping to add the case defined in #27264 as a test case for this, but could not find any tests that test BaseChatModel directly.

@FarhanChowdhury248
Copy link
Author

@jacoblee93 Do you mind having a look at this?

@FarhanChowdhury248
Copy link
Author

@baskaryan Can someone have a look at this?

@efriis efriis self-assigned this Dec 10, 2024
@efriis
Copy link
Member

efriis commented Dec 10, 2024

hey there! Sorry for the delay on this one. Could you add some tests either to this, or even better to some of the integration packages where you're seeing this failure mode with InMemoryCache? I don't fully understand why this needs to be implemented on BaseChatModel, instead of fixing on a specific cache that has a bug.

@FarhanChowdhury248
Copy link
Author

@efriis Thanks for taking the time to look into it. The fix is on BaseChatModel because the root cause of the issue has to do with how the cache reads are converted into a ChatResult. Any cache implementation that tries to read an unsupported object from its serialized version will run into this error.

Do you know where the ideal place to add tests for this? I looked into the test directories but found no designated directory/file for tests that focus on (de)serialization.

@efriis
Copy link
Member

efriis commented Dec 12, 2024

if this is the fix that's necessary, would recommend adding to libs/core/tests/unit_tests/fake/test_fake_chat_model.py or even a new one of libs/core/tests/unit_tests/fake/test_fake_chat_model_with_cache.py using the InMemoryCache

this will help me understand what the issue is

@efriis
Copy link
Member

efriis commented Dec 17, 2024

@FarhanChowdhury248 let me know if you're interested in writing some of those unit tests! Otherwise I'll close later this week (and folks are always welcome to reopen once those are up demonstrating the issue)

@FarhanChowdhury248
Copy link
Author

Hey @efriis. I plan on getting on this after tomorrow, when my last exam is done. Sorry I haven't been able to keep up with this.

@ccurme ccurme marked this pull request as draft December 20, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature Ɑ: core Related to langchain-core size:S This PR changes 10-29 lines, ignoring generated files.
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

2 participants