Allow fsspec URLs in open_(mf)dataset #4823

martindurant · 2021-01-18T16:22:35Z

Closes Allow fsspec/zarr/mfdataset #4461 and related
Tests added
Passes pre-commit run --all-files
User visible changes (including notable bug fixes) are documented in whats-new.rst
New functions/methods are listed in api.rst

martindurant · 2021-01-18T16:22:53Z

Docs to be added

martindurant · 2021-01-18T16:54:21Z

Question: should HTTP URLs be passed through unprocessed as before? I think that might be required by some of the netCDF engines, but we probably don't test this.

martindurant · 2021-01-18T17:52:47Z

pint errors in xarray/tests/test_units.py::TestVariable appear unrelated

charlesbluca · 2021-01-18T22:05:52Z

Looking over the changes to is_remote_uri(), it looks like this should resolve the issue I had at #4691!

xarray/backends/zarr.py

Co-authored-by: keewis <[email protected]>

martindurant · 2021-01-19T14:04:05Z

Next open question: aside from zarr, few of the other backends will know what to do with fsspec's dict-like mappers. Should we prevent them from passing through? Should we attempt to distinguish between directories and files, and make fsspec file-like objects? We could just allow the backends to fail later on incorrect input.

alexamici · 2021-01-21T12:19:24Z

@martindurant it is OK to let every backend raise errors for unsupported input. So no need to add any additional logic here IMO.

alexamici · 2021-01-21T12:29:32Z

@martindurant with respect to the backend API (old and new) looks good to me. I'm don't know fsspec so I can't comment on that.

martindurant · 2021-01-21T13:40:33Z

(please definitely do not merge until I've added documentation)

jhamman · 2021-01-21T17:28:28Z

xarray/backends/api.py

            )
-        paths = sorted(glob(_normalize_path(paths)))
+            paths = fs.glob(fs._strip_protocol(paths))
+            paths = [fs.get_mapper(path) for path in paths]


This is a bit tricky. This assumes the backend is to want a mapper object (as the zarr backend does). But, what if the glob returns a list of netcdf files? Wouldn't we want a list of file objects?

Right, this is my comment for "should we actually special case zarr". It could make files - for now it would just error. We don't have tests for this, though, but now might be the time to start.

Now tracking with the comments above, I think we have two options:

inject some backend specific logic in the api here to make a decision about what sort of object to return (if engine=='zarr', return mapper; else return file_obj)

only support globbing remote zarr stores

(1) seems to be the more reasonable thing to do here but is slightly less principled as we've been working to cleanly separate the api from the backends.

I suppose a third alternative might be to pass the paths through, and create mappers in the zarr backend (will re-instantiate the FS, but that's fine) and add the opening of remote files into each of the other backends that can handle it.

I have essentially done 1), but excluded HTTP for the non-zarr path, because it has a special place for some backends (dap...). In any case, I don't suppose anyone is using globbing with http, since it's generally unreliable.

martindurant · 2021-01-27T15:26:57Z

I am marking this PR as ready, but please ask me for specific test cases that might be relevant and should be included.

kmuehlbauer · 2021-01-27T16:09:15Z

@martindurant Should be fixed by #4845. Probably just needs rebase.

martindurant · 2021-01-27T16:10:39Z

Thanks, @kmuehlbauer

mathause · 2021-01-28T08:55:56Z

xarray/backends/api.py

-                "{!r}. Instead, supply paths as an explicit list of strings.".format(
-                    paths
-                )
+            from fsspec import open_files


Do you need to handle the case when fsspec is not installed?

As it is, this would raise ImportError. Would it be better to have a try/except ImportError/raise from, with an extra sentence?
"To open fsspec-compatible [remote?] URLs, you must install fsspec"
Note that you might still get an error in this block if, for example, it's an s3 URL, but s3fs is not installed.

I thought because L893 cannot be reached without fsspec.

