Adding open_groups to BackendEntryPointEngine, NetCDF4BackendEntrypoint, and H5netcdfBackendEntrypoint

[eni-awowale]

• Closes open_dict_of_datasets function to open any file containing nested groups #9137 and in support of Track merging datatree into xarray #8572
• Tests added
• User visible changes (including notable bug fixes) are documented in whats-new.rst
• New functions/methods are listed in api.rst

@TomNicholas, @shoyer, @owenlittlejohns, and @flamingbear

[eni-awowale]

I know we touched on this a bit during our meeting on Tuesday but I think tests for open_groups in netCDF_.py and h5netcdf.py should probably go in test_backends_datatree.py. We mentioned we could create separate test files for it but it looks like test_backends_datatree.py, has an empty class for each of these backends and it looks like they would fit nicely in here.

test_backends_datatree.py

@requires_netCDF4
class TestNetCDF4DatatreeIO(DatatreeIOBase):
    engine: T_DataTreeNetcdfEngine | None = "netcdf4"


@requires_h5netcdf
class TestH5NetCDFDatatreeIO(DatatreeIOBase):
    engine: T_DataTreeNetcdfEngine | None = "h5netcdf"

Let me know if I have that right and what you all think.
@TomNicholas, @shoyer, @keewis, @flamingbear, @owenlittlejohns

EDIT:
I think we could also add a couple more tests for open_datatree for those specific engines to those classes.

EDIT 2:
If it's okay with everyone I think we should also include a small netCDF file in test/data that has groups. example_1.nc and bears.nc don't have groups.

[TomNicholas]

Sounds good @eni-awowale

You may even be able to use basically the same groups test for multiple backends by adding it to DataTreeIOBase...

If it's okay with everyone I think we should also include a small netCDF file in test/data that has groups.

Having an included file to test against is a good idea, we just want it to be small.

example_1.nc and bears.nc don't have groups.

Where are these currently? We don't want to add files to the main repo - preferring to put them here https://github.com/pydata/xarray-data

[eni-awowale]

You may even be able to use basically the same groups test for multiple backends by adding it to DataTreeIOBase...

So, instead of adding the same type of tests to TestNetCDF4DatatreeIO and TestH5NetCDFDatatreeIO I can add them directly to DataTreeIOBase, if the are not engine specific? Okay, that makes sense!

@TomNicholas example_1.nc and bears.nc are here test/data.

We don't want to add files to the main repo - preferring to put them here

Okay, great there might already be some some sample files in there with groups that I can use!

[TomNicholas]

@TomNicholas example_1.nc and bears.nc are here test/data.

I didn't even know those existed... @max-sixty I know you have thought about example datasets for testing purposes - do you have an opinion on whether new files for testing should go in that directory or the separate repository?

[shoyer]

@TomNicholas example_1.nc and bears.nc are here test/data.

I didn't even know those existed... @max-sixty I know you have thought about example datasets for testing purposes - do you have an opinion on whether new files for testing should go in that directory or the separate repository?

We want tests to be runable without a network connection. If we need new files for testing, which I agree would be a good idea in this case, please add them here. Just keep them as small as possible (the current test files are all under 10 KB in size, most under 1 KB).

[shoyer]

Where are these currently? We don't want to add files to the main repo - preferring to put them here https://github.com/pydata/xarray-data

xarray-data is for sample/tutorial datasets, which can be much larger (enough data to make an interesting plot) than what we use for test data.

[TomNicholas]

These shouldn't be here right? They should all fall under `**kwargs``

[TomNicholas]

Or maybe they should be in the specific backend but not in common.py?

[eni-awowale]

Yeah, so these were added from PR #9199 for adding back the backend specific keyword arguments. I pulled this into my branch after it was merged to main. But they are not in common.py , they are consolidated as **kwargs in common.py.

[TomNicholas]

@eni-awowale this is how you should join paths

[TomNicholas]

We could have a default implementation here that calls open_groups, i.e.

Suggested change

	return backend.open_datatree(filename_or_obj, **kwargs)
	groups_dict = backend.open_datatree(filename_or_obj, **kwargs)
	return DataTree.from_dict(groups_dict)

The idea being that then backend developers don't actually have to implement open_datatree if they have implemented open_groups...

This was sort of discussed here (@keewis) #7437 (comment), but this seems like an rabbit hole that should be left for a future PR.

[keewis]

