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

Adding eviction policy in semantic cache class #99

Merged
merged 4 commits into from
May 30, 2024

Conversation

SHUBHAGYTA24
Copy link
Contributor

What does this PR do?

The proposed changes to the semantic_cache class aim to improve its efficiency and flexibility. By introducing eviction policy FIFO, we provide users with the ability to control how the cache behaves when it reaches its maximum capacity. This is crucial for maintaining optimal cache performance and for handling situations where the available memory is constrained.

Fixes # (issue)

Who can review?

Feel free to tag members/contributors who may be interested in your PR.

@merveenoyan @stevhliu

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@@ -1,48 +1,10 @@
{
Copy link
Member

@stevhliu stevhliu May 28, 2024

Choose a reason for hiding this comment

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

"I have included First In First Out (FIFO) eviction policy in the SemanticCache class to improve efficiency and flexibility. By introducing eviction policies, we provide users with the ability to control how the cache behaves when it reaches maximum capacity. This is crucial for maintaining optimal cache performance and for handling situations where memory is constrained.

Looking at the structure of the cache, the FIFO eviction policy is straightforward. Whenever a new question-answer pair is added to the cache, it's appended to the end of the lists. Thus, the oldest (first-in) items are at the beginning of the lists. When the cache reaches its maximum size and you need to evict an item, you remove (pop) the first item from each list.

However, the Least Recently Used (LRU) eviction policy is more complex because it requires knowledge of when each item in the cache was last accessed."


Reply via ReviewNB

Copy link
Member

@stevhliu stevhliu left a comment

Choose a reason for hiding this comment

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

Cool!

Is the LRU eviction policy implemented? I see that eviction_policy can be either LRU or FIFO, but in the evict function I only see FIFO implemented.

@SHUBHAGYTA24
Copy link
Contributor Author

No, I haven't included LRU as I didn't see cache system storing order/time of access. I have mentioned that since FIFO implementation was straight forward, I implemented that only as of now.

Will implement LRU as well in future PRs.

@stevhliu

@stevhliu
Copy link
Member

stevhliu commented May 29, 2024

Ah ok, maybe you can mention that in the recipe to avoid confusion. Maybe something like:

"Another eviction policy is the Least Recently Used (LRU) policy, which is more complex because it requires knowledge of when each item in the cache was last accessed. However, this policy is not yet available and will be implemented later."

Could probably also remove LRU from the docstring of the evict function as well since it isn't really an option currently.

@SHUBHAGYTA24
Copy link
Contributor Author

Sure. Done

@stevhliu

Copy link
Member

@stevhliu stevhliu left a comment

Choose a reason for hiding this comment

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

Very cool, thanks for adding to this recipe! 🤗

@stevhliu stevhliu merged commit 3aaa5bd into huggingface:main May 30, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants