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.

Sign up for GitHub

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

Jump to bottom

fix(remotestore): raise error if path includes scheme #2348

Merged
merged 16 commits into from
Oct 22, 2024
Merged

fix(remotestore): raise error if path includes scheme #2348

merged 16 commits into from
Oct 22, 2024

Conversation

jhamman
Copy link
Member

@jhamman jhamman commented Oct 13, 2024 •
edited
Loading

closes #2342

This PR makes a few important changes to the RemoteStore:

  1. Raise an error if a the provided path has a scheme.
  2. Raise a warning if the Filesystem was not created with asynchronous=True

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)

@jhamman
Copy link
Member Author

jhamman commented Oct 13, 2024

pre-commit.ci autofix

@jhamman jhamman changed the title fix(remotestore): raise error if path includes scheme fix(remotestore): raise error if path includes scheme + don't use dircache Oct 13, 2024
jhamman
jhamman commented Oct 13, 2024
View reviewed changes
src/zarr/storage/remote.py Outdated Show resolved Hide resolved
@jhamman jhamman changed the title fix(remotestore): raise error if path includes scheme + don't use dircache fix(remotestore): raise error if path includes scheme + refresh in fsspec ls/find Oct 14, 2024
@jhamman jhamman changed the base branch from v3 to main October 14, 2024 20:50
@jhamman jhamman requested a review from martindurant October 17, 2024 23:29
@jhamman
Copy link
Member Author

jhamman commented Oct 17, 2024

@martindurant - this is ready for another review.

@jhamman jhamman mentioned this pull request Oct 19, 2024
Closed
4 tasks
martindurant
martindurant reviewed Oct 21, 2024
View reviewed changes
src/zarr/storage/remote.py
f"fs ({fs}) was not created with `asynchronous=True`, this may lead to surprising behavior",
stacklevel=2,
)
if "://" in path and not path.startswith("http"):
Copy link
Member

Choose a reason for hiding this comment

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

Could consider also "::", since that is used in chained fsspec URLs, but should not normally end up passed to the FS. However, it is not as special as "://", so I'm ok not to make a special case for it.

src/zarr/storage/remote.py Outdated Show resolved Hide resolved
src/zarr/storage/remote.py Outdated Show resolved Hide resolved
@jhamman jhamman changed the title fix(remotestore): raise error if path includes scheme + refresh in fsspec ls/find fix(remotestore): raise error if path includes scheme Oct 21, 2024
@jhamman jhamman requested a review from martindurant October 21, 2024 16:56
@jhamman
Copy link
Member Author

jhamman commented Oct 22, 2024

pre-commit.ci autofix

@jhamman jhamman merged commit 5807cba into zarr-developers:main Oct 22, 2024
28 checks passed
@jhamman jhamman deleted the fix/remote-store-path branch October 22, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martindurant martindurant martindurant approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RemoteStore doesn't strip protocol correctly, breaking list operations
2 participants
@jhamman @martindurant