issue_comments

@dcherian note that cfgrib is now maintained by @iainrussell, anyway I agree that the in-tree driver can be safely removed.

@headtr1ck I don't think any deprecation is needed, the engine="cfgrib" should select the external backend if available and fall back on the internal one only for ancient versions of cfgrib.

The Compatibility Note I linked above appears to point out the the entry_points(group="xarray.backends") is available since Python 3.6, so we not need the versione check at all.

Should we by default use a temp file? or is there some benefit to retaining the index file locally? (asking as someone with very little context

Opening the a GRIB for the first time requires reading the whole file and saving the index to disk makes all subsequent open much faster. I took the decision to default to writing, but IMO the real issue is that it is not well documented how to switch it off 😅

@isodrosotherm setting indexpath='' will suppress the use of the external index file.

@kmuehlbauer in the representation I use the fully qualified name for the dimension / coordinate, but the corresponding DataArray will use the basename, e.g. both array will have lat as a coordinate. Sorry for the confusion, I need to add more context to the README.

@TomNicholas (cc @mraspaud)

Do you have use cases which one of these designs could handle but the other couldn't?

The two main classes of on-disk formats that, I know of, which cannot be always represented in the "group is a Dataset" approach are: - in netCDF following the CF conventions for groups, it is legal for an array to refer to a dimension or a coordinate in a different group and so arrays in the same group may have dimensions with the same name, but different size / coordinate values, (this was the orginal motivation to explore the DataGroup approach) - the current spec for the Next-generation file formats (NGFF) for bio-imaging has all scales of the same 5D data in the same group. (cc @joshmoore)

I don't have an example at hand, but my impression is that satellite products that use HDF5 file format also place arrays with inconsistent dimensions / coordinates in the same group.

@TomNicholas I also have a few comments on the comparison:

• Option (1) - Each group is a Dataset

• Model maps more directly onto netCDF (though still not exactly, because netCDF has dimensions as separate objects)

This is only true for flat netCDF files, once you introduce groups in a netCDF AND accept CF conventions the DataGroup approach can map 100% of the files, while the DataTree approach fails on a (admittedly small) class of them.

• Enforcing consistency between variables guarantees certain operations are always well-defined (in particular selection via an integer index like in .isel).
• Guarantees that all valid operations on a Dataset are also valid operations on a single group of a DataTree - so API can be essentially identical to Dataset.

Both points are only true for the DataArray in a single group, once you broadcast any operation to subgroups the two implementations would share the same limitations (dimensions in subgroups can be inconsistent in both cases).

In my opinion the advantage for the DataTree is minimal.

• Metadata (i.e. .attrs) are arguably most useful when set at this level

The two approach are identical in this respect, group attributes are mapped in the same way to DataTree and DataGroup

I share your views on all other points.

Note that current CF-1.9 now specifies how to read coordinates from any group in file (see spec).

If it is not already one of the goals of the new DataTree structure #4665 it would probably best be added. cc @TomNicholas @aurghs

1. but most backends serialise writes anyway, so the advantage is limited.

I'm not sure I understand this comment, specifically what is meant by "serialise writes". I often use Xarray to do distributed writes to Zarr stores using 100+ distributed dask workers. It works great. We would need the same thing from a TileDB backend.

I should have added "except Zarr" 😅 .

All netCDF writers use xr.backends.locks.get_write_lock to get a scheduler appropriate writing lock. The code is intricate and I don't find where to point you, but as I recall the lock was used so only one worker/process/thread could write to disk at a time.

Concurrent writes a la Zarr are awesome and xarray supports them now, so my point was: we can add non-concurrent write support to the plugin architecture quite easily and that will serve a lot of users. But supporting Zarr and other advanced backends via the plugin architecture is a lot more work.

@rabernat and all, at the time of the read-only backend refactor @aurghs and I spent quite some time analysing write support and thinking of a unifying strategy. This is my interpretation of our findings:

1. one of the big advantages of the unified xr.open_dataset API is that you don't need to specify the engine of the input data and you can rely on xarray guessing it. This is in general non true when you write your data, as you care about what format you are storing it.

2. another advantage of xr.open_dataset is that xarray manages all the functionaries related to dask and to in-memory cacheing, so backends only need to know how to lazily read from the storage. Current (rather complex) implementation has support for writing from dask and distributed workers but most backends serialise writes anyway, so the advantage is limited. This is not to say that it is not worth, but the cost / benefit ratio of supporting potentially distributed writes is much lower than read support.

3. that said, I'd really welcome a unified write API like ds.save(engine=...) or even xr.save_dataset(ds, engine=...) with a engine keyword argument and possibly other common options. Adding support for a single save_dataset entry point to the backend API is trivial, but adding full support for possibly distributed writes looks like it is much more work.

Also note that ATM @aurghs and I are overloaded at work and we would have very little time that we can spend on this :/

@shoyer I have two suggestions:

1. I would very much prefer to have the exception in the top level module in order to allow: python import xarray as xr try: ds = xr.open_dataset(...) except xr.NoMatchingEngineError: ...

2. I feel that the error message is very clear, but the name of the exception seems slightly confusing as it appears to imply that no engine can open the dataset when it is only that we couldn't autodetect which engine to use just inspecting at the filename_or_obj. I would prefer something along the lines of EngineAutodetectionError. But I don't feel too strong about it.

@shoyer and @pont-us my take is that engine=None is mostly and feature to help new users experiment with xarray. The logic for autodetecting the engine has always been extremely fragile and the result depends on what packages are installed on the system.

I think it is OK to add a dedicated xarray.EngineGuessingError, but I'd not try anything more complex and in general invite users to pass engine explicitly.

Examples of strings that identify a dataset when the right backend is installed, but are not paths and the FileNotFoundError would sound confusing: - /vsizip/my.zip/my.tif with rioxarray - https://myhost/myfile.nc with netcdf4 and others

@TomNicholas feel free to close the issue once you are done.

12899 passed, 1145 skipped, 211 xfailed, 63 xpassed, 321 warnings on my Mac.

How many xarray devs does it take to do a release? Answer: at least 5 apparently

🤦

But next time it will be sooooooo smooth!

I'm testing after pip install -U xarray just to be on the safe side 😄

hmm should I also delete the tag? It appears to be also linked to one of @keewis ' PRs (#5272)

yes, sorry: delete release AND tag. Then re-create release.

Deleting and re-creating the release should work (I've done it on one of my projects)

I guess I can't do this because I'm no one of the registered package owners on PyPI?

Nope. The maintainers on PyPI are Deepak, Joe, Max and Stephan.

@dcherian I'm back. I think the easiest is for you to upload the artefacts using with twine. If you need any help I'm in the zoom room.

@dcherian I'm sorry, I have cannot make it by 12:30 EST. I think you and @TomNicholas can go on without me or I will be back later.

@dcherian 11:30-12:30 EST I'm meeting with Stephan and Joe, I hope to have them merge #5235 :)

~12:30 EST is fine by me.~ I cannot make it :/

@TomNicholas I'm in Europe, with a meeting at 17:30-18:30 CEST (11:30-12:30 EST) we can probably start now-ish and finish after 3pm EST. I propose to hijack the community meeting zoom :)

@TomNicholas & @dcherian I'm having a look at the what's new, shall we set a time for going through the release process? do we want do the steps while in a call?

Any consensus on whether this is ready to merge? We should start the release process shortly (once the US wakes up).

Form what I can see the #5258 "bump versions" looks ready, someone with knowledge about minimum versions please merge it.

After that is in, @dcherian you can probably uncomment the var._meta bit in #4740 "faster interp" and we can merge it as well. Again I'm not a user of that part of code, and I would feel more conformable if someone with more knowledge would click on the "merge" button.

This leaves only #5235 as a blocker that has a failing test, a proposed fix by @keewis. ~but the OP hasn't responded yet. How should we handle it?~ This looks ready to merge now. Sorry @tomwhite I read the timestamps wrong. Great job!

@andersy005 are you OK to try and merge this "as is" sometime today and then help us making a couple of tests on test.pypi.prg before doing the real release tomorrow?

(worst case: we release the old way)

Any thoughts on merging #5244 and doing the v0.18.0 release via GitHub Actions?

I'm using GA in a couple of repositories and I find them very handy, but I usually had to make a couple of releases before getting all the details right, which looks awkward to do with a high profile project like xarray.

We could try release one or more release candidates and go for v0.18.0 only after we are sure the GA release procedure is working though.

@dcherian with dask>=2.15 you can update #4740 that looks mergiable as well, right?

I'll close this issue, @chiaral please follow up on ecmwf/cfgrib#231

@chiaral thanks for spotting this issue! This is actually a bug in cfgrib 0.9.9.0 not xarray. I'll transfer it to the cfgrib repo.

@max-sixty & @andersy005 mine was more of a general comment. It is OK to merge it for me, but please after the release!

I feel this PR is too big and diverse to accept it as a whole.

For example I like most changes from older style formatting to f-strings, but I disagree strongly the moving the return statements into functions improves readability.

@TomNicholas see my comment here: https://github.com/pydata/xarray/issues/5073#issuecomment-831700309

I think the best course for now is to only raise the deprecation warning in zarr and pydap, as al other in-tree backends actually support the lock argument.

Sorry for being so late to the party! I thought @aurghs did respond to this issue and forgot about it.

What has happened is not that lock (and group for the matter) disappeared altogether, but it has been moved to be a backend specific keyword argument. As far as I can see most internal backends still support it as before: cfgrib, netcdf4, pseudonetcdf, pynio and scipy.

The main difference is that lock was silently ignored by backend that don't support it like zarr and pydap.

The solution is to in fact only advertise the deprecation for those backends in my opinion as the functionality is legit for most of the backends.

Sorry again for seeing this so late.

I propose the following timeline that is the fastest I can personally try:

• Monday May the 3rd, feature freeze, please refrain from merging new features unless they are discussed here
• during feature freeze everybody please help reviewing the PR in the list above
• Thursday the 6th, target release day, the best would be to schedule a window for a couple of hours (?) for @TomAugspurger and me to going through the release steps together with @dcherian.

@TomAugspurger and others does it work for you all?

I don't know, I've never managed to get cfgrib and xarray to install on python 3.9. The conda solver fails.

It looks like there is a conflict between xarray and eccodes dependencies, but I didn't investigate.

I would do another regex special to check whether it contains a \

@max-sixty I was thinking to something more brutal: I would drop the r-prefix everywhere and fix by hand the failing tests (if any) ;)

@max-sixty I like the cleanup even it it is rather big. I for one find it very confusing when tests use non standard helpers.

BTW, I think you can skip the r-prefix everywhere making the change even more appealing. It is only useful when using \ operands in the the match string which is almost never the case in xarray.

@max-sixty I'm +1 on deprecating xarray flavour of open_rasterio and direct users to rioxarray.

BTW, I would rather prefer the next release to be 0.18.0 as the backend changes are both large and user visible (open_dataset now accept **kwargs that are added to **backend_kwargs).

@kmuehlbauer precisely. For example the AbstractDataStore is not part of the API anymore and it is kept only for backward compatibility.

What I see missing is a way defined at API level for the backends to "attach" arbitrary information and possibly code to the backend Datasets. cc @shoyer @jhamman

Closed by #5135

@bcbnz thanks for the help! I'll merge it.

@aurghs can you confirm that all failures are unrelated to the changes?

Yes, I would close the issue.

@keewis after merging #5139 the CI should not be failing anymore. The problem is now that cfgrib may not be consistently tested on all jobs and failures are silent.

I'll wait a bit for the fix in python-eccodes, otherwise I'll do a new release of the conda package of cfgrib.

I'll merge it now to stop the CI from failing. The real fix is on the python-eccodes side: https://github.com/conda-forge/python-eccodes-feedstock/issues/69 and https://github.com/conda-forge/python-eccodes-feedstock/issues/71

Technically this is an incompatibility between python-eccodes and eccodes, I'm trying to resolve it via a PR to the python-eccodes-feedstock: https://github.com/conda-forge/python-eccodes-feedstock/pull/71

As a general measure I would add ImportError to the exception caught on import so we avoid to crash xarray:

https://github.com/pydata/xarray/blob/186c2ff6542f090a8e97ffb755f866030be30be9/xarray/backends/cfgrib_.py#L18-L29

To work around the current problem we may need to add a eccodes>=2.21.0 to the CI :/

Thanks @keewis! ATM the main suspect is a flaky the dependency between python-eccodes and eccodes, not cfgrib itself. So it may look like a random error in CI.

Looking into ecmwf/cfgrib#226 that seems related.

This didn't close automatically, but it is closed by #5127

Making a backend doesn't have to be super difficult either depending if you already have a nice 3rd party module you can thinly wrap to return a Dataset instead of whatever is the default

Absolutely agree: https://github.com/corteva/rioxarray/blob/master/rioxarray/xarray_plugin.py

(the PR took a grand total of 48 hours from open to merge: https://github.com/corteva/rioxarray/pull/281)

It's funny how different our backgrounds are, I don't think I've had to deal with console_scripts.

$ grep -r console_scripts develop/ | wc -l 23

😅

The first comment is that I see the point of the feature at a theoretical level, but I'm at the third external backend that I'm writing and I didn't miss it. Adding an entrypoint is very easy and works seamlessly during development. Do you have in mind any use case where this is more convenient than adding an entrypoint?

I simply want to do:

```python from custom_backend import engine

ds = xr.load_dataset(filename, engine=engine) ```

That is much simpler than having to figure out what the How to register a backend is talking about. Because I'm a user who doesn't have any grand dreams (yet?) of creating public backend modules I therefore don't see the point in having to do all this extra paperwork.

Well, based on the amount of complexity a developer needs to master in order to write the backend, I would consider the registration to be a relatively trivial bit, especially because the syntax is the same as for the well known console_script.

On one hand I judge the feature to be relatively simple to support in the long long term, on the other hand I still feel that its benefit will be be quite marginal. Therefore I'm still a mild -1.

... we ask backend developers to inherit from BackendEntrypoint, for the sake of consistency I would enforce it here as well, instead to use duck-typing.

That's not how I read the docs. If this is how we actually want it then some words in it should be replaced with "must" and "requires".

But I don't think that should be such a hard requirement when a user insists on using a custom engine this way. I see it as an advanced option where it's the users responsibility to make sure that the engine class has

• the open_dataset method
• the open_dataset_parameters attribute
• the guess_can_open method

Which is very simple to do, see the test for an example. Subclassing using BackendEntrypoint adds complexity and readability issues so I want to at least feel a little motivated to use it but there's nothing fancy going on there, is there any plans?

On this one I can side with you. The initial proposal from @aurghs and myself was to use a dict 😄 .

This one is a higher lever design decisione, I think @jhamman and @shoyer need to weight in.

This VERY important to merge!

@aurghs you may also use _normalize_path that does all the checks:

https://github.com/pydata/xarray/blob/master/xarray/backends/api.py#L112

@aurghs & @dcherian note that at the moment most backends accept pathlib.Path (but it is not tested), so the isintance(..., str) doesn't run expanduser in that case.

@Illviljan sorry for being late to the party.

The first comment is that I see the point of the feature at a theoretical level, but I'm at the third external backend that I'm writing and I didn't miss it. Adding an entrypoint is very easy and works seamlessly during development.

Do you have in mind any use case where this is more convenient than adding an entrypoint?

WRT the implementation, at the request of @shoyer and @jhamman, we ask backend developers to inherit from BackendEntrypoint, for the sake of consistency I would enforce it here as well, instead to use duck-typing.

Same with BackendArray for plugin that want to support lazy loading.

I started a PR against rioxarray to add an engine via the plugin interface see https://github.com/corteva/rioxarray/pull/281

If nobody objects I'll merge this shortly as this breaks building against master of external projects testing out the backend-plugin infrastructure. We can always add a deprecation warning on a follow-up PR.

I see quite a few uses "in the wild":

https://github.com/search?l=Python&q=LazilyOuterIndexedArray&type=Code

I would suggest to keep it long term to avoid migration pains to external backend developers.

The reason to delegate all path normalisation to backends is that not all strings are paths, for example 'NETCDF:"~/sst.nc":tos' is a valid source string for GDAL / rasterio.

I would suggest to add path normalisation to the backends that needs them, in order to avoid any special case in the common code path.

@aurghs I suggest you add os.path.expanduser in the netCDF backend.

@keewis @mathause and @b8raoult the change looks good to me.

Non-conda users may very well hit the RuntimeError and xarray should not fail to load in that case, but throwing a the warning looks sensible to me.

Closed by #4810

The PR is ready for review. All tests pass. I plan to write the what's new over the week end and target a merge on Monday.

@jp-dark it is in fact possible to write an xarray backend without explicitly depending on xarray in your setup.py if you put all your backend glue code in a separate module not imported by the main __init__.py.

We use the setuptools entrypoints infrastructure that triggers a module load only from within xarray itself.

This is still work in progress, but we are implementing this strategy in cfgrib with success. You can get inspiration from the following PR by @aurghs:

https://github.com/ecmwf/cfgrib/pull/203

I don't think we will be ready by this week-end and we don't want to rush the switch to the new API without some thorough testing by others. We will wait for the next release.

@jhamman & @shoyer, unfortunately neither @aurghs nor myself will be available tomorrow, 18 Feb. We can skip this week's meeting or reschedule it, same time but another day. Your call.

The new flexible backend API is almost ready #4309 (see PRs with label CZI Grant) and the plan is for it to become the default backend API for the next major release (cc @shoyer @jhamman)

I'll prepare the PR to move the code in apiv2.py to api.py soon.

@dcherian do you have a target release date?

@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 it is OK to let every backend raise errors for unsupported input. So no need to add any additional logic here IMO.

With this change xr.list_engines() returns a dictionary with all usable engines.

The implementation is forward compatible, so once backends are split off the main package newer backend registered via the APIv2 plugin interface have precedence.

I'll merge it.

woops, I see you didn't finish, yet :)

I'll merge it after the last cleanup as I need this to start working on the API v1 to v2 PR. Thanks @shoyer !

Using importlib.util.find_spec is very elegant but much less readable than a try: except ModuleNotFoundError: block, I suggest to convert all checks on imports to them.

@mathause I think you are right, #4733 (the issue that #4737 aimed to fix) is probably not related to cfgrib, I didn't notice that no backend code was mentioned in the traceback.

I was fooled by the fact that we had a know bug in cfgrib https://github.com/ecmwf/cfgrib/issues/157 and the error message reported in #4733 was the same.

So the PR #4737 is needed anyhow, but it doesn't really solve the issue it refers to :/

Looks like an effective way to fix the bug is in xarray itself, so I reopen the issue.

@DanielAdriaansen thanks for reporting, this is a duplicate of https://github.com/ecmwf/cfgrib/issues/157 so I'm going to close this one, and you can follow there.

I'm a bit embarrassed as @aurghs identified this bug a while back, but I forgot about it!

The fix should be easy, I'll try to make a release shortly.

I merge it as it involves only the new APIv2 code.

I merge as it has been discussed yesterday in the community meeting.

@aurghs I think you should check what documentation needs updating beside the doctring.

Also this deserves and entry in what's new.

@aurghs there's a conflict, would you please fix it?

@max-sixty there's no direct test for the backend Store classes, only indirect tests through open_dataset. For example all other beckends do support pathlib.Path but none is tested.

Once we remove _normalize_path (PR is coming, and we may need a bit of discussion before approval) we will be able to test all backends.

I propose to merge this PR as it is.

I'm looking at other bits of knowledge of how backends work that are still present in the generic parts of open_dataset.

We see _autodetect_engine and _normalize_path.

We aim at removing _autodetect_engine in favour of adding a new can_open(filename_or_obj, ...) backend function declared via the plugin interface.

On the other hand _normalize_path can be removed entirely once ZarrStore.open_group will accept os.PathLike objects.

@keewis WRT your comments on list comprehensions we at B-Open prefer to use them only when they make the code one line and actually prefer explicit loops otherwise.

This is both because we find them more readable as code and because tracebacks as well are easier to parse when they point to the statement that raised.

These are cosmetic changes to only the apiv2.py file and the Linux py38-backend-api-v2 pass, so I can merge them without much risk.

I fixed the type ints and the two test failures are unrelated to the changes.

I'm going to test my shiny new merge rights :)

@shoyer I favour option 2. as a stable solution, possibly with all arguments moved to keyword-only ones: * users don't need to import and additional class * users get the argument completion onopen_dataset * xarray does validation and mangling in the class and passes to the backends only the non default values

I'm for using the words decode/decoding but I'm against the use of CF.

What backend will do is map the on-disk representation of the data (typically optimised for space) to the in-memory representation preferred by xarray (typically optimised of easy of use). This mapping is especially tricky for time-like variables.

CF is one possible way to specify the map, but it is not the only one. Both the GRIB format and all the spatial formats supported by GDAL/rasterio can encode rich data and decoding has (typically) nothing to do with the CF conventions.

My preferred meaning for the decode_-options is: * True: the backend attempts to map the data to the xarray natural data types (np.datetime64, np.float with mask and scale) * False: the backend attempts to return a representation of the data as close as possible to the on-disk one

Typically when a user asks the backend not to decode they intend to insepct the content of the data file to understand why something is not mapping in the expected way.

As an example: in the case of GRIB time-like values are represented as integers like 20190101, but cfgrib at the moment is forced to convert them into a fake CF representation before passing them to xarray, and when using decode_times=False a GRIB user is presented with something that has nothing to do with the on-disk representation.

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

@jhamman & @shoyer ping.

We'd like to have this (and optionally #4462) merged soon so we can more easily work on PR from master. Thanks!

@shoyer & @jhamman just to give you an idea, I aim to see open_dataset reduced to the following:

```python def open_dataset(filename_or_obj, , engine=None, chunks=None, cache=None, backend_kwargs=None, *kwargs): filename_or_obj = nomalize_filename_or_obj(filename_or_obj) if engine is None: engine = autodetect_engine(filename_or_obj) open_backend_dataset = get_opener(engine)

backend_ds = open_backend_dataset(filename_or_obj, **backend_kwargs, **kwargs)
ds = dataset_from_backend_dataset(
    backend_ds, chunks, cache, filename_or_obj=filename_or_obj, **kwargs
)
return ds

```

Where the key observation is that backend_ds variable must be either np.ndarray or subclasses of BackendArray. That is backend should not be concerned with the in-memory representation of the variables, so they know nothign about dask, cache behaviour, etc. (@shoyer this was addressed briefly today)

@jhamman I don't understand why 4f414f2 is shown here, it is a commit in master.

As for the rest, we re applied the engine selection refactor and added the new zarr bits.

All looks reasonable to me and all tests pass.

Thanks @keewis! So it is correct, I wanted to reset to master and re apply the changes.

Woops I didn't know that force pushing a branch would close the PR :/

I'll try to reopen this PR or create a new one. Sorry for the noise.

Yes, please.

Minor: also I'd personally find even more intuitive the selection da[[0, 2, 2], [1, 3]] that still shows you can repeat indexes, but somewhat looks more like a downsampling. Your call.

@jhamman: @shoyer requested to keep this PR on hold until #4187 is merged as it will produce conflicts.

So I converted the PR to draft, after #4187 gets in, we will fix the conflicts and resubmit.

@jhamman & @shoyer the refactor of the big if-chain to a dictionary in the form {backend_name: backend_open} is useful independently of the using the plugin interface, would make this PR smaller and may be useful in the bigger backend refactort.

If you don't mind I'll ask @TheRed86 to open a PR for this refactor alone.

@shoyer and @jhamman, I'm looking into the write support and if we let a backend return a xr.Dataset, as agreed, we lose the ability to support in-place change of a file, no update of attributes or mode='a', unless the backend has a unambiguous way to identify persistent xr.Dataset.

I'm not sure what options for in-place change are supported, but I see at least mode='a' for zarr. Let's discuss this tomorrow.

So weekly meeting every Thurday, perfect (cc @aurghs @TheRed86)