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

LRUStoreCache: cache "contains" by contains checks #1499

Merged

Conversation

madsbk
Copy link
Contributor

@madsbk madsbk commented Aug 14, 2023

Fixes #1497 by caching LRUStoreCache.__contains__ calls individually.

Apparently in ZarrTiffStore, a successful "contains" check of key doesn't necessarily means that key is in ZarrTiffStore.keys().

E.g. if key is a string and not in ZarrTiffStore.keys(), it still might be "contained" by ZarrTiffStore, see https://github.com/cgohlke/tifffile/blob/master/tifffile/tifffile.py#L12222

I guess that is why LRUStoreCache has both a key and a contains cache?

TODO:

  • Add unit tests and/or doctests in docstrings
  • [ ] Add docstrings and API docs for any new/modified user-facing classes and functions
  • [ ] New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

cc. @cgohlke

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Aug 14, 2023
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #1499 (d2c7ef9) into main (f542fca) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1499   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           37        37           
  Lines        14729     14735    +6     
=========================================
+ Hits         14729     14735    +6     
Files Changed Coverage Δ
zarr/_storage/v3.py 100.00% <100.00%> (ø)
zarr/storage.py 100.00% <100.00%> (ø)
zarr/tests/test_storage.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@madsbk madsbk marked this pull request as ready for review August 14, 2023 11:50
@cgohlke
Copy link
Contributor

cgohlke commented Aug 14, 2023

Thank you. I can confirm that this fixes issue #1497.

@rabernat
Copy link
Contributor

Apparently in ZarrTiffStore, a successful "contains" check of key doesn't necessarily means that key is in ZarrTiffStore.keys().

Could someone elaborate on why this is? It seems inconsistent--certainly different from how a normal mutable mapping works.

@cgohlke
Copy link
Contributor

cgohlke commented Aug 14, 2023

Apparently in ZarrTiffStore, a successful "contains" check of key doesn't necessarily means that key is in ZarrTiffStore.keys().

Could someone elaborate on why this is? It seems inconsistent--certainly different from how a normal mutable mapping works.

Good question.

  1. So far it has not been required AFAIK. Although I felt uneasy to rely on it without documentation.
  2. It seems excessive (to the point being unusable) to have keys() return potentially many millions of short strings to test one membership.
  3. It is not simple to check if a chunk is actually contained in a TIFF file (https://github.com/cgohlke/tifffile/blob/7b162f968bc978bd374dfe87f05a913e82afd283/tifffile/tifffile.py#L12973-L13018)

Copy link
Contributor

@rabernat rabernat left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

This seems like an acceptable solution to me. I still find it strange that __contains__ and keys() return inconsistent results, but I accept that behavior is desired for some stores.

@madsbk, please complete the TODO checklist (or cross out irrelevant items).

@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Aug 15, 2023
@madsbk
Copy link
Contributor Author

madsbk commented Aug 16, 2023

Added release notes, I think this can be merged

@rabernat rabernat merged commit 7017ce1 into zarr-developers:main Aug 16, 2023
21 checks 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.

tifffile.ZarrTiffStore wrapped in zarr.LRUStoreCache fails to read any chunks
3 participants