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

Better approach of making a cache #86

Open
vdusek opened this issue Apr 3, 2024 · 6 comments
Open

Better approach of making a cache #86

vdusek opened this issue Apr 3, 2024 · 6 comments
Labels
debt Code quality improvement or decrease of technical debt. hacktoberfest t-tooling Issues with this label are in the ownership of the tooling team.

Comments

@vdusek
Copy link
Collaborator

vdusek commented Apr 3, 2024

  • This is a follow-up issue to the discussion in Add storages and memory storage clients #82 (comment).
  • Currently, we have our own implementation of LRU cache in crawlee/_utils/lru_cache.py.
  • Let's do it in a more Pythonic way, maybe utilizing the built-in caching from functools std module (lru_cache decorator)?
@vdusek vdusek added t-tooling Issues with this label are in the ownership of the tooling team. debt Code quality improvement or decrease of technical debt. labels Apr 3, 2024
@Azathoth-X
Copy link

Azathoth-X commented Oct 4, 2024

Was reading on this and wanted to clear up somethings.
The lru_cache from functools has no delete function for a given key, it just clears whole cache.
Can you define what do you mean by pythonic?
The cache implementation looks good as it.

Edit: I just saw the usage of this class and del wasn't used atleast in the python codebase. Please do tell me if it is also being used in the ts base. I will try to implement functools LRU . Will update you if there is any progress from my side.

@29deepanshutyagi
Copy link

i want to work on this issue ,kindly assign me @B4nan , if it's still opened

@janbuchar
Copy link
Collaborator

i want to work on this issue ,kindly assign me @B4nan , if it's still opened

We don't assign issues for hacktoberfest. If you want to work on this, open a PR. First mergeable one gets merged.

@vdusek
Copy link
Collaborator Author

vdusek commented Oct 8, 2024

Can you define what do you mean by pythonic?

Try using something from the standard libraries.

@Azathoth-X
Copy link

Azathoth-X commented Oct 8, 2024

Hi @vdusek ,
Just wanted to ask do we need the delete individual type saved in cache function, in this class. I could only trace references of get, clear and creating key value pair for LRUcache class.
Only place where the delete function is used is unit tests.

def test_del(lru_cache: LRUCache[int]) -> None:
    # Key error on non-existent key
    with pytest.raises(KeyError):
        del lru_cache['non-existent-key']
    # No error with existing key
    len_before_del = len(lru_cache)
    del lru_cache['a']
    assert len(lru_cache) == len_before_del - 1
    assert 'a' not in lru_cache

So. if using stl, is the delete function to be kept in the implementation or not?

@vdusek
Copy link
Collaborator Author

vdusek commented Oct 10, 2024

Hi @Azathoth-X, if the delete is used only in the tests, I believe we do not need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Code quality improvement or decrease of technical debt. hacktoberfest t-tooling Issues with this label are in the ownership of the tooling team.
Projects
None yet
Development

No branches or pull requests

5 participants