not really, I was arguing that given any one of open_dataarray, open_dataset and open_datatree allows us to provide (somewhat inefficient) default implementations for the others. However, open_groups has a much closer relationship to open_datatree, so I think having a default implementation for open_datatree is fine (we just need to make sure that a backend that provides neither open_groups nor open_datatree doesn't complain about open_groups not existing if you called open_datatree).

So yeah, this might become a rabbit hole.

[TomNicholas]

Okay I see. That seems related, but also like a totally optional convenience feature that we should defer to later.

[eni-awowale]

Hi folks! I think this PR is a couple commits away from merging but they are a couple things to sort out.

mypy checks failing

The open_groups function returns a dict[str, Dataset] type. However, the return type for DataTree.from_dict() is a MutableMapping[str, Dataset | DataArray | DataTree | None] and we call DataTree.from_dict() in our open_datatree() backend functions, so mypy flags this as having an incompatible type. See below.
"from_dict" of "DataTree" has incompatible type "dict[str, Dataset]"; expected "MutableMapping[str, Dataset | DataArray | DataTree[Any] | None]" [arg-type]
I am not sure exactly how to proceed with this since updating the return type of DataTree.from_dict() causes other mypy checks to fail because the are expecting the MutuableMapping return type.

Some checks are failing but seem unrelated to the PR changes?

I noticed there are tests failing, but these seem to be consistent with the checks failing in main and are related to test_computations.py. The other test I see failing are the ubuntu 3.12 flaky tests. Are these failures we can ignore?

Updating _iter_nc_groups

I added the suggestion from @keewis so _iter_nc_groups is returning the parent node and now we don't have to reopen the store twice 🎉

cc
@TomNicholas, @keewis, @shoyer, @flamingbear

[keewis]

Are these failures we can ignore?

flaky is something that may fail at random, mostly because of server or network issues. Usually it will pass again on the next run (I reran it for you, let's see what happens). The other test failures are indeed also on main, so we can ignore it here.

Edit: flaky passes now, so I believe you can ignore the remaining test failures

I am not sure exactly how to proceed with this since updating the return type of DataTree.from_dict() causes other mypy checks to fail because the are expecting the MutableMapping return type.

Maybe the typing crowd can help? cc @max-sixty, @Illviljan, @headtr1ck

[Illviljan]

mypy checks failing

The open_groups function returns a dict[str, Dataset] type. However, the return type for DataTree.from_dict() is a MutableMapping[str, Dataset | DataArray | DataTree | None] and we call DataTree.from_dict() in our open_datatree() backend functions, so mypy flags this as having an incompatible type. See below. "from_dict" of "DataTree" has incompatible type "dict[str, Dataset]"; expected "MutableMapping[str, Dataset | DataArray | DataTree[Any] | None]" [arg-type] I am not sure exactly how to proceed with this since updating the return type of DataTree.from_dict() causes other mypy checks to fail because the are expecting the MutuableMapping return type.

It's this type of problem: https://stackoverflow.com/questions/73603289/why-doesnt-parameter-type-dictstr-unionstr-int-accept-value-of-type-di
from_dict should probably switch from MutableMapping to Mapping instead.

[Illviljan]

Mypy is correct here, a Mapping does not include a .pop and ignoring the typing errors doesn't solve the bug.

xarray uses Mappings frequently, for example xr.core.utils.FrozenDict(dict(a=3)).pop("a", None) so it's a real issue.

Either you explicitly convert it to dict i.e. d_cast = dict(d) or refactor the code to not use the .pop since I'm not so sure it's needed.

[eni-awowale]

Thanks, I updated it to explicitly covert the type to a dict. The mypy3.9 tests are still passing but the CI Mypy check seems to be returning the same error as before explicitly converting it to a dict.

[eni-awowale]

@TomNicholas and @keewis this issue we just moved over #9336 seems to be related to the latest batch of test failures.

[eni-awowale]

Yay all checks are passing! Does someone want to give this a quick look before merging?
cc
@TomNicholas, @keewis, @owenlittlejohns, @shoyer and @flamingbear

[headtr1ck]

Maybe we should not make the same mistake as with open_dataset and prevent liskov errors.

Suggested change

**kwargs: Any,

If the abstract method supports any kwargs, so must all subclass implementations, which is not what we want.

[eni-awowale]

@headtr1ck from my understanding I think the **kwargs were added back to fix this issue #9135

[headtr1ck]

Hmmm, Not sure.
But since this is the same problem in all other backend methods, I'm fine with leaving it as it is (and possibly change it in a future PR all together).

[eni-awowale]

Sounds good we can revisit this on another PR.

[headtr1ck]

This should be obsolete as well, when you remove it from the abstract method.

[headtr1ck]

Same here.