Repository: fsspec/filesystem_spec
Pull request #1673 (2024-09-16 14:23:15)

allow repeated extra arguments
This fixes the error happening when the user passes extra parameters that are also inferred automatically. E.g., happens in lib `datasets`:
```
File ./python-default/3.10.14/lib/python3.10/site-packages/datasets/load.py:2692, in load_from_disk(dataset_path, fs, keep_in_memory, storage_options)
   2689     storage_options = fs.storage_options
   2691 fs: fsspec.AbstractFileSystem
-> 2692 fs, *_ = url_to_fs(dataset_path, **(storage_options or {}))
   2693 if not fs.exists(dataset_path):
   2694     raise FileNotFoundError(f"Directory {dataset_path} not found")

File ./3.10.14/lib/python3.10/site-packages/fsspec/core.py:396, in url_to_fs(url, **kwargs)
    385 known_kwargs = {
    386     "compression",
    387     "encoding",
   (...)
    393     "num",
    394 }
    395 kwargs = {k: v for k, v in kwargs.items() if k not in known_kwargs}
--> 396 chain = _un_chain(url, kwargs)
    397 inkwargs = {}
    398 # Reverse iterate the chain, creating a nested target_* structure

File ./3.10.14/lib/python3.10/site-packages/fsspec/core.py:349, in _un_chain(path, kwargs)
    347 if bit is bits[0]:
    348     kws.update(kwargs)
--> 349 kw = dict(**extra_kwargs, **kws)
    350 bit = cls._strip_protocol(bit)
    351 if (
    352     protocol in {"blockcache", "filecache", "simplecache"}
    353     and "target_protocol" not in kw
    354 ):

TypeError: dict() got multiple values for keyword argument 'account_name'
```

### This fix

```python
from fsspec.core import url_to_fs
url_to_fs('az://DIR@ACCOUNT.blob.core.windows.net/DATA', **{'anon': False, 'account_name': 'ACCOUNT'})
```
Out before:
```
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[1], line 2
      1 from fsspec.core import url_to_fs
----> 2 url_to_fs('az://DIR@ACCOUNT.blob.core.windows.net/DATA', **{'anon': False, 'account_name': 'ACCOUNT'})

...
...
    347 if bit is bits[0]:
    348     kws.update(kwargs)
--> 349 kw = dict(**extra_kwargs, **kws)
    350 bit = cls._strip_protocol(bit)
    351 if (
    352     protocol in {"blockcache", "filecache", "simplecache"}
    353     and "target_protocol" not in kw
    354 ):

TypeError: dict() got multiple values for keyword argument 'account_name'
```


Out after:
```
(<adlfs.spec.AzureBlobFileSystem at 0x10708f460>, 'DIR/DATA')
```

martindurant: Merging in recent PRs will make CI pass

Patch:
diff --git a/fsspec/core.py b/fsspec/core.py
index de66be2..1954667 100644
--- a/fsspec/core.py
+++ b/fsspec/core.py
@@ -346,7 +346,10 @@ def _un_chain(path, kwargs):
         kws = kwargs.pop(protocol, {})
         if bit is bits[0]:
             kws.update(kwargs)
