issue_comments

I like those timings.

If anyone wants to see how awesome consolidated metadata is, you can try it in this binder: https://github.com/rabernat/pangeo_ecco_examples/

I did a bit of lazy profiling here: https://gist.github.com/rabernat/ce1fb414cf53541afe2245363b06c49d

Things that used to take ~40s now take ~1s. Especially since loading the data is one of the first steps in any pangeo notebook, this is a huge improvement in usability.

Thanks to everyone who helped make it happen!

@rabernat if you're ready, let's merge this.

The failures on Appveyor are unrelated (an issue with int32 and cftime)

👍

Sent from my iPhone

On Dec 4, 2018, at 6:37 PM, Stephan Hoyer notifications@github.com wrote:

@rabernat if you're ready, let's merge this.

The failures on Appveyor are related (an issue with int32 and cftime)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

I'm happy here. ...but Appveyor is not.

Hello @rabernat! Thanks for updating the PR.

• There are no PEP8 issues in the file xarray/backends/api.py !

• In the file xarray/backends/zarr.py, following are the PEP8 issues :

  Line 240:80: E501 line too long (82 > 79 characters)

• There are no PEP8 issues in the file xarray/core/dataset.py !

• There are no PEP8 issues in the file xarray/tests/test_backends.py !

• There are no PEP8 issues in the file xarray/tests/test_distributed.py !

Comment last updated on December 04, 2018 at 19:34 Hours UTC

We may also need some text around updating consolidated data-sets, but that can maybe wait to see what kind of usage people try.

Since xarray cannot append or modify in-place existing zarr stores, this seems outside the scope of xarray for now. But maybe it is worth mentioning in the docs.

LGTM

Do you think there should be more explicit text of how to add consolidation to existing zarr/xarray data-sets, rather than creating them with consolidation turned on?

We may also need some text around updating consolidated data-sets, but that can maybe wait to see what kind of usage people try.

Keywords are now all consolidated and all tests are go.

Ready to merge?

Great to see this. On the API, FWIW I'd vote for using the same keyword (consolidated) in both, less burden on the user to remember what to use.

Glad to see this happening, by the way. Once in, catalogs using intake-xarray can be updated and I don't thin the code will need to change.

Will the default for both options be False for the time being?

Yes

Will the default for both options be False for the time being?

So if the test issues can be considered resolved, the only decision we need to make is about the API.

Do we prefer (the current way): python ds.to_zarr(fname, consolidate=True) xr.open_zarr(fname, consolidated=True)

or @shoyer's suggestion python ds.to_zarr(fname, consolidated=True) xr.open_zarr(fname, consolidated=True) ???

I remember dealing with this in my pull request -- if I recall correctly scheduler was pointing to the scheduler.get function instead. It was a minor bug that was either fixed in the next release of xarray (0.11.0) or Dask (0.20.1).

I bet this is due to the latest dask release (1.0). We can fix this in another PR.

Not sure I understand why there are tests failing now. The failing function is test_basic_compute.

https://travis-ci.org/pydata/xarray/jobs/460873430#L7489

At first glance, this does not appear to have anything to do with my PR. The relevant error is: ```

____ test_basic_compute ______________________________ def test_basic_compute(): ds = Dataset({'foo': ('x', range(5)), 'bar': ('x', range(5))}).chunk({'x': 2}) for get in [dask.threaded.get, dask.multiprocessing.get, dask.local.get_sync, None]: with (dask.config.set(scheduler=get) if LooseVersion(dask.__version) >= LooseVersion('0.19.4') else dask.config.set(scheduler=get) if LooseVersion(dask.version) >= LooseVersion('0.18.0') else dask.set_options(get=get)):

          ds.compute()

xarray/tests/test_dask.py:843:

xarray/core/dataset.py:597: in compute return new.load(kwargs) xarray/core/dataset.py:494: in load evaluated_data = da.compute(*lazy_data.values(), kwargs) ../../../miniconda/envs/test_env/lib/python3.6/site-packages/dask/base.py:390: in compute collections=collections) ../../../miniconda/envs/test_env/lib/python3.6/site-packages/dask/base.py:865: in get_scheduler return get_scheduler(scheduler=config.get('scheduler', None))

get = None, scheduler = <function get at 0x7fc31d9ae048>, collections = None cls = None def get_scheduler(get=None, scheduler=None, collections=None, cls=None): """ Get scheduler function

    There are various ways to specify the scheduler to use:

    1.  Passing in get= parameters (deprecated)
    2.  Passing in scheduler= parameters
    3.  Passing these into global confiuration
    4.  Using defaults of a dask collection

    This function centralizes the logic to determine the right scheduler to use
    from those many options
    """
    if get is not None:
        if scheduler is not None:
            raise ValueError("Both get= and scheduler= provided.  Choose one")
        warn_on_get(get)
        return get

    if scheduler is not None:

      if scheduler.lower() in named_schedulers:

E AttributeError: 'function' object has no attribute 'lower' ../../../miniconda/envs/test_env/lib/python3.6/site-packages/dask/base.py:854: AttributeError ```

Also need to add some version checks...this will only work with zarr > 2.2.

Ping @lilyminium for a review.