Zarr consolidated

[rabernat]

This PR adds support for reading and writing of consolidated metadata in zarr stores.

• Closeshow to incorporate zarr's new open_consolidated method? #2558 (remove if there is no corresponding issue, which should only be the case for minor changes)
• Tests added (for all bug fixes or enhancements)
• Fully documented, including whats-new.rst for all changes and api.rst for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

[pep8speaks]

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

[rabernat]

Ping @lilyminium for a review.

[jhamman]

would it make sense for zarr to handle this?

[rabernat]

What do you mean?

[jhamman]

I meant reloading the zarr store automatically.

[rabernat]

I think that would be hard to achieve. And I'm not sure it's necessary. Frankly I don't know why we return a store object from to_zarr at all.

[martindurant]

zarr.consolidate_metadata returns the output of open_consolidated on the same store, so this is already happening

[rabernat]

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

[jhamman]

reminder to update this version check too.

[rabernat]

Being more explicit about the version seems to fix this issue here. In the tests I have used the importorskip approach.

[rabernat]

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

[shoyer]

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

[lilyminium]

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).

[rabernat]

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):

ds.to_zarr(fname, consolidate=True)
xr.open_zarr(fname, consolidated=True)

or @shoyer's suggestion

ds.to_zarr(fname, consolidated=True)
xr.open_zarr(fname, consolidated=True)

???

[martindurant]

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

[rabernat]

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

Yes

[martindurant]

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.

[alimanfoo]

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.

[rabernat]

Keywords are now all consolidated and all tests are go.

Ready to merge?

[jhamman]

I think this is basically ready. I had a few small questions/comments but this looks safe for a merge here soon.

[jhamman]

Can you reference the issue this is attached to: (:issue:`2558`).

[jhamman]

do we need to consider this TODO here?

[jhamman]

Anything to do here now?

[rabernat]

Do we feel that it's important to expose this functionality from within xarray? I don't.

[martindurant]

I also don't.
I think it's ok for xarray to have an opinion on what the special key is called.

[rabernat]

I propose we just leave these TODO's here as is. If anyone ever needs this feature from the xarray side, this will help guide them on how to implement it.

[martindurant]

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.

[rabernat]

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.

[jhamman]

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

[shoyer]

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

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

[rabernat]

👍

…

Sent from my iPhone

On Dec 4, 2018, at 6:37 PM, Stephan Hoyer ***@***.***> 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.

[rabernat]

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!

[martindurant]

I like those timings.

[rabernat]

This PR adds support for reading and writing of consolidated metadata in zarr stores.

• Closeshow to incorporate zarr's new open_consolidated method? #2558 (remove if there is no corresponding issue, which should only be the case for minor changes)
• Tests added (for all bug fixes or enhancements)
• Fully documented, including whats-new.rst for all changes and api.rst for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

[pep8speaks]

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

[rabernat]

Ping @lilyminium for a review.

[jhamman]

would it make sense for zarr to handle this?

[rabernat]

What do you mean?

[jhamman]

I meant reloading the zarr store automatically.

[rabernat]

I think that would be hard to achieve. And I'm not sure it's necessary. Frankly I don't know why we return a store object from to_zarr at all.

[martindurant]

zarr.consolidate_metadata returns the output of open_consolidated on the same store, so this is already happening

[rabernat]

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

[jhamman]

reminder to update this version check too.

[rabernat]

Being more explicit about the version seems to fix this issue here. In the tests I have used the importorskip approach.

[rabernat]

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

[shoyer]

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

[lilyminium]

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).

[rabernat]

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):

ds.to_zarr(fname, consolidate=True)
xr.open_zarr(fname, consolidated=True)

or @shoyer's suggestion

ds.to_zarr(fname, consolidated=True)
xr.open_zarr(fname, consolidated=True)

???

[martindurant]

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

[rabernat]

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

Yes

[martindurant]

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.

[alimanfoo]

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.

[rabernat]

Keywords are now all consolidated and all tests are go.

Ready to merge?

[jhamman]

I think this is basically ready. I had a few small questions/comments but this looks safe for a merge here soon.

[jhamman]

Can you reference the issue this is attached to: (:issue:`2558`).

[jhamman]

do we need to consider this TODO here?

[jhamman]

Anything to do here now?

[rabernat]

Do we feel that it's important to expose this functionality from within xarray? I don't.

[martindurant]

I also don't.
I think it's ok for xarray to have an opinion on what the special key is called.

[rabernat]

I propose we just leave these TODO's here as is. If anyone ever needs this feature from the xarray side, this will help guide them on how to implement it.

[martindurant]

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.

[rabernat]

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.

[jhamman]

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

[shoyer]

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

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

[rabernat]

👍

…

Sent from my iPhone

On Dec 4, 2018, at 6:37 PM, Stephan Hoyer ***@***.***> 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.

[rabernat]

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!

[martindurant]

I like those timings.

[rabernat]

This PR adds support for reading and writing of consolidated metadata in zarr stores.

• Closeshow to incorporate zarr's new open_consolidated method? #2558 (remove if there is no corresponding issue, which should only be the case for minor changes)
• Tests added (for all bug fixes or enhancements)
• Fully documented, including whats-new.rst for all changes and api.rst for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

[pep8speaks]

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

[rabernat]

Ping @lilyminium for a review.

[jhamman]

would it make sense for zarr to handle this?

[rabernat]

What do you mean?

[jhamman]

I meant reloading the zarr store automatically.

[rabernat]

I think that would be hard to achieve. And I'm not sure it's necessary. Frankly I don't know why we return a store object from to_zarr at all.

[martindurant]

zarr.consolidate_metadata returns the output of open_consolidated on the same store, so this is already happening

[rabernat]

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

[jhamman]

reminder to update this version check too.

[rabernat]

Being more explicit about the version seems to fix this issue here. In the tests I have used the importorskip approach.

[rabernat]

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

[shoyer]

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

[lilyminium]

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).

[rabernat]

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):

ds.to_zarr(fname, consolidate=True)
xr.open_zarr(fname, consolidated=True)

or @shoyer's suggestion

ds.to_zarr(fname, consolidated=True)
xr.open_zarr(fname, consolidated=True)

???

[martindurant]

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

[rabernat]

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

Yes

[martindurant]

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.

[alimanfoo]

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.

[rabernat]

Keywords are now all consolidated and all tests are go.

Ready to merge?

[jhamman]

I think this is basically ready. I had a few small questions/comments but this looks safe for a merge here soon.

[jhamman]

Can you reference the issue this is attached to: (:issue:`2558`).

[jhamman]

do we need to consider this TODO here?

[jhamman]

Anything to do here now?

[rabernat]

Do we feel that it's important to expose this functionality from within xarray? I don't.

[martindurant]

I also don't.
I think it's ok for xarray to have an opinion on what the special key is called.

[rabernat]

I propose we just leave these TODO's here as is. If anyone ever needs this feature from the xarray side, this will help guide them on how to implement it.

[martindurant]

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.

[rabernat]

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.

[jhamman]

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

[shoyer]

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

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

[rabernat]

👍

…

Sent from my iPhone

On Dec 4, 2018, at 6:37 PM, Stephan Hoyer ***@***.***> 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.

[rabernat]

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!

[martindurant]

I like those timings.

[rabernat]

This PR adds support for reading and writing of consolidated metadata in zarr stores.

• Closeshow to incorporate zarr's new open_consolidated method? #2558 (remove if there is no corresponding issue, which should only be the case for minor changes)
• Tests added (for all bug fixes or enhancements)
• Fully documented, including whats-new.rst for all changes and api.rst for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

[pep8speaks]

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

[rabernat]

Ping @lilyminium for a review.

[jhamman]

would it make sense for zarr to handle this?

[rabernat]

What do you mean?

[jhamman]

I meant reloading the zarr store automatically.

[rabernat]

I think that would be hard to achieve. And I'm not sure it's necessary. Frankly I don't know why we return a store object from to_zarr at all.

[martindurant]

zarr.consolidate_metadata returns the output of open_consolidated on the same store, so this is already happening

[rabernat]

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

[jhamman]

reminder to update this version check too.

[rabernat]

Being more explicit about the version seems to fix this issue here. In the tests I have used the importorskip approach.

[rabernat]

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

[shoyer]

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

[lilyminium]

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).

[rabernat]

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):

ds.to_zarr(fname, consolidate=True)
xr.open_zarr(fname, consolidated=True)

or @shoyer's suggestion

ds.to_zarr(fname, consolidated=True)
xr.open_zarr(fname, consolidated=True)

???

[martindurant]

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

[rabernat]

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

Yes

[martindurant]

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.

[alimanfoo]

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.

[rabernat]

Keywords are now all consolidated and all tests are go.

Ready to merge?

[jhamman]

I think this is basically ready. I had a few small questions/comments but this looks safe for a merge here soon.

[jhamman]

Can you reference the issue this is attached to: (:issue:`2558`).

[jhamman]

do we need to consider this TODO here?

[jhamman]

Anything to do here now?

[rabernat]

Do we feel that it's important to expose this functionality from within xarray? I don't.

[martindurant]

I also don't.
I think it's ok for xarray to have an opinion on what the special key is called.

[rabernat]

I propose we just leave these TODO's here as is. If anyone ever needs this feature from the xarray side, this will help guide them on how to implement it.

[martindurant]

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.

[rabernat]

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.

[jhamman]

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

[shoyer]

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

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

[rabernat]

👍

…

Sent from my iPhone

On Dec 4, 2018, at 6:37 PM, Stephan Hoyer ***@***.***> 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.

[rabernat]

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!

[martindurant]

I like those timings.

[rabernat]

This PR adds support for reading and writing of consolidated metadata in zarr stores.

• Closeshow to incorporate zarr's new open_consolidated method? #2558 (remove if there is no corresponding issue, which should only be the case for minor changes)
• Tests added (for all bug fixes or enhancements)
• Fully documented, including whats-new.rst for all changes and api.rst for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

[pep8speaks]

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

[rabernat]

Ping @lilyminium for a review.

[jhamman]

would it make sense for zarr to handle this?

[rabernat]

What do you mean?

[jhamman]

I meant reloading the zarr store automatically.

[rabernat]

I think that would be hard to achieve. And I'm not sure it's necessary. Frankly I don't know why we return a store object from to_zarr at all.

[martindurant]

zarr.consolidate_metadata returns the output of open_consolidated on the same store, so this is already happening

[rabernat]

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

[jhamman]

reminder to update this version check too.

[rabernat]

Being more explicit about the version seems to fix this issue here. In the tests I have used the importorskip approach.

[rabernat]

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

[shoyer]

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

[lilyminium]

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).

[rabernat]

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):

ds.to_zarr(fname, consolidate=True)
xr.open_zarr(fname, consolidated=True)

or @shoyer's suggestion

ds.to_zarr(fname, consolidated=True)
xr.open_zarr(fname, consolidated=True)

???

[martindurant]

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

[rabernat]

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

Yes

[martindurant]

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.

[alimanfoo]

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.

[rabernat]

Keywords are now all consolidated and all tests are go.

Ready to merge?

[jhamman]

I think this is basically ready. I had a few small questions/comments but this looks safe for a merge here soon.

[jhamman]

Can you reference the issue this is attached to: (:issue:`2558`).

[jhamman]

do we need to consider this TODO here?

[jhamman]

Anything to do here now?

[rabernat]

Do we feel that it's important to expose this functionality from within xarray? I don't.

[martindurant]

