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.

Sign up for GitHub

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

Jump to bottom

Distributed cache feature using Redis #518

Merged
merged 22 commits into from
Aug 17, 2023
Merged

Distributed cache feature using Redis #518

merged 22 commits into from
Aug 17, 2023

Conversation

a9raag
Copy link
Contributor

@a9raag a9raag commented Aug 16, 2023 •
edited
Loading

Distributed Cache using Redis #496

Feature for distributed cache. This allows us to store keys in Redis key-value store.
Sorry, it took a while I spent too much time thinking about a design for this and how it will integrate into GPTCache's existing code base.

I have also attached a high-level design of how Distributed Cache works.

Notes

A couple of notes about how the eviction will work based on different scalar storage configurations:

Scenario 1:
Scalar Storage is MongoDB,
Eviction Base is Redis.
Then the keys will be stored in Redis, and cache call back will update the TTL.

Scenario 2:
Scalar Storage is Redis,
Eviction Base is also Redis,
Then storing keys separately becomes redundant since Redis eviction is based on Memory usage rather than the number of keys.
Also, we'll be fetching values multiple times both while getting scalar data and while hitting cache callback, and it will add network overhead.

To tackle this I created a NoOpEviction implementation (which skips cache operations) since,

  1. Redis internally manage the eviction based on memory size and eviction policy for all keys.
  2. We can configure RedisCacheStorage to update TTL upon creation and during access. This way values will be fetched from Redis only once.

GPT Distributed Cache

a9raag added 8 commits August 16, 2023 13:35
@a9raag
[add] initial draft for distributed caching
ad669b4 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[mod] Simplify Model schema for distributed cache management.
ca1a986 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[mod] update expiry time for keys when accessed or saved
2f62920 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[add] NoOpEviction mechanism to avoid redundant calls to redis
9de88f8 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[add] unit tests for distributed cache
d83bc2a 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[add] temporary directory for sqlite
b23c660 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[mod] skip adding scalar ids to cache if NoOpEviction is used
e017f0d 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[add] documentation, example code and refactoring
730da1e 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@mergify mergify bot added the dco-passed label Aug 16, 2023
@SimFG SimFG changed the base branch from main to dev August 16, 2023 08:27
a9raag added 5 commits August 16, 2023 14:01
@a9raag
[refactor] added docstrings, consistent quotes, removed literal compa…
ce49ee4 
…rison

Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[refactor] removed unused imports
8586d16 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[fix] add lazy import for redis in distributed_cache.py
3506cbd 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[add] pylint: disable=wrong-import-position
b8c5d20 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[refactor] grouped import statements for distributed cache
0a9749a 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
Copy link
Contributor Author

a9raag commented Aug 16, 2023

@SimFG
Can you help the test cases are failing because of

'(ReadTimeoutError("HTTPSConnectionPool(host='huggingface.co', port=443): Read timed out. (read timeout=10.0)"),
 '(Request ID: 3afc7015-e7b6-4b6a-abd6-acd698b9469f)')' thrown while requesting GET 
https://huggingface.co/GPTCache/paraphrase-albert-small-v2/resolve/main/tokenizer.json

@SimFG
Copy link
Collaborator

SimFG commented Aug 16, 2023

@a9raag ok, I have rerun it

a9raag added 5 commits August 16, 2023 19:00
@a9raag
[refactor] revert get_data_manager and manager_factory signatures…
7d113e3 
… to include memory cache config

Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[mod] add default values to memory cache params
1dd36df 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[mod] add param to set mammemory-samples config for redis cache
f1a3633 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[mod] updated cache config for testing lru cache
d650109 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[del] removed test-lru-cache
1814738 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
Copy link
Contributor Author

a9raag commented Aug 16, 2023

@SimFG it failed again

@a9raag
[add] description for maxmemory_samples param
a143f51 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@codecov
Copy link

codecov bot commented Aug 17, 2023 •
edited
Loading

Codecov Report

Merging #518 (ce8db36) into dev (5aa7e1e) will decrease coverage by 0.16%.
Report is 1 commits behind head on dev.
The diff coverage is 93.33%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #518      +/-   ##
==========================================
- Coverage   94.05%   93.89%   -0.16%     
==========================================
  Files          94       95       +1     
  Lines        3920     4014      +94     
==========================================
+ Hits         3687     3769      +82     
- Misses        233      245      +12
Files Changed Coverage Δ
gptcache/manager/eviction/memory_cache.py 90.90% <ø> (ø)
gptcache/manager/eviction/distributed_cache.py 89.65% <89.65%> (ø)
gptcache/manager/scalar_data/redis_storage.py 96.13% <93.18%> (-1.51%) ⬇️
gptcache/__init__.py 100.00% <100.00%> (ø)
gptcache/manager/data_manager.py 89.60% <100.00%> (+0.10%) ⬆️
gptcache/manager/eviction/manager.py 100.00% <100.00%> (ø)
gptcache/manager/factory.py 96.55% <100.00%> (+0.89%) ⬆️

... and 1 file with indirect coverage changes

a9raag added 2 commits August 17, 2023 09:56
@a9raag
[add] unit test for validating ttl configuration
1b6abc0 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[add] unit tests for validating cache configuration, ttl access
4ead9c8 
Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
[add] unit test to validate str based access for eviction base in `ge…
ce8db36 
…t_data_manager`

Signed-off-by: Anurag Wagh <a9raag@gmail.com>
@a9raag
Copy link
Contributor Author

a9raag commented Aug 17, 2023

@SimFG apologies if I keep bugging you but the Unit Test workflow failed again 😭

Here's the error log

/home/runner/work/_temp/c7cf298b-8f2b-475f-995a-f3f5562139f1.sh: line 3:  3496 Illegal instruction    
 (core dumped) python3 -m pytest -k "not embedding and not processor" --cov=gptcache --cov-report xml:coverage.xml --cov-append ./tests/
Error: Process completed with exit code 132.

@a9raag a9raag changed the title Feature/distributed cache Distributed cache feature using Redis Aug 17, 2023
@mergify mergify bot added the ci-passed label Aug 17, 2023
@a9raag
Copy link
Contributor Author

a9raag commented Aug 17, 2023

Hi @SimFG and @xiaofan-luan
I've updated the PR.
The Unit Tests and CodeCov have now passed.
Could you please review it when you get a chance?
Thanks : )

@SimFG
Copy link
Collaborator

SimFG commented Aug 17, 2023

@a9raag great!!! i will check it today

@SimFG
Copy link
Collaborator

SimFG commented Aug 17, 2023

/lgtm
/approve

@sre-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a9raag, SimFG

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot sre-ci-robot merged commit c2deaec into zilliztech:dev Aug 17, 2023
@a9raag a9raag deleted the feature/distributed-cache branch September 14, 2023 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimFG SimFG Awaiting requested review from SimFG

@xiaofan-luan xiaofan-luan Awaiting requested review from xiaofan-luan

Assignees

@SimFG SimFG

Labels
approved ci-passed dco-passed lgtm size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@a9raag @SimFG @sre-ci-robot