WIP: Proposed refactor of read API for backends #4477

aurghs · 2020-09-30T20:12:36Z

The first draft of the new backend API:

Move decoding inside the bakends.
Backends return Dataset with BackendArray.
Xarray manages chunking and caching.
Some code is duplicated, it will be simplified later.
Zarr chunking is still inside the backend for now.

cc @jhamman @shoyer

Addresses Flexible Backend - AbstractDataStore definition #4309
Tests added
Passes isort . && black . && mypy . && flake8
User visible changes (including notable bug fixes) are documented in whats-new.rst
New functions/methods are listed in api.rst

Add necessary imports for this function.

# Conflicts: # xarray/backends/api.py

…ad-refactor # Conflicts: # xarray/backends/apiv2.py

- to be used in apiv2 without instantiate the object

- modify signature - move default setting inside backends

alexamici · 2020-09-30T20:19:21Z

Note that this PR doesn't change xarray behaviour unless the user sets the environment variable XARRAY_BACKEND_API=v2.

To test the new code paths you can use: XARRAY_BACKEND_API=v2 pytest -v xarray/tests/test_backend*py

shoyer · 2020-10-06T06:18:42Z

xarray/backends/apiv2.py

+    backend_ds = open_backend_dataset(
+        filename_or_obj,
+        **backend_kwargs,
+        **{k: v for k, v in kwargs.items() if v is not None},


Passing everything directly as supplied by the user feels a little non-ideal to me here. I wonder if we could group together decoding options into a single argument first? See #4490 for a proposal of what that could look like.

shoyer · 2020-10-06T06:21:17Z

xarray/backends/cfgrib_.py

+    if kwargs:
+        warnings.warn(
+            "The following keywords are not supported by the engine "
+            "and will be ignored: %r" % kwargs
+        )


Issuing warnings for unrecognized keyword arguments seems dangerously permissive. Could we raise errors here?

(This might be easier once we switch to putting all the CF decoding conventions into a single argument, which could then be extended without breaking every backend.)

shoyer · 2020-10-06T06:22:32Z

xarray/backends/cfgrib_.py

+
+        ds = Dataset(vars, attrs=attrs)
+        ds = ds.set_coords(coord_names.intersection(vars))
+        ds._file_obj = file_obj


It would be nice to make this a public API. I guess users still won't want to see this in most cases, so perhaps just keep this in mind for something to document in the new "how to write a backend" documentation?

shoyer · 2020-10-06T06:27:04Z

xarray/backends/zarr.py

+    @classmethod
+    def get_chunk(cls, name, var, chunks):


classmethod is fine, though I guess staticmethod would also work.

shoyer · 2020-10-06T06:28:08Z

xarray/backends/apiv2.py

+    backend_kwargs,
+    **kwargs,


Could these be grouped into a single argument, something like extra_tokens?

shoyer · 2020-10-06T06:28:58Z

I really like the direction this is going! See #4490 for a related design proposal, that could build on top of this (and/or potentially make this easier).

shoyer · 2020-10-08T05:54:33Z

xarray/backends/cfgrib_.py

+    if kwargs:
+        raise ValueError(
+            "The following keywords are not supported by the engine "
+            "and will be ignored: %r" % kwargs
+        )


A simpler option is to just omit these lines and **kwargs entirely. Python will supply an error message like TypeError: open_backend_dataset_cfgrib() got an unexpected keyword argument 'something_unexpected'

Yes, you are right! It doesn't make sense to keep these lines!
The warning was needed in the first version of the code because I didn't filter the input keys. But the idea was to remove it later.

…2.dataset_from_backend_dataset`

…lated error message

shoyer · 2020-10-08T15:33:56Z

xarray/backends/apiv2.py

+    backend_kwargs=None,
+    **kwargs,


One thing that isn't quite clear to me: why have both **kwargs and backend_kwargs, when they are merged together?

Is the intention to only keep one of them in the long term?

Yes, the intention is to keep only one in long term.

which one? :)

I would keep kwargs, but reading #4490 I suspect that you prefer to keep backend_kwargs.
If we keep backend_kwargs, maybe it is more clear for the user the separation between xarray inputs and backend inputs. I also like the way you suggest to use deprecate_kwargs.
The main pro for keeping kwargs is that the user can save 21 chars - backend_kwargs=dict() (and that is important!).
I also prefer flat structures, when possible. But that's a matter of taste.

…ctor # Conflicts: # xarray/backends/zarr.py

shoyer · 2020-10-22T15:07:33Z

Merging per offline discussion. This is all hidden behind an environment variable, so this will allow @aurghs and @alexamici to continue working on these new features.

aurghs and others added 27 commits September 25, 2020 19:07

add in api.open_dataset dispatching to stub apiv2

f961606

remove in apiv2 check for input AbstractDataStore

fb166fa

bugfix typo

0221eec

add kwarg engines in _get_backend_cls needed by apiv2

36a02c7

add alpha support for h5netcdf

cfb8cb8

style: clean not used code, modify some variable/function name

4256bc8

Add ENGINES entry for cfgrib.

1bc7391

Define function open_backend_dataset_cfgrib() to be used in apiv2.py.

748fe5a

Add necessary imports for this function.

Apply black to check formatting.

fb368fe

Apply black to check formatting.

80e111c

add dummy zarr apiv2 backend

e15ca6b

Merge branch 'master' into backend-read-refactor

025cc87

# Conflicts: # xarray/backends/api.py

align apiv2.open_dataset to api.open_dataset

4b19399

remove unused extra_coords in open_backend_dataset_*

572595f

Merge remote-tracking branch 'origin/cfgrib_refactor' into backend-re…

d6e632e

…ad-refactor # Conflicts: # xarray/backends/apiv2.py

remove extra_coords in open_backend_dataset_cfgrib

74aba14

transform zarr maybe_chunk and get_chunks in classmethod

d6280ec

- to be used in apiv2 without instantiate the object

make alpha zarr apiv2 working

c0e0f34

refactor apiv2.open_dataset:

6431101

- modify signature - move default setting inside backends

move dataset_from_backend_dataset out of apiv2.open_dataset

50d1ebe

remove blank lines

383d323

remove blank lines

457a09c

style

2803fe3

Re-write error messages

08db0bd

Fix code style

1f11845

Fix code style

93303b1

remove unused import

bc2fe00

shoyer mentioned this pull request Oct 6, 2020

Group together decoding options into a single argument #4490

Open

shoyer reviewed Oct 6, 2020

View reviewed changes

aurghs added 2 commits October 8, 2020 07:03

replace warning with ValueError for not supported kwargs in backends

d694146

change zarr.ZarStore.get_chunks into a static method

56f4d3f

shoyer reviewed Oct 8, 2020

View reviewed changes

aurghs added 3 commits October 8, 2020 09:45

group backend_kwargs and kwargs in extra_tokes argument in apiv…

df23b18

…2.dataset_from_backend_dataset`

remove in open_backend_dayaset_${engine} signature kwarags and the re…

a04e6ac

…lated error message

black

de29a4c

shoyer reviewed Oct 8, 2020

View reviewed changes

alexamici added 3 commits October 15, 2020 09:23

Merge remote-tracking branch 'upstream/master' into backend-read-refa…

2a7ebde

…ctor # Conflicts: # xarray/backends/zarr.py

Try add a strategy with an environment variable

443cf39

Try add a strategy with an environment variable

9471710

alexamici added topic-backends grant-czi labels Oct 22, 2020

black

aa23209

shoyer approved these changes Oct 22, 2020

View reviewed changes

shoyer marked this pull request as ready for review October 22, 2020 15:06

shoyer merged commit cc271e6 into pydata:master Oct 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Proposed refactor of read API for backends #4477

WIP: Proposed refactor of read API for backends #4477

aurghs commented Sep 30, 2020

alexamici commented Sep 30, 2020

shoyer Oct 6, 2020

shoyer Oct 6, 2020

shoyer Oct 6, 2020

shoyer Oct 6, 2020

shoyer Oct 6, 2020

shoyer commented Oct 6, 2020

shoyer Oct 8, 2020

aurghs Oct 8, 2020 •

edited

Loading

shoyer Oct 8, 2020

aurghs Oct 8, 2020

shoyer Oct 8, 2020

aurghs Oct 8, 2020

shoyer commented Oct 22, 2020

WIP: Proposed refactor of read API for backends #4477

WIP: Proposed refactor of read API for backends #4477

Conversation

aurghs commented Sep 30, 2020

alexamici commented Sep 30, 2020

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

shoyer commented Oct 6, 2020

Choose a reason for hiding this comment

aurghs Oct 8, 2020 • edited Loading

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

shoyer commented Oct 22, 2020

aurghs Oct 8, 2020 •

edited

Loading