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

Use pydantic for cache entries and hits #195

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

tylerhutcherson
Copy link
Collaborator

This PR handles 2 things:

  1. Uses pydantic for both CacheEntry and CacheHit models. It ended up being better and cleaner to have two separate models.... one for what we write, and the other for what we load.

  2. Filtering. @justin-cechmanek I want your feedback on this. I landed on a technique to allow for the user to define a list of filterable_fields as part of the semantic cache class init. What this does is give us a way to support arbitrary filters (scope, permissions, tags, numerics.... anything) within reason. Then you can create any FilterExpression and pass through at query time. This extends your initial implementation.

@tylerhutcherson tylerhutcherson added the enhancement New feature or request label Aug 1, 2024
Copy link
Collaborator

@justin-cechmanek justin-cechmanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just one note about updating the doc string

@@ -68,9 +49,6 @@ def __init__(
Args:
name (str, optional): The name of the semantic cache search index.
Defaults to "llmcache".
prefix (Optional[str], optional): The prefix for Redis keys
associated with the semantic cache search index. Defaults to
None, and the index name will be used as the key prefix.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a description for filterable_fields?

And I don't think this raises a Value error if the index name is not provided, so we can drop that comment line.

redisvl/extensions/llmcache/semantic.py Outdated Show resolved Hide resolved
@tylerhutcherson tylerhutcherson merged commit 0a2e432 into main Aug 1, 2024
19 of 20 checks passed
@tylerhutcherson tylerhutcherson deleted the feat/RAAE-207/cache-entry-model branch August 1, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants