issue_comments

Thanks @weiji14 and @Mikejmnez . This is a great contribution.

Note that zarr.open* now works with fsspec URLs (in master)

Thanks @weiji14 and @Mikejmnez for your contribution!

Hello @weiji14! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2020-09-21 23:44:07 UTC

Just to note down a few things:

1. The deprecated "auto_chunk" kwarg was removed
2. open_zarr uses chunks="auto" by default, whereas open_dataset uses chunks=None (see my comment inline)

The different default chunk behaviour (point 2) is worth raising, and it might be best to postpone the deprecation of open_zarr until the next release, so that there's time to discuss what we want the default setting to be (None or auto).

These all sound good to me!

I agree that we shouldn't change the default behavior for open_dataset, and should keep open_zarr around for now -- there is no urgent need to deprecate it.

I agree. I think we should keep open_zarr around.

Just wanted to mention that two of the reviewers in the last PR (see https://github.com/pydata/xarray/pull/4003#issuecomment-619644606 and https://github.com/pydata/xarray/pull/4003#issuecomment-620169860) seemed in favour of deprecating open_zarr. If I'm counting the votes correctly (did I miss anyone?), that's 2 for, and 2 against. We'll need a tiebreaker :laughing:

As reminder (because it took me a while to remember!), one goal with this refactor is to have open_mfdataset work with all backends (including zarr and rasterio) by specifying the engine kwarg.

Yes exactly, time does fly (half a year has gone by already!).

Currently I'm trying to piggyback Zarr into test_openmfdataset_manyfiles from #1983, ~~and am currently having trouble finding out why opening Zarr stores via open_mfdataset doesn't return a dask backed array like the other engines (Edit: it only happens when chunks is None, see https://github.com/pydata/xarray/pull/4187#discussion_r448734418). Might need to spend another day digging through the code to see if this is expected behaviour.~~ Edit: got a workaround solution in b3d6a6a46f8ead25b6f7f593f7b46f43a4de650c by using chunks="auto" as was the default in open_zarr.

As a note I'm working on implementing zarr spec v3 in zarr-python, still deciding how we want to handle the new spec/API.

If there are any changes that you would like or dislike in an API, feedback is welcome.

Thanks for chipping in @Carreau! I'm sure the community will have some useful suggestions. Just cross-referencing https://zarr-developers.github.io/zarr/specs/2019/06/19/zarr-v3-update.html so others can get a better feel for where things are at.

I wonder if it's really worth deprecating open_zarr(). open_dataset(..., engine='zarr') is a bit more verbose, especially with backend_kwargs to pass optional arguments. It seems pretty harmless to keep open_zarr() around, especially if it's just an alias for open_datraset(engine='zarr').

Depends on which line in the Zen of Python you want to follow - "Simple is better than complex", or "There should be one-- and preferably only one --obvious way to do it". From a maintenance perspective, it's balancing the cost of a deprecation cycle vs writing code that tests both instances I guess.

We could also automatically detect zarr stores in open_dataset without requiring engine='zarr' if:

1. the argument inherits from collections.abc.Mapping, and
2. it contains a key '.zgroup', corresponding to zarr metadata.

As for the annoyance of needing to write backend_kwargs={"consolidated": True}, I wonder if we could detect this automatically by checking for the existence of a .zmetadata key? This would add a small amount of overhead (one file access) but this probably would not be prohibitively expensive.

These are some pretty good ideas. I also wonder if there's a way to mimic the dataset identifiers like in rasterio, something like xr.open_dataset("zarr:some_zarrfile.zarr"). Feels a lot more like fsspec's url chaining too.

Counter-argument would be that the cyclomatic complexity of open_dataset is already too high, and it really should be refactored before adding more 'magic'. Especially if new backend engines come online (e.g. #4142).

As a note I'm working on implementing zarr spec v3 in zarr-python, still deciding how we want to handle the new spec/API.

If there are any changes that you would like or dislike in an API, feedback is welcome.

I wonder if it's really worth deprecating open_zarr(). open_dataset(..., engine='zarr') is a bit more verbose, especially with backend_kwargs to pass optional arguments. It seems pretty harmless to keep open_zarr() around, especially if it's just an alias for open_datraset(engine='zarr').

I agree. I think we should keep open_zarr around.

As reminder (because it took me a while to remember!), one goal with this refactor is to have open_mfdataset work with all backends (including zarr and rasterio) by specifying the engine kwarg.

As for the annoyance of needing to write backend_kwargs={"consolidated": True}, I wonder if we could detect this automatically by checking for the existence of a .zmetadata key? This would add a small amount of overhead (one file access) but this probably would not be prohibitively expensive.

We could also automatically detect zarr stores in open_dataset without requiring engine='zarr' if:

1. the argument inherits from collections.abc.Mapping, and
2. it contains a key '.zgroup', corresponding to zarr metadata.

I wonder if it's really worth deprecating open_zarr(). open_dataset(..., engine='zarr') is a bit more verbose, especially with backend_kwargs to pass optional arguments. It seems pretty harmless to keep open_zarr() around, especially if it's just an alias for open_datraset(engine='zarr').

Four more failures, something to do with dask? Seems related to #3919 and #3921.

• [ ] TestZarrDictStore.test_vectorized_indexing - IndexError: only slices with step >= 1 are supported
• [x] TestZarrDictStore.test_manual_chunk - ZeroDivisionError: integer division or modulo by zero
• [ ] TestZarrDirectoryStore.test_vectorized_indexing - IndexError: only slices with step >= 1 are supported
• [x] TestZarrDirectoryStore.test_manual_chunk - ZeroDivisionError: integer division or modulo by zero

Edit: Fixed the ZeroDivisionErrror in 6fbeadf41a1a547383da0c8f4499c99099dbdf97. The IndexError was fixed in a hacky way though, see https://github.com/pydata/xarray/pull/4187#discussion_r448077275.

Is it ok to drop the deprecated auto_chunk tests here in this PR (or leave it to another PR)? The deprecation warning was first added in https://github.com/pydata/xarray/pull/2530/commits/ae4cf0ab19b3e563bde90a48b3e6ee615930d4a1, and I see that auto_chunk was used back in v0.12.1 at http://xarray.pydata.org/en/v0.12.1/generated/xarray.open_zarr.html.

Nevermind, I found it. There was an if that should have been an elif. Onward to the next error - UnboundLocalError. Edit: Also fixed!

This is the one test failure (AttributeError) on Linux py36-bare-minimum:

Been scratching my head debugging this one. There doesn't seem to be an obvious reason why this test is failing, since 1) this test isn't for Zarr and 2) this test shouldn't be affected by the new if blocks checking if engine=="zarr". Will need to double check the logic here.