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

[Feature] Add support for using select user-defined zarr stores #62

Merged
merged 42 commits into from
Jan 18, 2023

Conversation

oruebel
Copy link
Contributor

@oruebel oruebel commented Jan 5, 2023

Motivation

Currently ZarrIO only supports Zarr's the default DirectoryStore. It would be nice to add support for additional data stores.

  • Allow custom DirectoryStore, NestedDirectoryStore and TempStore for read and write in ZarrIO
  • Update test_zarr_io.py to support specification of backend stores and add unit tests based on test_zarr_io.py for the new stores.
  • Update handling of references on read to support file-based stores (e.g., ZipStore or database stores. This will require updating the read logic to open target object not via the full path, but by opening the Zarr file itself and then retrieving the linked object.
  • Update NWBZarrIO to support the new options of ZarrIO
  • Add export tests based on test_io_convert.py for the new stores
  • Add developer documentation for how to integrate new storage backends with ZarrIO
  • Update ZarrIO tutorial (and maybe the NWB tutorial) to show how to write using custom Zarr stores

@oruebel oruebel added category: enhancement improvements of code or code behavior priority: low alternative solution already working and/or relevant to only specific user(s) labels Jan 5, 2023
@oruebel oruebel added this to the Next Release milestone Jan 6, 2023
@oruebel
Copy link
Contributor Author

oruebel commented Jan 6, 2023

I will split this into two PRs. Allowing TempStore, DirectoryStore, and NestedDirectoryStore is much simpler as those all derive from DirectoryStore. Adding the more file- and database-oriented stores, e.g., zip or SQL, will be more work with regard to support for links and references (especially during export and when linking across Zarr files with different backend stores). I created issue #65 to explore adding more stores.

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2023

Codecov Report

Base: 80.57% // Head: 82.03% // Increases project coverage by +1.45% 🎉

Coverage data is based on head (cfd0788) compared to base (42134e2).
Patch coverage: 98.20% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev      #62      +/-   ##
==========================================
+ Coverage   80.57%   82.03%   +1.45%     
==========================================
  Files          10       11       +1     
  Lines        2435     2644     +209     
==========================================
+ Hits         1962     2169     +207     
- Misses        473      475       +2     
Impacted Files Coverage Δ
src/hdmf_zarr/backend.py 88.14% <90.90%> (+0.51%) ⬆️
tests/unit/base_tests_zarrio.py 98.73% <98.48%> (ø)
src/hdmf_zarr/nwb.py 34.28% <100.00%> (-1.83%) ⬇️
tests/unit/test_io_convert.py 97.86% <100.00%> (+0.75%) ⬆️
tests/unit/test_zarrio.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@oruebel oruebel marked this pull request as ready for review January 7, 2023 13:41
@oruebel oruebel requested a review from mavaylon1 January 7, 2023 13:42
@oruebel oruebel changed the title Add support for using select user-defined zarr stores [Feature] Add support for using select user-defined zarr stores Jan 8, 2023
@oruebel
Copy link
Contributor Author

oruebel commented Jan 11, 2023

@rly @mavaylon1 this PR is ready for review

@oruebel
Copy link
Contributor Author

oruebel commented Jan 17, 2023

Note, the external link check is failing here, because it includes a link to a file that is being added to the main repo by this PR. I.e., once this PR is merged that link should be correct.

tests/unit/test_io_convert.py Outdated Show resolved Hide resolved
tests/unit/test_io_convert.py Outdated Show resolved Hide resolved
@oruebel
Copy link
Contributor Author

oruebel commented Jan 18, 2023

@rly thanks for the suggestions. I have now resolved all the items and this PR is now ready for you to take another look at for review. Specifically I:

  • Removed the unused filepath parameter from the ZarrIO.get_builder_exists_on_disk
  • Increased the HDMF version to 3.5
  • Fixed the documentation of class members of test mixin classes
  • Consistently close IO objects in tests that we explicitly open
  • Updated the documentation to integrate new data stores

@rly
Copy link
Contributor

rly commented Jan 18, 2023

Looks good to me. Thanks for the changes!

@oruebel oruebel merged commit f59da74 into dev Jan 18, 2023
@oruebel oruebel deleted the add/alternate_stores branch January 18, 2023 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: enhancement improvements of code or code behavior priority: low alternative solution already working and/or relevant to only specific user(s) topic: storage issues related to storage schema and formats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants