Repository: dask/dask
Pull request: Update expected error msg for protocol difference in fsspec
Also remove check for error is a set of filenames is passed in as this
is now supported upstream.

Fixes #6327 

- [x] Tests added / passed
- [x] Passes `black dask` / `flake8 dask`

Open question around whether a `set` of filenames is OK.  I've removed the test for that for now, happy to put it back in and add some checks on the `dask` side of things if that's what we settle on.

Problem statement:
Failing dask/bytes/tests/test_local.py with fsspec master
cc @martindurant

```
========================================================================= FAILURES =========================================================================
______________________________________________________________ test_urlpath_inference_errors _______________________________________________________________

    def test_urlpath_inference_errors():
        # Empty list
        with pytest.raises(ValueError, match="empty"):
            get_fs_token_paths([])

        # Protocols differ
        with pytest.raises(ValueError, match="the same protocol"):
>           get_fs_token_paths(["s3://test/path.csv", "/other/path.csv"])

dask/bytes/tests/test_local.py:74:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

urlpath = ['s3://test/path.csv', '/other/path.csv'], mode = 'rb', num = 1, name_function = None, storage_options = None, protocol = None, expand = True

    def get_fs_token_paths(
        urlpath,
        mode="rb",
        num=1,
        name_function=None,
        storage_options=None,
        protocol=None,
        expand=True,
    ):
        """Filesystem, deterministic token, and paths from a urlpath and options.

        Parameters
        ----------
        urlpath: string or iterable
            Absolute or relative filepath, URL (may include protocols like
            ``s3://``), or globstring pointing to data.
        mode: str, optional
            Mode in which to open files.
        num: int, optional
            If opening in writing mode, number of files we expect to create.
        name_function: callable, optional
            If opening in writing mode, this callable is used to generate path
            names. Names are generated for each partition by
            ``urlpath.replace('*', name_function(partition_index))``.
        storage_options: dict, optional
            Additional keywords to pass to the filesystem class.
        protocol: str or None
            To override the protocol specifier in the URL
        expand: bool
            Expand string paths for writing, assuming the path is a directory
        """
>       chain = _un_chain(urlpath, storage_options or {})

../../Envs/dask-dev/lib/python3.7/site-packages/fsspec/core.py:484:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

path = ['s3://test/path.csv', '/other/path.csv'], kwargs = ({}, {})

    def _un_chain(path, kwargs):
        if isinstance(path, (tuple, list)):
            bits = [_un_chain(p, kwargs) for p in path]
            out = []
            for pbit in zip(*bits):
                paths, protocols, kwargs = zip(*pbit)
                if len(set(protocols)) > 1:
>                   raise ValueError("Protocol mismatch in URL chain")
E                   ValueError: Protocol mismatch in URL chain

../../Envs/dask-dev/lib/python3.7/site-packages/fsspec/core.py:258: ValueError

During handling of the above exception, another exception occurred:

    def test_urlpath_inference_errors():
        # Empty list
        with pytest.raises(ValueError, match="empty"):
            get_fs_token_paths([])

        # Protocols differ
        with pytest.raises(ValueError, match="the same protocol"):
>           get_fs_token_paths(["s3://test/path.csv", "/other/path.csv"])
E           AssertionError: Pattern 'the same protocol' does not match 'Protocol mismatch in URL chain'

dask/bytes/tests/test_local.py:74: AssertionError
_____________________________________________________________________ test_with_paths ______________________________________________________________________

    @pytest.mark.skipif(sys.platform == "win32", reason="pathlib and moto clash on windows")
    def test_with_paths():
        with filetexts(files, mode="b"):
            url = pathlib.Path("./.test.accounts.*")
>           sample, values = read_bytes(url, blocksize=None)

dask/bytes/tests/test_local.py:215:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
dask/bytes/core.py:93: in read_bytes
    fs, fs_token, paths = get_fs_token_paths(urlpath, mode="rb", storage_options=kwargs)
../../Envs/dask-dev/lib/python3.7/site-packages/fsspec/core.py:484: in get_fs_token_paths
    chain = _un_chain(urlpath, storage_options or {})
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

path = PosixPath('.test.accounts.*'), kwargs = {}

    def _un_chain(path, kwargs):
        if isinstance(path, (tuple, list)):
            bits = [_un_chain(p, kwargs) for p in path]
            out = []
            for pbit in zip(*bits):
                paths, protocols, kwargs = zip(*pbit)
                if len(set(protocols)) > 1:
                    raise ValueError("Protocol mismatch in URL chain")
                if len(set(paths)) == 1:
                    paths = paths[0]
                else:
                    paths = list(paths)
                out.append([paths, protocols[0], kwargs[0]])
            return out
        x = re.compile(".*[^a-z]+.*")  # test for non protocol-like single word
        bits = (
            [p if "://" in p or x.match(p) else p + "://" for p in path.split("::")]
>           if "::" in path
            else [path]
        )
E       TypeError: argument of type 'PosixPath' is not iterable

../../Envs/dask-dev/lib/python3.7/site-packages/fsspec/core.py:268: TypeError
================================================================ slowest 10 test durations =================================================================

(0.00 durations hidden.  Use -vv to show these durations.)
==================================================================== 2 failed in 0.49s =====================================================================
```

Hints from issue discussion:
For the first one: he test should be updated to the new error message

For the second one, fsspec.core.get_fs_token_paths should probably call `fsspec.utils.stringify_path` for the input path(s)
Hey, I'm putting in a PR to update the error messages now that @martindurant merged in the fixes in `fsspec` --

question: we now have a new failure in the same test which expects passing a `set` of filenames to `get_fs_token_paths` to fail with a `TypeError`:

```
        with pytest.raises(TypeError):
>           get_fs_token_paths(
                {
                    "sets/are.csv",
                    "unordered/so/they.csv",
                    "should/not/be.csv",
                    "allowed.csv",
                }
            )
E           Failed: DID NOT RAISE <class 'TypeError'>
```

but `get_fs_token_paths` explicitly supports `set`s (now):

```
    if isinstance(urlpath, (list, tuple, set)):
        urlpath = [stringify_path(u) for u in urlpath]
```

Do we want to remove this test from `dask`?  Or do we want to catch this on our end if we don't want to mess with set ordering (or lack thereof)?
Agree, this block should be removed. While dask may generally not pass sets of file-paths for opening (because order matters), other libraries are free to do so.
Ok, I'll add in a check on the `read_bytes` side to raise a `TypeError` if someone passes in a set.

Commit message: Move check for ordered collections into bytes/core.py

`fsspec` allows for sets to be pasted to `get_fs_token_paths` since
other libraries using `fsspec` may not rely on the paths being in an
ordered collection.  Dask does, so we need to perform that check on our
end.

Patch to review:
diff --git a/dask/bytes/core.py b/dask/bytes/core.py
index 76b5a3b312f..12c96c4e846 100644
--- a/dask/bytes/core.py
+++ b/dask/bytes/core.py
@@ -1,4 +1,5 @@
 import copy
+from pathlib import Path
 
 from fsspec.core import (  # noqa: F401
     OpenFile,  # noqa: F401
@@ -90,6 +91,9 @@ def read_bytes(
         represented in the corresponding block.
 
     """
+    if not isinstance(urlpath, (str, list, tuple, Path)):
+        raise TypeError("Path should be a string, pathlib.Path, list or tuple")
+
     fs, fs_token, paths = get_fs_token_paths(urlpath, mode="rb", storage_options=kwargs)
 
     if len(paths) == 0:
diff --git a/dask/bytes/tests/test_local.py b/dask/bytes/tests/test_local.py
index 2eed2da7484..f34c61ac4ff 100644
--- a/dask/bytes/tests/test_local.py
+++ b/dask/bytes/tests/test_local.py
@@ -70,7 +70,7 @@ def test_urlpath_inference_errors():
         get_fs_token_paths([])
 
     # Protocols differ
-    with pytest.raises(ValueError, match="the same protocol"):
+    with pytest.raises(ValueError, match="Protocol mismatch"):
         get_fs_token_paths(["s3://test/path.csv", "/other/path.csv"])
 
     # Options differ
@@ -82,9 +82,12 @@ def test_urlpath_inference_errors():
             ]
         )
 
-    # Unknown type
+
+def test_unordered_urlpath_errors():
+
+    # Unordered urlpath argument
     with pytest.raises(TypeError):
-        get_fs_token_paths(
+        read_bytes(
             {
                 "sets/are.csv",
                 "unordered/so/they.csv",


Code review comments:
diff_hunk: @@ -70,7 +70,7 @@ def test_urlpath_inference_errors():
         get_fs_token_paths([])
 
     # Protocols differ
-    with pytest.raises(ValueError, match="the same protocol"):
+    with pytest.raises(ValueError, match="Protocol mismatch"):
path: dask/bytes/tests/test_local.py
text: @user2:
This seems to be testing a purely fsspec function. @user1 is there coverage for this in fsspec itself? If so, I think we just remove this here.

@user1:
I'll put in this exact case.

@author:
We have a few other tests in there that are directly testing `get_fs_token_paths` -- should these all be moved to `fsspec`?

@user1:
When originally this code was factored out into fsspec, we thought it would be good to keep testing them here, although many were copied or adapted. I'm not sure if it's the case anymore.

@author:
I'm going to go ahead and strip out tests that are duplicated upstream.

diff_hunk: @@ -90,6 +91,9 @@ def read_bytes(
         represented in the corresponding block.
 
     """
+    if not isinstance(urlpath, (str, list, tuple, Path)):
path: dask/bytes/core.py
text: @user1:
Changing `Path` to `os.PathLike` should allow anything that implements `__fspath__`.

I don't know when `os.PathLike` was added to the stdlib. If it's not present for py36 then you can use `hasattr(urlpath, '__fspath__')`.

@author:
TIL
It is in py36 so I'll do that.