Skip to content

Feat: improves delete_dir for s3fs-backed FsspecStore#2661

Merged
dcherian merged 20 commits into
zarr-developers:mainfrom
carshadi:feat-fsspecstore-bulk-delete
Feb 14, 2025
Merged

Feat: improves delete_dir for s3fs-backed FsspecStore#2661
dcherian merged 20 commits into
zarr-developers:mainfrom
carshadi:feat-fsspecstore-bulk-delete

Conversation

@carshadi

@carshadi carshadi commented Jan 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Improves performance of FsspecStore.delete_dir when underlying fs is s3fs, by passing a list of filepaths to be removed in bulk instead of one-by-one.

Resolves #2659

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)

carshadi and others added 2 commits January 6, 2025 14:38
@carshadi
Implement asynchronous directory deletion in FsspecStore
11ac3d9 
- override Store.delete_dir default method, which deletes keys one by one, to support bulk deletion for fsspec implementations that support a list of paths in the fs._rm method.
- This can greatly reduce the number of requests to S3, which reduces likelihood of running into throttling errors and improves delete performance.
- Currently, only s3fs is supported.
@carshadi
Merge branch 'zarr-developers:main' into feat-fsspecstore-bulk-delete
32d8bc9
@jhamman jhamman requested a review from martindurant January 6, 2025 21:55
d-v-b
d-v-b reviewed Jan 7, 2025
View reviewed changes
Comment thread src/zarr/storage/_fsspec.py Outdated
d-v-b
d-v-b reviewed Jan 7, 2025
View reviewed changes
Comment thread src/zarr/storage/_fsspec.py Outdated
d-v-b
d-v-b reviewed Jan 7, 2025
View reviewed changes
Comment thread src/zarr/storage/_fsspec.py Outdated
d-v-b
d-v-b reviewed Jan 7, 2025
View reviewed changes
Comment thread src/zarr/storage/_fsspec.py Outdated
martindurant
martindurant reviewed Jan 7, 2025
View reviewed changes

@martindurant martindurant left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have reservations for this code. It seems to me, that calling ._rm() should be all that's required, and let fsspec handle everything else.

Comment thread src/zarr/storage/_fsspec.py Outdated
Comment thread src/zarr/storage/_fsspec.py Outdated
Comment thread src/zarr/storage/_fsspec.py Outdated
@carshadi carshadi requested review from d-v-b and martindurant January 8, 2025 18:00
martindurant
martindurant reviewed Jan 8, 2025
View reviewed changes
Comment thread src/zarr/storage/_fsspec.py Outdated
@dstansby dstansby added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 9, 2025
@github-actions github-actions Bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Jan 10, 2025
jhamman
jhamman requested changes Jan 24, 2025
View reviewed changes

@jhamman jhamman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @carshadi for continued efforts here. I'd like to see some fsspec specific tests here if possible.

Comment thread src/zarr/storage/_fsspec.py Outdated
@github-actions github-actions Bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 24, 2025
@carshadi

carshadi commented Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

Thanks @carshadi for continued efforts here. I'd like to see some fsspec specific tests here if possible.

@jhamman added a few test cases. Let me know if there are others that would be useful.

@carshadi @jhamman
Update src/zarr/storage/_fsspec.py
6ad9a0a 
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
jhamman
jhamman approved these changes Jan 29, 2025
View reviewed changes

@jhamman jhamman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good here!

@carshadi - the only thing this needs is a release note.

@martindurant - care to do the final review and/or merge?

martindurant
martindurant reviewed Jan 29, 2025
View reviewed changes
Comment thread src/zarr/storage/_fsspec.py Outdated
Comment thread src/zarr/storage/_fsspec.py Outdated
Comment thread tests/test_store/test_fsspec.py Outdated
Comment thread tests/test_store/test_fsspec.py
@github-actions github-actions Bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Feb 14, 2025
@carshadi

carshadi commented Feb 14, 2025

Copy link
Copy Markdown
Contributor Author

Hi @martindurant , @jhamman , let me know if there's anything else I can do here. thanks

@martindurant

martindurant commented Feb 14, 2025

Copy link
Copy Markdown
Member

+1

@dcherian dcherian enabled auto-merge (squash) February 14, 2025 18:52
@dcherian dcherian merged commit 48f7c9a into zarr-developers:main Feb 14, 2025
@carshadi carshadi deleted the feat-fsspecstore-bulk-delete branch February 14, 2025 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martindurant martindurant martindurant left review comments
@jhamman jhamman jhamman approved these changes
@d-v-b d-v-b Awaiting requested review from d-v-b

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

FsspecStore directory deletion performance improvements

6 participants

@carshadi @martindurant @jhamman @d-v-b @dcherian @dstansby