Repository: zarr-developers/zarr-python

Issue:
RemoteStore doesn't strip protocol correctly, breaking list operations
I've discovered _what I thought was_ a pretty major gap in our support for remote storage. Instead it turns out to be a weird behavior around prefix and protocol stripping with path urls.

Unfortunately there is no way to reproduce this afaict without write access to an actual s3 bucket because RemoteStore requires an async filesystem... ~Which makes me think we are currently not testing RemoteStore at all? 🤔~ Edit: not true. 

```python
import s3fs
import zarr
s3 = s3fs.S3FileSystem()

# replace with a bucket you can write to
target_url = "s3://icechunk-test/ryan/zarr3-tests/groups/1"
store = zarr.storage.RemoteStore(s3, mode="w", path=target_url)
# create a group
g = zarr.group(store=store, zarr_version=3)
# create a child
a = g.create("foo", shape=10, dtype='i4')

# try to discover children
print(list(g))
print(g.members())
[i for i in g.arrays()]
```

All of these return an empty list, along with the warning
```
Object at icechunk-test/ryan/zarr3-tests/groups/1/foo is not recognized as a component of a Zarr hierarchy.
Object at icechunk-test/ryan/zarr3-tests/groups/1/zarr.json is not recognized as a component of a Zarr hierarchy.
```

Unfortunately, this means that Xarray can't discover the arrays in a group and so can't open any RemoteStore datasets with zarr version >= 3.

https://github.com/pydata/xarray/blob/70a2a55afb4a73a4d5027506ea87f918949e2b7c/xarray/backends/zarr.py#L627C61-L627C85

I'm on version 3.0.0a8.dev12+gff530c36

Discussion:
jhamman: pre-commit.ci autofix
jhamman: @martindurant - this is ready for another review.
jhamman: pre-commit.ci autofix

Patch:
diff --git a/docs/guide/storage.rst b/docs/guide/storage.rst
index dfda553c..0019f993 100644
--- a/docs/guide/storage.rst
+++ b/docs/guide/storage.rst
@@ -72,7 +72,7 @@ that implements the `AbstractFileSystem` API,
 .. code-block:: python
 
    >>> import zarr
-   >>> store = zarr.storage.RemoteStore("gs://foo/bar", mode="r")
+   >>> store = zarr.storage.RemoteStore.from_url("gs://foo/bar", mode="r")
    >>> zarr.open(store=store)
    <Array <RemoteStore(GCSFileSystem, foo/bar)> shape=(10, 20) dtype=float32>
 
diff --git a/src/zarr/abc/store.py b/src/zarr/abc/store.py
index a995a6bf..045da7e8 100644
--- a/src/zarr/abc/store.py
+++ b/src/zarr/abc/store.py
@@ -168,7 +168,7 @@ class Store(ABC):
 
         Returns
         -------
-        store:
+        store
             A new store of the same type with the new mode.
 
         Examples
diff --git a/src/zarr/storage/remote.py b/src/zarr/storage/remote.py
index 812b1e24..1f7d5f7a 100644
--- a/src/zarr/storage/remote.py
+++ b/src/zarr/storage/remote.py
@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import warnings
 from typing import TYPE_CHECKING, Any, Self
 
 from zarr.abc.store import ByteRangeRequest, Store
@@ -32,7 +33,8 @@ class RemoteStore(Store):
     mode : AccessModeLiteral
         The access mode to use.
     path : str
-        The root path of the store.
+        The root path of the store. This should be a relative path and must not include the
+        filesystem scheme.
     allowed_exceptions : tuple[type[Exception], ...]
         When fetching data, these cases will be deemed to correspond to missing keys.
 
@@ -44,6 +46,23 @@ class RemoteStore(Store):
     supports_deletes
     supports_partial_writes
     supports_listing
