-
Notifications
You must be signed in to change notification settings - Fork 38
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 custom redis cache #33
Add custom redis cache #33
Conversation
Fix merge conflict
Did a small refactor and encapsualted the default cache that you've used. it probably still needs some work to achieve full integration though. |
@ishaan-jaff @krrishdholakia any opinions so far guys? This needs some love & testing, will gladly help, but I need the intial approval to push things forward. |
any feedback here...? |
Sorry for the delay @grski will take a look and respond by EOD |
thank you very much, no need to hurry if you have other priorities, the information is enough that you have this on the todo list, no worries :) |
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.
FYI t's failing a test due to a missing redis import. Once that's fixed should be good to merge
feat(envs): add env handling based on .env file
@ishaan-jaff @krrishdholakia |
@@ -33,7 +33,7 @@ jobs: | |||
with: | |||
poetry-version: 1.3.2 | |||
- name: Install dependencies | |||
run: poetry install | |||
run: poetry install --all-extras |
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.
in ci/cd all extras should be included
return {"choices": [{"message": {"content": extracted_result}}]} | ||
|
||
|
||
class DefaultCache(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.
this is the default cache i just moved over, cleaning it up slightly
return self.map_results(extracted_result=extracted_result) | ||
|
||
|
||
class RedisCache(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.
simple redis exact match case
For now, we do the simplest approach possible, meaning we cache the results in Redis. Using exact match strategy. In the future this should be extended to other stores and similarity based matching to enable true semantic caching. For that vector store handling needs to be added. Proper exception handling.
Validations and defaults for environment variables were added using pydantic and python-dotenv for local setup. In the future this should be used to load and validate all the env variables as it's currently the preferred go-to approach in Python world.
Tests need to be added, but for that to happen I think we should refactor the whole testing approach.
Pipelines will fail coz of some lints that are still left out there to be fixed and some tests that were broken since the beginning. We also need to work on that.
Documentation to follow if this implementation is deemed merge-worthy.