(I don't really mind - just because you mention backward compatibility - I am also fine to leave it as is)

xarray/backends/api.py

mathause · 2021-01-28T14:46:08Z

xarray/backends/api.py

-                "{!r}. Instead, supply paths as an explicit list of strings.".format(
-                    paths
-                )
+            from fsspec import open_files


I thought because L893 cannot be reached without fsspec.

martindurant · 2021-01-28T17:27:50Z

I have decided, on reflection, to back away on the scope here and only implement for zarr for now, since, frankly, I am confused about what should happen for other backends, and they are not tested. Yes, some of them are happy to accept file-like objects, but others either don't do that at all, or want the URL passing through. My code would have changed how things were handled, depending on whether it passed through open_dataset or open_mfdataset. Best would be to set up a set of expectations as tests.

raybellwaves · 2021-02-11T18:42:59Z

I think my Q on SO is related to this PR https://stackoverflow.com/questions/66145459/open-mfdataset-on-remote-zarr-store-giving-zarr-errors-groupnotfounderror

Was looking at reading a remote zarr store using open_mfdataset

@martindurant suggested putting the single "file" (mapping) in a list which works
ds = xr.open_mfdataset([file], engine="zarr")

but I also wanted to test the other suggestion

ds = xr.open_mfdataset(uri, engine="zarr", backend_kwargs=dict(storage_options={'anon': True}))

On current xarray master I get

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/ray.bell/Documents/PYTHON_dev/xarray/xarray/backends/api.py", line 884, in open_mfdataset
    raise OSError("no files to open")
OSError: no files to open

On this branch it works

git clone https://github.com/martindurant/xarray.git
git checkout fsspec_mk2
conda create -c conda-forge -n xarray-tests python=3.8
conda env update -f ci/requirements/environment.yml
conda activate xarray-tests
pip install -e .
pip install s3fs

import xarray as xr
uri = "s3://era5-pds/zarr/2020/12/data/eastward_wind_at_10_metres.zarr"
ds = xr.open_mfdataset(uri, engine="zarr", backend_kwargs=dict(storage_options={'anon': True}))

martindurant · 2021-02-12T18:06:49Z

@raybellwaves , might I paraphrase to "this PR is useful, please merge!" ?

keewis

some minor comments, but otherwise looks good to me.

keewis · 2021-02-12T21:54:22Z

xarray/tests/test_backends.py

        with raises_regex(ValueError, "wild-card"):
            open_mfdataset("http://some/remote/uri")

+    @requires_fsspec
+    def test_open_mfdataset_no_files(self):
+        pytest.importorskip("aiobotocore")


this means this test is always skipped. Should this be added to some of the environments?

xarray/tests/test_backends.py

keewis · 2021-02-15T01:00:12Z

could you add aiobotocore to environment.yml, too? py38-all-but-dask should be environment.yml without dask / distributed. Also, could you merge in recent master to resolve the conflicts?

add aiobotocore

martindurant · 2021-02-16T14:26:12Z

Can someone please explain the minimum version policy that is failing

Package           Required             Policy               Status
----------------- -------------------- -------------------- ------
aiobotocore       1.1.2   (2020-08-18) 0.12    (2020-02-24) > (!) (w)

because it pins othr deps too tightly, makes solver panic

keewis · 2021-02-16T17:53:14Z

if you wait two days the minimum version policy check should pass with aiobotocore=1.1.2, too.

dcherian · 2021-02-16T21:17:53Z

There are 3 approvals and @martindurant has been quite patient! Thanks. merging. We can update the min_deps_check later.

martindurant · 2021-02-16T21:26:52Z

Thank you, @dcherian

* upstream/master: FIX: h5py>=3 string decoding (pydata#4893) Update matplotlib's canonical (pydata#4919) Adding vectorized indexing docs (pydata#4711) Allow fsspec URLs in open_(mf)dataset (pydata#4823) Fix typos in example notebooks (pydata#4908) pre-commit autoupdate CI (pydata#4906) replace the ci-trigger action with a external one (pydata#4905) Update area_weighted_temperature.ipynb (pydata#4903) hide the decorator from the test traceback (pydata#4900) Sort backends (pydata#4886) Compatibility with dask 2021.02.0 (pydata#4884)

Martin Durant added 2 commits January 18, 2021 11:20

'Add fsspec hooks

4eb13ff

run black

bb4174e

fix outdated test

86e045f

Martin Durant added 3 commits January 18, 2021 12:11

nother slot

0582d57

plumb deeper

e220b3c

spacing

906e920

martindurant mentioned this pull request Jan 18, 2021

Non-HTTPS remote URLs no longer work as input for open_zarr #4691

Closed

martindurant mentioned this pull request Jan 18, 2021

Allow fsspec/zarr/mfdataset #4461

Closed

5 tasks

Merge branch 'master' into fsspec_mk2

246b171

keewis reviewed Jan 19, 2021

View reviewed changes

xarray/backends/zarr.py Outdated Show resolved Hide resolved

martindurant and others added 2 commits January 19, 2021 08:54

Update xarray/backends/zarr.py

f6b4634

Co-authored-by: keewis <[email protected]>

import reorder

74b3360

scottyhq mentioned this pull request Jan 20, 2021

Allow engine='zarr' and passing args for new xarray API intake/intake-xarray#89

Closed

martindurant changed the title ~~Fsspec mk2~~ Allow fsspec URLs in open_(mf)dataset Jan 20, 2021

alexamici approved these changes Jan 21, 2021

View reviewed changes

keewis marked this pull request as draft January 21, 2021 15:25

jhamman reviewed Jan 21, 2021

View reviewed changes

Martin Durant added 3 commits January 27, 2021 09:47

Separate zarr engine code

ff737b4

lint: import order

414d81c

Don't glob twice for fileobjs

f029213

dcherian added the topic-backends label Jan 27, 2021

Merge branch 'master' into fsspec_mk2

41e4402

andersy005 approved these changes Jan 28, 2021

View reviewed changes

mathause reviewed Jan 28, 2021

View reviewed changes

Martin Durant added 3 commits January 28, 2021 12:12

Only implement for zarr

c40ce4e

Merge branch 'master' into fsspec_mk2

466eb40

Reinstate original exception

e6eb41b

scottyhq mentioned this pull request Feb 3, 2021

Bug: Unable to specify list for urlpath with NetCDFSource + fsspec intake/intake-xarray#98

Closed

pbranson mentioned this pull request Feb 5, 2021

ClientResponseError:400 message='Bad Request' when opening OpenDAP urls with xarray fsspec/filesystem_spec#525

Closed

keewis approved these changes Feb 12, 2021

View reviewed changes

apply suggestions

5e3b6a4

raybellwaves and others added 3 commits February 16, 2021 05:55

add aiobotocore

c82ba9e

Merge pull request #1 from raybellwaves/patch-1

1bcf4e2

add aiobotocore

Merge branch 'master' into fsspec_mk2

c500ec0

Martin Durant added 2 commits February 16, 2021 10:02

try min

3cdf30f

rmove aoi from min

40f9603

because it pins othr deps too tightly, makes solver panic

dcherian merged commit 8bf415a into pydata:master Feb 16, 2021

martindurant deleted the fsspec_mk2 branch February 16, 2021 21:26

raybellwaves mentioned this pull request Feb 17, 2021

ENH: use fsspec to allow read remote single-files corteva/rioxarray#246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow fsspec URLs in open_(mf)dataset #4823

Allow fsspec URLs in open_(mf)dataset #4823

martindurant commented Jan 18, 2021 •

edited

Loading

martindurant commented Jan 18, 2021

martindurant commented Jan 18, 2021

martindurant commented Jan 18, 2021 •

edited

Loading

charlesbluca commented Jan 18, 2021

martindurant commented Jan 19, 2021

alexamici commented Jan 21, 2021

alexamici commented Jan 21, 2021

martindurant commented Jan 21, 2021

jhamman Jan 21, 2021

martindurant Jan 21, 2021

jhamman Jan 21, 2021

martindurant Jan 21, 2021

martindurant Jan 27, 2021

martindurant commented Jan 27, 2021

kmuehlbauer commented Jan 27, 2021

martindurant commented Jan 27, 2021

mathause Jan 28, 2021

martindurant Jan 28, 2021

mathause Jan 28, 2021

mathause Jan 28, 2021

mathause Jan 28, 2021

martindurant commented Jan 28, 2021

raybellwaves commented Feb 11, 2021

martindurant commented Feb 12, 2021

keewis left a comment

keewis Feb 12, 2021

keewis commented Feb 15, 2021

martindurant commented Feb 16, 2021

keewis commented Feb 16, 2021

dcherian commented Feb 16, 2021

martindurant commented Feb 16, 2021

Allow fsspec URLs in open_(mf)dataset #4823

Allow fsspec URLs in open_(mf)dataset #4823

Conversation

martindurant commented Jan 18, 2021 • edited Loading

martindurant commented Jan 18, 2021

martindurant commented Jan 18, 2021

martindurant commented Jan 18, 2021 • edited Loading

charlesbluca commented Jan 18, 2021

martindurant commented Jan 19, 2021

alexamici commented Jan 21, 2021

alexamici commented Jan 21, 2021

martindurant commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martindurant commented Jan 27, 2021

kmuehlbauer commented Jan 27, 2021

martindurant commented Jan 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martindurant commented Jan 28, 2021

raybellwaves commented Feb 11, 2021

martindurant commented Feb 12, 2021

keewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keewis commented Feb 15, 2021

martindurant commented Feb 16, 2021

keewis commented Feb 16, 2021

dcherian commented Feb 16, 2021

martindurant commented Feb 16, 2021

martindurant commented Jan 18, 2021 •

edited

Loading

martindurant commented Jan 18, 2021 •

edited

Loading