I also don't.
I think it's ok for xarray to have an opinion on what the special key is called.

[rabernat]

I propose we just leave these TODO's here as is. If anyone ever needs this feature from the xarray side, this will help guide them on how to implement it.

[martindurant]

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.

[rabernat]

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.

[jhamman]

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

[shoyer]

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

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

[rabernat]

👍

…

Sent from my iPhone

On Dec 4, 2018, at 6:37 PM, Stephan Hoyer ***@***.***> 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.

[rabernat]

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!

[martindurant]

I like those timings.

[rabernat]

This PR adds support for reading and writing of consolidated metadata in zarr stores.

• Closeshow to incorporate zarr's new open_consolidated method? #2558 (remove if there is no corresponding issue, which should only be the case for minor changes)
• Tests added (for all bug fixes or enhancements)
• Fully documented, including whats-new.rst for all changes and api.rst for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

[pep8speaks]

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

[rabernat]

Ping @lilyminium for a review.

[jhamman]

would it make sense for zarr to handle this?

[rabernat]

What do you mean?

[jhamman]

I meant reloading the zarr store automatically.

[rabernat]

I think that would be hard to achieve. And I'm not sure it's necessary. Frankly I don't know why we return a store object from to_zarr at all.

[martindurant]

zarr.consolidate_metadata returns the output of open_consolidated on the same store, so this is already happening

[rabernat]

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

[jhamman]

reminder to update this version check too.

[rabernat]

Being more explicit about the version seems to fix this issue here. In the tests I have used the importorskip approach.

[rabernat]

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

[shoyer]

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

[lilyminium]

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).

[rabernat]

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):

ds.to_zarr(fname, consolidate=True)
xr.open_zarr(fname, consolidated=True)

or @shoyer's suggestion

ds.to_zarr(fname, consolidated=True)
xr.open_zarr(fname, consolidated=True)

???

[martindurant]

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

[rabernat]

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

Yes

[martindurant]

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.

[alimanfoo]

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.

[rabernat]

Keywords are now all consolidated and all tests are go.

Ready to merge?

[jhamman]

I think this is basically ready. I had a few small questions/comments but this looks safe for a merge here soon.

[jhamman]

Can you reference the issue this is attached to: (:issue:`2558`).

[jhamman]

do we need to consider this TODO here?

[jhamman]

Anything to do here now?

[rabernat]

Do we feel that it's important to expose this functionality from within xarray? I don't.

[martindurant]

I also don't.
I think it's ok for xarray to have an opinion on what the special key is called.

[rabernat]

I propose we just leave these TODO's here as is. If anyone ever needs this feature from the xarray side, this will help guide them on how to implement it.

[martindurant]

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.

[rabernat]

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.

[jhamman]

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

[shoyer]

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

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

[rabernat]

👍

…

Sent from my iPhone

On Dec 4, 2018, at 6:37 PM, Stephan Hoyer ***@***.***> 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.

[rabernat]

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!

[martindurant]

I like those timings.

[rabernat]

This PR adds support for reading and writing of consolidated metadata in zarr stores.

• Closeshow to incorporate zarr's new open_consolidated method? #2558 (remove if there is no corresponding issue, which should only be the case for minor changes)
• Tests added (for all bug fixes or enhancements)
• Fully documented, including whats-new.rst for all changes and api.rst for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

[pep8speaks]

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

[rabernat]

Ping @lilyminium for a review.

[jhamman]

would it make sense for zarr to handle this?

[rabernat]

What do you mean?

[jhamman]

I meant reloading the zarr store automatically.

[rabernat]

I think that would be hard to achieve. And I'm not sure it's necessary. Frankly I don't know why we return a store object from to_zarr at all.

[martindurant]

zarr.consolidate_metadata returns the output of open_consolidated on the same store, so this is already happening

[rabernat]

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

[jhamman]

reminder to update this version check too.

[rabernat]

Being more explicit about the version seems to fix this issue here. In the tests I have used the importorskip approach.