+
+    Raises
+    ------
+    TypeError
+        If the Filesystem does not support async operations.
+    ValueError
+        If the path argument includes a scheme.
+
+    Warns
+    -----
+    UserWarning
+        If the file system (fs) was not created with `asynchronous=True`.
+
+    See Also
+    --------
+    RemoteStore.from_upath
+    RemoteStore.from_url
     """
 
     # based on FSSpec
@@ -69,6 +88,15 @@ class RemoteStore(Store):
 
         if not self.fs.async_impl:
             raise TypeError("Filesystem needs to support async operations.")
+        if not self.fs.asynchronous:
+            warnings.warn(
+                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"):
+            # `not path.startswith("http")` is a special case for the http filesystem (¯\_(ツ)_/¯)
+            scheme, _ = path.split("://", maxsplit=1)
+            raise ValueError(f"path argument to RemoteStore must not include scheme ({scheme}://)")
 
     @classmethod
     def from_upath(
@@ -134,7 +162,17 @@ class RemoteStore(Store):
             # before fsspec==2024.3.1
             from fsspec.core import url_to_fs
 
-        fs, path = url_to_fs(url, **storage_options)
+        opts = storage_options or {}
+        opts = {"asynchronous": True, **opts}
+
+        fs, path = url_to_fs(url, **opts)
+
+        # fsspec is not consistent about removing the scheme from the path, so check and strip it here
+        # https://github.com/fsspec/filesystem_spec/issues/1722
+        if "://" in path and not path.startswith("http"):
+            # `not path.startswith("http")` is a special case for the http filesystem (¯\_(ツ)_/¯)
+            path = fs._strip_protocol(path)
+
         return cls(fs=fs, path=path, mode=mode, allowed_exceptions=allowed_exceptions)
 
     async def clear(self) -> None:


Test patch:
diff --git a/tests/test_store/test_core.py b/tests/test_store/test_core.py
index 771dc3c4..38292e23 100644
--- a/tests/test_store/test_core.py
+++ b/tests/test_store/test_core.py
@@ -71,10 +71,7 @@ async def test_make_store_path_invalid() -> None:
 
 
 async def test_make_store_path_fsspec(monkeypatch) -> None:
-    import fsspec.implementations.memory
-
-    monkeypatch.setattr(fsspec.implementations.memory.MemoryFileSystem, "async_impl", True)
-    store_path = await make_store_path("memory://")
+    store_path = await make_store_path("http://foo.com/bar")
     assert isinstance(store_path.store, RemoteStore)
 
 
diff --git a/tests/test_store/test_remote.py b/tests/test_store/test_remote.py
index c8e9a162..2ad1fd78 100644
--- a/tests/test_store/test_remote.py
+++ b/tests/test_store/test_remote.py
@@ -86,10 +86,12 @@ def s3(s3_base: None) -> Generator[s3fs.S3FileSystem, None, None]:
 
 async def test_basic() -> None:
     store = RemoteStore.from_url(
-        f"s3://{test_bucket_name}",
+        f"s3://{test_bucket_name}/foo/spam/",
         mode="w",
         storage_options={"endpoint_url": endpoint_url, "anon": False},
     )
+    assert store.fs.asynchronous
+    assert store.path == f"{test_bucket_name}/foo/spam"
     assert await _collect_aiterator(store.list()) == ()
     assert not await store.exists("foo")
     data = b"hello"
@@ -109,7 +111,7 @@ class TestRemoteStoreS3(StoreTests[RemoteStore, cpu.Buffer]):
     @pytest.fixture
     def store_kwargs(self, request) -> dict[str, str | bool]:
         fs, path = fsspec.url_to_fs(
-            f"s3://{test_bucket_name}", endpoint_url=endpoint_url, anon=False
+            f"s3://{test_bucket_name}", endpoint_url=endpoint_url, anon=False, asynchronous=True
         )
         return {"fs": fs, "path": path, "mode": "r+"}
 
@@ -143,9 +145,7 @@ class TestRemoteStoreS3(StoreTests[RemoteStore, cpu.Buffer]):
     def test_store_supports_listing(self, store: RemoteStore) -> None:
         assert store.supports_listing
 
-    async def test_remote_store_from_uri(
-        self, store: RemoteStore, store_kwargs: dict[str, str | bool]
-    ):
+    async def test_remote_store_from_uri(self, store: RemoteStore):
         storage_options = {
             "endpoint_url": endpoint_url,
             "anon": False,
@@ -183,9 +183,32 @@ class TestRemoteStoreS3(StoreTests[RemoteStore, cpu.Buffer]):
         assert dict(group.attrs) == {"key": "value-3"}
 
     def test_from_upath(self) -> None:
-        path = UPath(f"s3://{test_bucket_name}", endpoint_url=endpoint_url, anon=False)
+        path = UPath(
+            f"s3://{test_bucket_name}/foo/bar/",
+            endpoint_url=endpoint_url,
+            anon=False,
+            asynchronous=True,
+        )
         result = RemoteStore.from_upath(path)
         assert result.fs.endpoint_url == endpoint_url
+        assert result.fs.asynchronous
+        assert result.path == f"{test_bucket_name}/foo/bar"
+
+    def test_init_raises_if_path_has_scheme(self, store_kwargs) -> None:
+        # regression test for https://github.com/zarr-developers/zarr-python/issues/2342
+        store_kwargs["path"] = "s3://" + store_kwargs["path"]
+        with pytest.raises(
+            ValueError, match="path argument to RemoteStore must not include scheme .*"
+        ):
+            self.store_cls(**store_kwargs)
+
+    def test_init_warns_if_fs_asynchronous_is_false(self) -> None:
+        fs, path = fsspec.url_to_fs(
+            f"s3://{test_bucket_name}", endpoint_url=endpoint_url, anon=False, asynchronous=False
+        )
+        store_kwargs = {"fs": fs, "path": path, "mode": "r+"}
+        with pytest.warns(UserWarning, match=r".* was not created with `asynchronous=True`.*"):
+            self.store_cls(**store_kwargs)
 
     async def test_empty_nonexistent_path(self, store_kwargs) -> None:
         # regression test for https://github.com/zarr-developers/zarr-python/pull/2343