Skip to content
Prev Previous commit
Next Next commit
fixup
  • Loading branch information
TomAugspurger committed Sep 17, 2024
commit 72fb559dae2ec62d92634777fbbf44c1b090af95
5 changes: 1 addition & 4 deletions src/zarr/store/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ def __eq__(self, other: Any) -> bool:
async def make_store_path(
store_like: StoreLike | None,
*,
path: str | None = None,
mode: AccessModeLiteral | None = None,
storage_options: dict[str, Any] | None = None,
) -> StorePath:
Expand All @@ -105,7 +104,7 @@ async def make_store_path(
result = StorePath(await LocalStore.open(root=store_like, mode=mode or "r"))
Copy link
Member

Choose a reason for hiding this comment

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

we can pass **storage_options to LocalStore as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could be missing something, but I don't think that'll work. LocalStore.open will call LocalStore.__init__, which just takes root and mode, which are passed as regular args here.

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean. I was thinking auto_mkdir would be passed through but if that's not the case, let's not get distracted here.

elif isinstance(store_like, str):
storage_options = storage_options or {}
fs, path = fsspec.url_to_fs(store_like, **storage_options)
fs, _ = fsspec.url_to_fs(store_like, **storage_options)
Copy link
Member

Choose a reason for hiding this comment

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

A few thoughts here:

  1. It would be nice to avoid creating the FileSystem if possible. A lighter approach could attempt to parse the string and look for :// (and not file://).
  2. If we do have to create filesystem, it would be nice to reuse it when creating the RemoteStore. Something like RemoteStore.from_filesystem(...) could be a nice pattern.

Copy link
Member

Choose a reason for hiding this comment

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

See below for the way we used to do this. May be worth replicating parts of this

if "://" in store or "::" in store:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed that refactor. It's a bit bigger than I'd like, but the basic version is that RemoteStores's __init__ matches what it uses internally: you can only given it an async filesystem.

There's a from_url and from_path that handles the other stuff that was previously in __init__.

I removed RemoteStore._url. I think that that's safe to remove, but I need to actually think through how it interacted with store.path

if "file" not in fs.protocol:
storage_options = storage_options or {}
result = StorePath(RemoteStore(url=store_like, mode=mode or "r", **storage_options))
Expand All @@ -118,8 +117,6 @@ async def make_store_path(
else:
raise TypeError

if path is not None:
result = result / path
return result


Expand Down