[rabernat]

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

[shoyer]

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

[lilyminium]

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).

[rabernat]

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):

ds.to_zarr(fname, consolidate=True)
xr.open_zarr(fname, consolidated=True)

or @shoyer's suggestion

ds.to_zarr(fname, consolidated=True)
xr.open_zarr(fname, consolidated=True)

???

[martindurant]

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

[rabernat]

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

Yes

[martindurant]

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.

[alimanfoo]

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.

[rabernat]

Keywords are now all consolidated and all tests are go.

Ready to merge?

[jhamman]

I think this is basically ready. I had a few small questions/comments but this looks safe for a merge here soon.

[jhamman]

Can you reference the issue this is attached to: (:issue:`2558`).

[jhamman]

do we need to consider this TODO here?

[jhamman]

Anything to do here now?

[rabernat]

Do we feel that it's important to expose this functionality from within xarray? I don't.

[martindurant]

I also don't.
I think it's ok for xarray to have an opinion on what the special key is called.

[rabernat]

I propose we just leave these TODO's here as is. If anyone ever needs this feature from the xarray side, this will help guide them on how to implement it.

[martindurant]

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.

[rabernat]

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.

[jhamman]

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

[shoyer]

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

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

[rabernat]

👍

…

Sent from my iPhone

On Dec 4, 2018, at 6:37 PM, Stephan Hoyer ***@***.***> 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.

[rabernat]

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!

[martindurant]

I like those timings.

[rabernat]

This PR adds support for reading and writing of consolidated metadata in zarr stores.

• Closeshow to incorporate zarr's new open_consolidated method? #2558 (remove if there is no corresponding issue, which should only be the case for minor changes)
• Tests added (for all bug fixes or enhancements)
• Fully documented, including whats-new.rst for all changes and api.rst for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

[pep8speaks]

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

[rabernat]

Ping @lilyminium for a review.

[jhamman]

would it make sense for zarr to handle this?

[rabernat]

What do you mean?

[jhamman]

I meant reloading the zarr store automatically.

[rabernat]

I think that would be hard to achieve. And I'm not sure it's necessary. Frankly I don't know why we return a store object from to_zarr at all.

[martindurant]

zarr.consolidate_metadata returns the output of open_consolidated on the same store, so this is already happening

[rabernat]

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

[jhamman]

reminder to update this version check too.

[rabernat]

Being more explicit about the version seems to fix this issue here. In the tests I have used the importorskip approach.

[rabernat]

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

[shoyer]

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

[lilyminium]

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).

[rabernat]

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):

ds.to_zarr(fname, consolidate=True)
xr.open_zarr(fname, consolidated=True)

or @shoyer's suggestion

ds.to_zarr(fname, consolidated=True)
xr.open_zarr(fname, consolidated=True)

???

[martindurant]

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

[rabernat]

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

Yes

[martindurant]

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.

[alimanfoo]

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.

[rabernat]

Keywords are now all consolidated and all tests are go.

Ready to merge?

[jhamman]

I think this is basically ready. I had a few small questions/comments but this looks safe for a merge here soon.

[jhamman]

Can you reference the issue this is attached to: (:issue:`2558`).

[jhamman]

do we need to consider this TODO here?

[jhamman]

Anything to do here now?

[rabernat]

Do we feel that it's important to expose this functionality from within xarray? I don't.

[martindurant]

I also don't.
I think it's ok for xarray to have an opinion on what the special key is called.

[rabernat]

I propose we just leave these TODO's here as is. If anyone ever needs this feature from the xarray side, this will help guide them on how to implement it.

[martindurant]

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.

[rabernat]

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.

[jhamman]

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

[shoyer]

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

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

[rabernat]

👍

…

Sent from my iPhone

On Dec 4, 2018, at 6:37 PM, Stephan Hoyer ***@***.***> 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.

[rabernat]

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!

[martindurant]

I like those timings.