-
-
Notifications
You must be signed in to change notification settings - Fork 514
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
Blog: Mastering Caching #219
Conversation
WalkthroughThe recent updates introduce a series of blog posts and code examples focused on caching strategies in Python, particularly for functions returning Pydantic models. The posts cover in-memory caching with Changes
Poem
TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 11
Configuration used: CodeRabbit UI
Files selected for processing (7)
- docs/blog/index.md (1 hunks)
- docs/blog/posts/caching.md (1 hunks)
- docs/blog/posts/citations.md (1 hunks)
- docs/blog/posts/rag-and-beyond.md (10 hunks)
- examples/caching/diskcache.py (1 hunks)
- examples/caching/lru.py (1 hunks)
- examples/caching/redis.py (1 hunks)
Files skipped from review due to trivial changes (3)
- docs/blog/index.md
- docs/blog/posts/citations.md
- docs/blog/posts/rag-and-beyond.md
Additional comments: 8
examples/caching/diskcache.py (7)
1-6:
The imports are correctly added as per the summary.8-8:
Theclient
variable is correctly initialized.10-12:
TheUserDetail
model is correctly defined with the appropriate fields.14-14:
The disk cache is correctly initialized at the specified directory.16-38:
Theinstructor_cache
decorator is correctly implemented to cache the results of functions returning Pydantic models.40-48:
Theextract
function is correctly decorated withinstructor_cache
and uses theclient
as expected.51-70:
Thetest_extract
function is correctly implemented to test the caching behavior and performance.examples/caching/lru.py (1)
- 12-20:
The
functools.lru_cache
decorator requires the function's return value to be hashable, but theclient.chat.completions.create
method is likely returning a non-hashable object (like a dictionary or a custom object). Verify that the return value is compatible withlru_cache
.
docs/blog/posts/caching.md
Outdated
> Instructor make working with language models easy, but they are still computationally expensive. | ||
|
||
Today, we're diving into optimizing instructor code while maintaining the excellent DX offered by Pydantic models. We'll tackle the challenges of caching Pydantic models, typically incompatible with `pickle`, and explore solutions that use `decorators` like using `functools.cache`. Then, we'll craft custom decorators with `diskcache` and `redis`. |
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.
The word "Instructor" should be lowercase when not used as a proper noun, and "DX" should be expanded to "Developer Experience" for clarity.
|
||
!!! warning "Changing the Model does not Invalidate the Cache" | ||
|
||
Note that changing the model does not invalidate the cache. This is because the cache key is based on the function's name and arguments, not the model. This means that if we change the model, the cache will still return the old result. |
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.
The warning about changing the model not invalidating the cache is important, but it should be made clear that this is a potential pitfall and developers should manually invalidate the cache if the model changes.
docs/blog/posts/caching.md
Outdated
def instructor_cache(func): | ||
"""Cache a function that returns a Pydantic model""" | ||
return_type = inspect.signature(func).return_annotation | ||
if not issubclass(return_type, BaseModel): | ||
raise ValueError("The return type must be a Pydantic model") | ||
|
||
@functools.wraps(func) | ||
def wrapper(*args, **kwargs): | ||
key = f"{func.__name__}-{str(args)}-{str(kwargs)}" | ||
# Check if the result is already cached | ||
if (cached := cache.get(key)) is not None: | ||
# Deserialize from JSON based on the return type | ||
if issubclass(return_type, BaseModel): | ||
return return_type.model_validate_json(cached) | ||
|
||
# Call the function and cache its result | ||
result = func(*args, **kwargs) | ||
serialized_result = result.model_dump_json() | ||
cache.set(key, serialized_result) | ||
|
||
return result | ||
|
||
return wrapper |
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.
The instructor_cache
decorator is using string concatenation to create the cache key, which could lead to collisions if the string representations of different arguments result in the same string. Consider using a more robust method for generating unique cache keys.
docs/blog/posts/caching.md
Outdated
|
||
!!! note "Looking carefully" | ||
|
||
If you look carefully at the code above you'll notice that we're using the same `instructor_cache` decorator as before. The implemntations is the same, but we're using a different caching backend! |
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.
There is a typo in the word "implementations". It should be corrected to "implementation".
start = time.perf_counter() | ||
model = extract("Extract jason is 25 years old") | ||
assert model.name.lower() == "jason" | ||
assert model.age == 25 | ||
print(f"Time taken: {time.perf_counter() - start}") |
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.
The test assertions are based on the assumption that the external service will always return the same output for a given input, which may not be reliable. Consider mocking the external service to ensure consistent test results.
examples/caching/redis.py
Outdated
import redis | ||
import functools | ||
import inspect | ||
import json | ||
import instructor | ||
|
||
from pydantic import BaseModel | ||
from openai import OpenAI | ||
|
||
client = instructor.patch(OpenAI()) | ||
cache = redis.Redis("localhost") | ||
|
||
def instructor_cache(func): | ||
"""Cache a function that returns a Pydantic model""" | ||
return_type = inspect.signature(func).return_annotation | ||
if not issubclass(return_type, BaseModel): | ||
raise ValueError("The return type must be a Pydantic model") | ||
|
||
@functools.wraps(func) | ||
def wrapper(*args, **kwargs): | ||
key = f"{func.__name__}-{str(args)}-{str(kwargs)}" | ||
# Check if the result is already cached | ||
if (cached := cache.get(key)) is not None: | ||
# Deserialize from JSON based on the return type | ||
if issubclass(return_type, BaseModel): | ||
return return_type.model_validate_json(cached) | ||
|
||
# Call the function and cache its result | ||
result = func(*args, **kwargs) | ||
serialized_result = result.model_dump_json() | ||
cache.set(key, serialized_result) | ||
|
||
return result | ||
|
||
return wrapper | ||
|
||
|
||
class UserDetail(BaseModel): | ||
name: str | ||
age: int | ||
|
||
@instructor_cache | ||
def extract(data) -> UserDetail: | ||
# Assuming client.chat.completions.create returns a UserDetail instance | ||
return client.chat.completions.create( | ||
model="gpt-3.5-turbo", | ||
response_model=UserDetail, | ||
messages=[ | ||
{"role": "user", "content": data}, | ||
] | ||
) | ||
|
||
def test_extract(): | ||
import time | ||
|
||
start = time.perf_counter() | ||
model = extract("Extract jason is 25 years old") | ||
assert model.name.lower() == "jason" | ||
assert model.age == 25 | ||
print(f"Time taken: {time.perf_counter() - start}") | ||
|
||
start = time.perf_counter() | ||
model = extract("Extract jason is 25 years old") | ||
assert model.name.lower() == "jason" | ||
assert model.age == 25 | ||
print(f"Time taken: {time.perf_counter() - start}") | ||
|
||
|
||
if __name__ == "__main__": | ||
test_extract() | ||
# Time taken: 0.798335583996959 | ||
# Time taken: 0.00017016706988215446 |
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.
- The
instructor_cache
decorator assumes that the function it decorates will return a Pydantic model, which is a strong assumption that may not always hold true. This could be made more flexible or clearly documented. - The cache key generation in line 21 does not account for mutable types or non-hashable types in
args
andkwargs
, which could lead to issues. - There is no expiration time set for the cache entries, which could lead to stale data issues.
- The
test_extract
function uses hardcoded assertions that may not be reliable for testing if the AI service's response changes over time. - The
test_extract
function prints the time taken but does not assert on the expected caching behavior, such as a significantly reduced time for the second call.
examples/caching/redis.py
Outdated
|
||
@functools.wraps(func) | ||
def wrapper(*args, **kwargs): | ||
key = f"{func.__name__}-{str(args)}-{str(kwargs)}" |
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.
The cache key generation strategy may lead to collisions or may not handle complex data types well. Consider using a more robust serialization method for cache keys.
examples/caching/redis.py
Outdated
if issubclass(return_type, BaseModel): | ||
return return_type.model_validate_json(cached) |
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.
Redundant check for return_type
being a subclass of BaseModel
. This check is already performed before defining the wrapper function.
examples/caching/redis.py
Outdated
serialized_result = result.model_dump_json() | ||
cache.set(key, serialized_result) |
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.
Consider adding an expiration time (TTL) for the cache entries to prevent stale data from persisting indefinitely.
examples/caching/redis.py
Outdated
def test_extract(): | ||
import time | ||
|
||
start = time.perf_counter() | ||
model = extract("Extract jason is 25 years old") | ||
assert model.name.lower() == "jason" | ||
assert model.age == 25 | ||
print(f"Time taken: {time.perf_counter() - start}") | ||
|
||
start = time.perf_counter() | ||
model = extract("Extract jason is 25 years old") | ||
assert model.name.lower() == "jason" | ||
assert model.age == 25 | ||
print(f"Time taken: {time.perf_counter() - start}") |
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.
The test_extract
function should include assertions or checks to confirm that the caching behavior is working as expected, such as verifying that the second call hits the cache and does not perform the full computation.
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.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- docs/blog/posts/caching.md (1 hunks)
- examples/caching/example_diskcache.py (1 hunks)
- examples/caching/example_redis.py (1 hunks)
Files skipped from review due to trivial changes (1)
- docs/blog/posts/caching.md
Additional comments: 9
examples/caching/example_diskcache.py (4)
6-7:
The imports are appropriate for the functionality described in the summary.17-39:
Theinstructor_cache
decorator is correctly implemented to cache the output of functions returning a Pydantic model.41-49:
Theextract
function is correctly decorated withinstructor_cache
and is set to return aUserDetail
instance.52-65:
Thetest_extract
function correctly tests the caching functionality by asserting the results of two calls to theextract
function with the same input.examples/caching/example_redis.py (5)
1-10:
The imports and global variable initialization forclient
andcache
are correctly implemented as per the summary.12-34:
Theinstructor_cache
decorator is correctly implemented to cache the result of a function that returns a Pydantic model. However, there is a potential issue with the cache key generation and the deserialization process.37-50:
TheUserDetail
model and theextract
function are correctly implemented and decorated with@instructor_cache
as per the summary.52-65:
Thetest_extract
function is correctly implemented to test the caching behavior of theextract
function.68-71:
The main block is correctly implemented to run thetest_extract
function and print the time taken for execution.
# Time taken: 0.7285366660216823 | ||
# Time taken: 9.841693099588156e-05 |
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.
The commented time taken values should be removed or updated to avoid confusion, as they may not reflect the current performance after changes.
if (cached := cache.get(key)) is not None: | ||
# Deserialize from JSON based on the return type | ||
if issubclass(return_type, BaseModel): | ||
return return_type.model_validate_json(cached) | ||
|
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.
The deserialization from JSON assumes that the result is stored as JSON, but the serialization process is not explicitly converting the result to JSON before caching it.
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.
yes it does on line 29
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.
@jxnl My apologies for the oversight. You are correct, the serialization to JSON is indeed handled on line 29 with serialized_result = result.model_dump_json()
. Thank you for pointing that out.
🐇
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- mkdocs.yml
Files selected for processing (2)
- .gitignore (1 hunks)
- docs/concepts/caching.md (1 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional comments: 8
docs/concepts/caching.md (8)
1-5:
The introduction to thefunctools.cache
section is well-written and provides a clear context for when this caching method is appropriate.19-27:
The use offunctools.cache
to decorate theextract
function is correctly implemented for in-memory caching.30-32:
The warning about the cache not being invalidated when the model changes is a crucial piece of information for users to understand the limitations of the caching mechanism.47-48:
The output comments in the example show the performance improvement due to caching, but the second time taken appears to be an unrealistic example. Please verify the accuracy of these times.87-123:
The note about reusing theinstructor_cache
decorator for bothdiskcache
andredis
caching promotes code reuse and consistency.96-117:
Theinstructor_cache
decorator fordiskcache
is well-defined, with checks for the return type being a Pydantic model and appropriate serialization/deserialization.178-181:
The explanation of theinstructor_cache
decorator's functionality, including key generation and result serialization, is clear and informative.198-220:
Theinstructor_cache
decorator for Redis caching is correctly implemented, mirroring thediskcache
version's checks and serialization logic.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Refactor
functools
,diskcache
, and Redis for optimizing function performance in Python.