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

Prune stores #1791

Open
wants to merge 30 commits into
base: v3
Choose a base branch
from
Open

Prune stores #1791

wants to merge 30 commits into from

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Apr 11, 2024

Removes a lot of stores from the v2 codebase. I basically removed every store that required an extra dependency (other than fsspec), and also removed the N5 stores and the sqlite store. The logic for doing this is to thin down the v2 codebase.

Depends on #1742.

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)

d-v-b and others added 12 commits April 4, 2024 13:14
* refactor(v3): Using appropriate types

* fix(v3): Typing fixes + minor code fixes

* fix(v3): _sync_iter works with coroutines

* docs(v3/store/core.py): clearer comment

* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic

* fix(zarr/v3): correct zarr format + remove unused method

* fix(v3/store/core.py): Potential suggestion on handling str store_like

* refactor(zarr/v3): Add more typing

* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
…elopers#1728)

* Specify v3 hatch envs using GitHub actions matrix

* Update .github/workflows/test-v3.yml

Co-authored-by: Joe Hamman <jhamman1@gmail.com>

* Update .github/workflows/test-v3.yml

Co-authored-by: Joe Hamman <jhamman1@gmail.com>

* test on 3.12 too

* no 3.12

---------

Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
* black -> ruff + cleanup

* format

* Preserve git blame

* pre-commit fix
…ntributing.rst (zarr-developers#1643)

Co-authored-by: Joe Hamman <joe@earthmover.io>
@d-v-b d-v-b added in progress Someone is currently working on this V3 Related to compatibility with V3 spec labels Apr 11, 2024
@d-v-b
Copy link
Contributor Author

d-v-b commented Apr 19, 2024

This PR specifically removes following the following v2 stores, and their v3 counterparts:

One question that @jhamman raised was whether we want to remove ZipStore or not. Since FSSpec supports Zip archives as a backend, is it possible that we could get ZipStore functionality via FSStore? @martindurant what do you think?

Once we decide on whether or not we keep ZipStore, then I think this PR is good to go, barring any other feedback / concerns.

@rabernat
Copy link
Contributor

My feeling is that Zip stores are really an underappreciated feature of Zarr. Whatever we choose in terms of design, we should document Zip stores prominently.

I'm curious--are there performance or feature differences between the native Zip store and the fsspec version?

@martindurant
Copy link
Member

fsspec's ZipFS ought to be able to do everything, I would have thought, with the additional plus of being able to work with remote zips too (usually "r" and "w" modes only). It has certainly not been tested for speed, convenience being the priority over performance. A more specialised, and therefore simple, implementation can be reasonable if there is someone to maintain it.

@mxmlnkn , you might be interested by another way of thinking of compressed bunch-of-files containers discussed here.

@jhamman jhamman added this to the 3.0.0.alpha milestone Apr 22, 2024
@d-v-b
Copy link
Contributor Author

d-v-b commented Apr 22, 2024

Updates:

  • My initial effort in this PR removed everything in one commit. I have reverted that, and broken the removals into one commit per storage class.
  • I am keeping ZipStore, since it doesn't require any external dependencies and some community members expressed an interested in keeping it.

I think the ZipStore was the only controversial aspect here, and that issue should be sorted. I would like to merge this in the next few days, unless anyone has objections.

@d-v-b d-v-b removed the in progress Someone is currently working on this label Apr 23, 2024
@d-v-b d-v-b mentioned this pull request May 21, 2024
6 tasks
@jhamman jhamman modified the milestones: 3.0.0.alpha, 3.0.0 May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V3 Related to compatibility with V3 spec
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

None yet

8 participants