-        kw = dict(**extra_kwargs, **kws)
+        kw = dict(
+            **{k: v for k, v in extra_kwargs.items() if k not in kws or v != kws[k]},
+            **kws,
+        )
         bit = cls._strip_protocol(bit)
         if (
             protocol in {"blockcache", "filecache", "simplecache"}
@@ -578,7 +581,7 @@ def expand_paths_if_needed(paths, mode, num, fs, name_function):
     paths = list(paths)
 
     if "w" in mode:  # read mode
-        if sum([1 for p in paths if "*" in p]) > 1:
+        if sum(1 for p in paths if "*" in p) > 1:
             raise ValueError(
                 "When writing data, only one filename mask can be specified."
             )
diff --git a/fsspec/implementations/git.py b/fsspec/implementations/git.py
index 7c34d93..7b9d353 100644
--- a/fsspec/implementations/git.py
+++ b/fsspec/implementations/git.py
@@ -55,6 +55,8 @@ class GitFileSystem(AbstractFileSystem):
         tree = comm.tree
         for part in parts:
             if part and isinstance(tree, pygit2.Tree):
+                if part not in tree:
+                    raise FileNotFoundError(path)
                 tree = tree[part]
         return tree
 
@@ -69,46 +71,32 @@ class GitFileSystem(AbstractFileSystem):
             out["ref"], path = path.split("@", 1)
         return out
 
+    @staticmethod
+    def _object_to_info(obj, path=None):
+        # obj.name and obj.filemode are None for the root tree!
+        is_dir = isinstance(obj, pygit2.Tree)
+        return {
+            "type": "directory" if is_dir else "file",
+            "name": (
+                "/".join([path, obj.name or ""]).lstrip("/") if path else obj.name
+            ),
+            "hex": str(obj.id),
+            "mode": "100644" if obj.filemode is None else f"{obj.filemode:o}",
+            "size": 0 if is_dir else obj.size,
+        }
+
     def ls(self, path, detail=True, ref=None, **kwargs):
-        path = self._strip_protocol(path)
-        tree = self._path_to_object(path, ref)
-        if isinstance(tree, pygit2.Tree):
-            out = []
-            for obj in tree:
-                if isinstance(obj, pygit2.Tree):
-                    out.append(
-                        {
-                            "type": "directory",
-                            "name": "/".join([path, obj.name]).lstrip("/"),
-                            "hex": obj.hex,
-                            "mode": f"{obj.filemode:o}",
-                            "size": 0,
-                        }
-                    )
-                else:
-                    out.append(
-                        {
-                            "type": "file",
-                            "name": "/".join([path, obj.name]).lstrip("/"),
-                            "hex": obj.hex,
-                            "mode": f"{obj.filemode:o}",
-                            "size": obj.size,
-                        }
-                    )
-        else:
-            obj = tree
-            out = [
-                {
-                    "type": "file",
-                    "name": obj.name,
-                    "hex": obj.hex,
-                    "mode": f"{obj.filemode:o}",
-                    "size": obj.size,
-                }
-            ]
-        if detail:
-            return out
-        return [o["name"] for o in out]
+        tree = self._path_to_object(self._strip_protocol(path), ref)
+        return [
+            GitFileSystem._object_to_info(obj, path)
+            if detail
+            else GitFileSystem._object_to_info(obj, path)["name"]
+            for obj in (tree if isinstance(tree, pygit2.Tree) else [tree])
+        ]
+
+    def info(self, path, ref=None, **kwargs):
+        tree = self._path_to_object(self._strip_protocol(path), ref)
+        return GitFileSystem._object_to_info(tree, path)
 
     def ukey(self, path, ref=None):
         return self.info(path, ref=ref)["hex"]
diff --git a/fsspec/implementations/memory.py b/fsspec/implementations/memory.py
index 93860af..3b6f0d2 100644
--- a/fsspec/implementations/memory.py
+++ b/fsspec/implementations/memory.py
@@ -248,6 +248,10 @@ class MemoryFileSystem(AbstractFileSystem):
         except KeyError as e:
             raise FileNotFoundError(path) from e
 
+    def isfile(self, path):
+        path = self._strip_protocol(path)
+        return path in self.store
+
     def rm(self, path, recursive=False, maxdepth=None):
         if isinstance(path, str):
             path = self._strip_protocol(path)
@@ -255,14 +259,14 @@ class MemoryFileSystem(AbstractFileSystem):
             path = [self._strip_protocol(p) for p in path]
         paths = self.expand_path(path, recursive=recursive, maxdepth=maxdepth)
         for p in reversed(paths):
+            if self.isfile(p):
+                self.rm_file(p)
             # If the expanded path doesn't exist, it is only because the expanded
             # path was a directory that does not exist in self.pseudo_dirs. This
             # is possible if you directly create files without making the
             # directories first.
-            if not self.exists(p):
+            elif not self.exists(p):
                 continue
-            if self.isfile(p):
-                self.rm_file(p)
             else:
                 self.rmdir(p)
 
diff --git a/fsspec/implementations/reference.py b/fsspec/implementations/reference.py
index 6904d7b..d5d3f29 100644
--- a/fsspec/implementations/reference.py
+++ b/fsspec/implementations/reference.py
@@ -7,7 +7,7 @@ import math
 import os
 from itertools import chain
 from functools import lru_cache
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Literal
 
 import fsspec.core
 
@@ -104,7 +104,13 @@ class LazyReferenceMapper(collections.abc.MutableMapping):
         return pd
 
     def __init__(
-        self, root, fs=None, out_root=None, cache_size=128, categorical_threshold=10
+        self,
+        root,
+        fs=None,
+        out_root=None,
+        cache_size=128,
+        categorical_threshold=10,
+        engine: Literal["fastparquet", "pyarrow"] = "fastparquet",
     ):
         """
 
@@ -126,16 +132,25 @@ class LazyReferenceMapper(collections.abc.MutableMapping):
             Encode urls as pandas.Categorical to reduce memory footprint if the ratio
             of the number of unique urls to total number of refs for each variable
             is greater than or equal to this number. (default 10)
+        engine: Literal["fastparquet","pyarrow"]
+            Engine choice for reading parquet files. (default is "fastparquet")
         """
+
         self.root = root
         self.chunk_sizes = {}
         self.out_root = out_root or self.root
         self.cat_thresh = categorical_threshold
+        self.engine = engine
         self.cache_size = cache_size
         self.url = self.root + "/{field}/refs.{record}.parq"
         # TODO: derive fs from `root`
         self.fs = fsspec.filesystem("file") if fs is None else fs
 
+        from importlib.util import find_spec
+
+        if self.engine == "pyarrow" and find_spec("pyarrow") is None:
+            raise ImportError("engine choice `pyarrow` is not installed.")
+
     def __getattr__(self, item):
         if item in ("_items", "record_size", "zmetadata"):
             self.setup()
@@ -158,7 +173,7 @@ class LazyReferenceMapper(collections.abc.MutableMapping):
             """cached parquet file loader"""
             path = self.url.format(field=field, record=record)
             data = io.BytesIO(self.fs.cat_file(path))
-            df = self.pd.read_parquet(data, engine="fastparquet")
+            df = self.pd.read_parquet(data, engine=self.engine)
             refs = {c: df[c].to_numpy() for c in df.columns}
             return refs
 
@@ -463,18 +478,28 @@ class LazyReferenceMapper(collections.abc.MutableMapping):
 
         fn = f"{base_url or self.out_root}/{field}/refs.{record}.parq"
         self.fs.mkdirs(f"{base_url or self.out_root}/{field}", exist_ok=True)
+
+        if self.engine == "pyarrow":
+            df_backend_kwargs = {"write_statistics": False}
+        elif self.engine == "fastparquet":
+            df_backend_kwargs = {
+                "stats": False,
+                "object_encoding": object_encoding,
+                "has_nulls": has_nulls,
+            }
+        else:
+            raise NotImplementedError(f"{self.engine} not supported")
+
         df.to_parquet(
             fn,
-            engine="fastparquet",
+            engine=self.engine,
             storage_options=storage_options
             or getattr(self.fs, "storage_options", None),
             compression="zstd",
             index=False,
-            stats=False,
-            object_encoding=object_encoding,
-            has_nulls=has_nulls,
-            # **kwargs,
+            **df_backend_kwargs,
         )
+
         partition.clear()
         self._items.pop((field, record))
 
@@ -486,6 +511,7 @@ class LazyReferenceMapper(collections.abc.MutableMapping):
         base_url: str
             Location of the output
         """
+
         # write what we have so far and clear sub chunks
         for thing in list(self._items):
             if isinstance(thing, tuple):
diff --git a/fsspec/spec.py b/fsspec/spec.py
index 8229170..8284366 100644
--- a/fsspec/spec.py
+++ b/fsspec/spec.py
@@ -428,11 +428,9 @@ class AbstractFileSystem(metaclass=_Cached):
         except (FileNotFoundError, OSError) as e:
             if on_error == "raise":
                 raise
-            elif callable(on_error):
+            if callable(on_error):
                 on_error(e)
-            if detail:
-                return path, {}, {}
-            return path, [], []
+            return
 
         for info in listing:
             # each info name must be at least [path]/part , but here
@@ -650,7 +648,7 @@ class AbstractFileSystem(metaclass=_Cached):
         Returns a single dictionary, with exactly the same information as ``ls``
         would with ``detail=True``.
 
-        The default implementation should calls ls and could be overridden by a
+        The default implementation calls ls and could be overridden by a
         shortcut. kwargs are passed on to ```ls()``.
 
         Some file systems might not be able to measure the file's size, in


Test patch:
diff --git a/fsspec/implementations/tests/test_git.py b/fsspec/implementations/tests/test_git.py
index ffa7b47..2aeb544 100644
--- a/fsspec/implementations/tests/test_git.py
+++ b/fsspec/implementations/tests/test_git.py
@@ -61,6 +61,39 @@ def test_refs(repo):
         assert f.read() == b"data3"
 
 
+def _check_FileNotFoundError(f, *args, **kwargs):
+    with pytest.raises(FileNotFoundError):
+        f(*args, **kwargs)
+
+
+def test_file_existence_checks(repo):
+    d, sha = repo
+
+    fs, _ = fsspec.url_to_fs(f"git://{d}:abranch@")
+
+    assert fs.lexists("inner")
+    assert fs.exists("inner")
+    assert fs.isdir("inner")
+    assert fs.info("inner")
+    assert fs.ls("inner")
+
+    assert fs.lexists("inner/file1")
+    assert fs.exists("inner/file1")
+    assert fs.info("inner/file1")
+    assert fs.ls("inner/file1")
+
+    assert not fs.lexists("non-existing-file")
+    assert not fs.exists("non-existing-file")
+
+    assert not fs.isfile("non-existing-file")
+    assert not fs.isdir("non-existing-file")
+
+    _check_FileNotFoundError(fs.info, "non-existing-file")
+    _check_FileNotFoundError(fs.size, "non-existing-file")
+    _check_FileNotFoundError(fs.ls, "non-existing-file")
+    _check_FileNotFoundError(fs.open, "non-existing-file")
+
+
 def test_url(repo):
     d, sha = repo
     fs, _, paths = fsspec.core.get_fs_token_paths(f"git://file1::file://{d}")
diff --git a/fsspec/implementations/tests/test_reference.py b/fsspec/implementations/tests/test_reference.py
index 4f025cc..d82dc17 100644
--- a/fsspec/implementations/tests/test_reference.py
+++ b/fsspec/implementations/tests/test_reference.py
@@ -761,18 +761,25 @@ def test_append_parquet(lazy_refs, m):
     assert lazy2["data/1"] == b"Adata"
 
 
-def test_deep_parq(m):
+@pytest.mark.parametrize("engine", ["fastparquet", "pyarrow"])
+def test_deep_parq(m, engine):
     pytest.importorskip("kerchunk")
     zarr = pytest.importorskip("zarr")
+
     lz = fsspec.implementations.reference.LazyReferenceMapper.create(
-        "memory://out.parq", fs=m
+        "memory://out.parq",
+        fs=m,
+        engine=engine,
     )
     g = zarr.open_group(lz, mode="w")
+
     g2 = g.create_group("instant")
     g2.create_dataset(name="one", data=[1, 2, 3])
     lz.flush()
 
-    lz = fsspec.implementations.reference.LazyReferenceMapper("memory://out.parq", fs=m)
+    lz = fsspec.implementations.reference.LazyReferenceMapper(
+        "memory://out.parq", fs=m, engine=engine
+    )
     g = zarr.open_group(lz)
     assert g.instant.one[:].tolist() == [1, 2, 3]
     assert sorted(_["name"] for _ in lz.ls("")) == [".zgroup", ".zmetadata", "instant"]
@@ -780,6 +787,7 @@ def test_deep_parq(m):
         "instant/.zgroup",
         "instant/one",
     ]
+
     assert sorted(_["name"] for _ in lz.ls("instant/one")) == [
         "instant/one/.zarray",
         "instant/one/0",
diff --git a/fsspec/tests/test_core.py b/fsspec/tests/test_core.py
index 53592af..1cdeec9 100644
--- a/fsspec/tests/test_core.py
+++ b/fsspec/tests/test_core.py
@@ -465,3 +465,21 @@ def test_chained_url(ftp_writable):
 def test_automkdir_local():
     fs, _ = fsspec.core.url_to_fs("file://", auto_mkdir=True)
     assert fs.auto_mkdir is True
+
+
+def test_repeated_argument():
+    pytest.importorskip("adlfs")
+    from fsspec.core import url_to_fs
+
+    fs, url = url_to_fs(
+        "az://DIR@ACCOUNT.blob.core.windows.net/DATA",
+        anon=False,
+        account_name="ACCOUNT",
+    )
+    assert fs.storage_options == {"account_name": "ACCOUNT", "anon": False}
+    with pytest.raises(TypeError):
+        url_to_fs(
+            "az://DIR@ACCOUNT.blob.core.windows.net/DATA",
+            anon=False,
+            account_name="OTHER",
+        )