Repository: zarr-developers/VirtualiZarr

Issue:
`open_virtual_dataset` fails when there is a subgroup
(This issue is inspired by the NAS GES DISC [GPM_3IMERGHH_07](https://disc.gsfc.nasa.gov/datasets/GPM_3IMERGHH_07/summary) dataset, which has this same structure. cc @abarciauskas-bgse)

Consider a NetCDF dataset that has a valid NetCDF group at one level of the hierarchy and then a sub-group beneath that. We can make one like this:

```python
import xarray as xr
ds = xr.DataArray([1, 2, 3], name="foo").to_dataset()
ds.to_netcdf("test.nc")
# store the same data in a sub group
ds.to_netcdf("test.nc", group="subgroup", mode="a")
```

Xarray can open either group fine.
```python
xr.open_dataset("test.nc")
xr.open_dataset("test.nc", group="subgroup")
```
For the root group, it just ignores the sub group.

However, VirtualiZarr doesn't like it

```python
from virtualizarr import open_virtual_dataset
open_virtual_dataset("test.nc", group='')
```

```
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
Cell In[7], line 1
----> 1 dsv = open_virtual_dataset("test.nc", group='')

File [/srv/conda/envs/notebook/lib/python3.12/site-packages/virtualizarr/backend.py:200](https://hub.openveda.cloud/srv/conda/envs/notebook/lib/python3.12/site-packages/virtualizarr/backend.py#line=199), in open_virtual_dataset(filepath, filetype, group, drop_variables, loadable_variables, decode_times, cftime_variables, indexes, virtual_array_class, virtual_backend_kwargs, reader_options, backend)
    197 if backend_cls is None:
    198     raise NotImplementedError(f"Unsupported file type: {filetype.name}")
--> 200 vds = backend_cls.open_virtual_dataset(
    201     filepath,
    202     group=group,
    203     drop_variables=drop_variables,
    204     loadable_variables=loadable_variables,
    205     decode_times=decode_times,
    206     indexes=indexes,
    207     virtual_backend_kwargs=virtual_backend_kwargs,
    208     reader_options=reader_options,
    209 )
    211 return vds

File [/srv/conda/envs/notebook/lib/python3.12/site-packages/virtualizarr/readers/hdf5.py:48](https://hub.openveda.cloud/srv/conda/envs/notebook/lib/python3.12/site-packages/virtualizarr/readers/hdf5.py#line=47), in HDF5VirtualBackend.open_virtual_dataset(filepath, group, drop_variables, loadable_variables, decode_times, indexes, virtual_backend_kwargs, reader_options)
     42 refs = SingleHdf5ToZarr(
     43     filepath, inline_threshold=0, **reader_options
     44 ).translate()
     46 refs = extract_group(refs, group)
---> 48 virtual_vars, attrs, coord_names = virtual_vars_and_metadata_from_kerchunk_refs(
     49     refs,
     50     loadable_variables,
     51     drop_variables,
     52     fs_root=Path.cwd().as_uri(),
     53 )
     55 loadable_vars, indexes = open_loadable_vars_and_indexes(
     56     filepath,
     57     loadable_variables=loadable_variables,
   (...)
     62     decode_times=decode_times,
     63 )
     65 return construct_virtual_dataset(
     66     virtual_vars=virtual_vars,
     67     loadable_vars=loadable_vars,
   (...)
     70     attrs=attrs,
     71 )

File [/srv/conda/envs/notebook/lib/python3.12/site-packages/virtualizarr/translators/kerchunk.py:34](https://hub.openveda.cloud/srv/conda/envs/notebook/lib/python3.12/site-packages/virtualizarr/translators/kerchunk.py#line=33), in virtual_vars_and_metadata_from_kerchunk_refs(vds_refs, loadable_variables, drop_variables, virtual_array_class, fs_root)
     17 def virtual_vars_and_metadata_from_kerchunk_refs(
     18     vds_refs: KerchunkStoreRefs,
     19     loadable_variables,
   (...)
     22     fs_root: str | None = None,
     23 ) -> tuple[Mapping[str, Variable], dict[str, Any], list[str]]:
     24     """
     25     Parses all useful information from a set kerchunk references (for a single group).
     26 
   (...)
     31         Required if any paths are relative in order to turn them into absolute paths (which virtualizarr requires).
     32     """
---> 34     virtual_vars = virtual_vars_from_kerchunk_refs(
     35         vds_refs,
     36         drop_variables=drop_variables + loadable_variables,
     37         virtual_array_class=virtual_array_class,
     38         fs_root=fs_root,
     39     )
     40     ds_attrs = fully_decode_arr_refs(vds_refs["refs"]).get(".zattrs", {})
     41     coord_names = ds_attrs.pop("coordinates", [])

File [/srv/conda/envs/notebook/lib/python3.12/site-packages/virtualizarr/translators/kerchunk.py:110](https://hub.openveda.cloud/srv/conda/envs/notebook/lib/python3.12/site-packages/virtualizarr/translators/kerchunk.py#line=109), in virtual_vars_from_kerchunk_refs(refs, drop_variables, virtual_array_class, fs_root)
    104     drop_variables = []
    105 var_names_to_keep = [
    106     var_name for var_name in var_names if var_name not in drop_variables
    107 ]
    109 vars = {
--> 110     var_name: variable_from_kerchunk_refs(
    111         refs, var_name, virtual_array_class, fs_root=fs_root
    112     )
    113     for var_name in var_names_to_keep
    114 }
    115 return vars

File [/srv/conda/envs/notebook/lib/python3.12/site-packages/virtualizarr/translators/kerchunk.py:164](https://hub.openveda.cloud/srv/conda/envs/notebook/lib/python3.12/site-packages/virtualizarr/translators/kerchunk.py#line=163), in variable_from_kerchunk_refs(refs, var_name, virtual_array_class, fs_root)
    161 """Create a single xarray Variable by reading specific keys of a kerchunk references dict."""
    163 arr_refs = extract_array_refs(refs, var_name)
--> 164 chunk_dict, zarray, zattrs = parse_array_refs(arr_refs)
    165 # we want to remove the _ARRAY_DIMENSIONS from the final variables' .attrs
    166 dims = zattrs.pop("_ARRAY_DIMENSIONS")

File [/srv/conda/envs/notebook/lib/python3.12/site-packages/virtualizarr/translators/kerchunk.py:258](https://hub.openveda.cloud/srv/conda/envs/notebook/lib/python3.12/site-packages/virtualizarr/translators/kerchunk.py#line=257), in parse_array_refs(arr_refs)
    255 def parse_array_refs(
    256     arr_refs: KerchunkArrRefs,
    257 ) -> tuple[dict, ZArray, ZAttrs]:
--> 258     zarray = ZArray.from_kerchunk_refs(arr_refs.pop(".zarray"))
    259     zattrs = arr_refs.pop(".zattrs", {})
    260     chunk_dict = arr_refs

KeyError: '.zarray'
```

It looks like VirtualiZarr is assuming that all child nodes in the hierarchy are arrays, not groups.

---

I'm also curious why we are not going through the new [Non-kerchunk backend for HDF5/netcdf4 files](https://github.com/zarr-developers/VirtualiZarr/pull/87), rather than the kerchunk backend. How do you turn that on? (I'm on 1.2.0.)


Related to #84

Discussion:

Patch:
diff --git a/docs/releases.rst b/docs/releases.rst
index 0512e3b..79b8e26 100644
--- a/docs/releases.rst
+++ b/docs/releases.rst
@@ -12,12 +12,19 @@ New Features
 Breaking changes
 ~~~~~~~~~~~~~~~~
 
+- Passing ``group=None`` (the default) to ``open_virtual_dataset`` for a file with multiple groups no longer raises an error, instead it gives you the root group.
+  This new behaviour is more consistent with ``xarray.open_dataset``.
+  (:issue:`336`, :pull:`337`) By `Tom Nicholas <https://github.com/TomNicholas>`_.
+
 Deprecations
 ~~~~~~~~~~~~
 
 Bug fixes
 ~~~~~~~~~
 
+- Fix bug preventing generating references for the root group of a file when a subgroup exists.
+  (:issue:`336`, :pull:`337`) By `Tom Nicholas <https://github.com/TomNicholas>`_.
+
 Documentation
 ~~~~~~~~~~~~~
 
diff --git a/virtualizarr/readers/fits.py b/virtualizarr/readers/fits.py
index 1a21877..d285187 100644
--- a/virtualizarr/readers/fits.py
+++ b/virtualizarr/readers/fits.py
@@ -37,7 +37,9 @@ class FITSVirtualBackend(VirtualBackend):
         # handle inconsistency in kerchunk, see GH issue https://github.com/zarr-developers/VirtualiZarr/issues/160
         refs = KerchunkStoreRefs({"refs": process_file(filepath, **reader_options)})
 
-        refs = extract_group(refs, group)
+        # both group=None and group='' mean to read root group
+        if group:
+            refs = extract_group(refs, group)
 
         virtual_vars, attrs, coord_names = virtual_vars_and_metadata_from_kerchunk_refs(
             refs,
diff --git a/virtualizarr/readers/hdf5.py b/virtualizarr/readers/hdf5.py
index 0226a5e..eafcfb9 100644
--- a/virtualizarr/readers/hdf5.py
+++ b/virtualizarr/readers/hdf5.py
@@ -43,7 +43,9 @@ class HDF5VirtualBackend(VirtualBackend):
             filepath, inline_threshold=0, **reader_options
         ).translate()
 
-        refs = extract_group(refs, group)
+        # both group=None and group='' mean to read root group
+        if group:
+            refs = extract_group(refs, group)
 
         virtual_vars, attrs, coord_names = virtual_vars_and_metadata_from_kerchunk_refs(
             refs,
diff --git a/virtualizarr/readers/netcdf3.py b/virtualizarr/readers/netcdf3.py
index b90e67d..a32fb82 100644
--- a/virtualizarr/readers/netcdf3.py
+++ b/virtualizarr/readers/netcdf3.py
@@ -9,7 +9,6 @@ from virtualizarr.readers.common import (
     open_loadable_vars_and_indexes,
 )
 from virtualizarr.translators.kerchunk import (
-    extract_group,
     virtual_vars_and_metadata_from_kerchunk_refs,
 )
 from virtualizarr.utils import check_for_collisions
@@ -41,7 +40,11 @@ class NetCDF3VirtualBackend(VirtualBackend):
 
         refs = NetCDF3ToZarr(filepath, inline_threshold=0, **reader_options).translate()
 
-        refs = extract_group(refs, group)
+        # both group=None and group='' mean to read root group
+        if group:
+            raise ValueError(
+                "group kwarg passed, but netCDF3 files can't have multiple groups!"
+            )
 
         virtual_vars, attrs, coord_names = virtual_vars_and_metadata_from_kerchunk_refs(
             refs,
diff --git a/virtualizarr/readers/tiff.py b/virtualizarr/readers/tiff.py
index c475558..b9230e4 100644
--- a/virtualizarr/readers/tiff.py
+++ b/virtualizarr/readers/tiff.py
@@ -52,7 +52,9 @@ class TIFFVirtualBackend(VirtualBackend):
         # handle inconsistency in kerchunk, see GH issue https://github.com/zarr-developers/VirtualiZarr/issues/160
         refs = KerchunkStoreRefs({"refs": tiff_to_zarr(filepath, **reader_options)})
 
-        refs = extract_group(refs, group)
+        # both group=None and group='' mean to read root group
+        if group:
+            refs = extract_group(refs, group)
 
         virtual_vars, attrs, coord_names = virtual_vars_and_metadata_from_kerchunk_refs(
             refs,
diff --git a/virtualizarr/translators/kerchunk.py b/virtualizarr/translators/kerchunk.py
index 3b159e9..d818870 100644
--- a/virtualizarr/translators/kerchunk.py
+++ b/virtualizarr/translators/kerchunk.py
@@ -43,42 +43,43 @@ def virtual_vars_and_metadata_from_kerchunk_refs(
     return virtual_vars, ds_attrs, coord_names
 
 
-def extract_group(vds_refs: KerchunkStoreRefs, group: str | None) -> KerchunkStoreRefs:
-    """Extract only the part of the kerchunk reference dict that is relevant to a single HDF group"""
+def extract_group(vds_refs: KerchunkStoreRefs, group: str) -> KerchunkStoreRefs:
+    """
+    Extract only the part of the kerchunk reference dict that is relevant to a single HDF group.
+
+    Parameters
+    ----------
+    vds_refs : KerchunkStoreRefs
+    group : str
+        Should be a non-empty string
+    """
     hdf_groups = [
         k.removesuffix(".zgroup") for k in vds_refs["refs"].keys() if ".zgroup" in k
     ]
-    if len(hdf_groups) == 1:
-        return vds_refs
-    else:
-        if group is None:
-            raise ValueError(
-                f"Multiple HDF Groups found. Must specify group= keyword to select one of {hdf_groups}"
-            )
-        else:
-            # Ensure supplied group kwarg is consistent with kerchunk keys
-            if not group.endswith("/"):
-                group += "/"
-            if group.startswith("/"):
-                group = group.removeprefix("/")
-
-        if group not in hdf_groups:
-            raise ValueError(f'Group "{group}" not found in {hdf_groups}')
-
-        # Filter by group prefix and remove prefix from all keys
-        groupdict = {
-            k.removeprefix(group): v
-            for k, v in vds_refs["refs"].items()
-            if k.startswith(group)
-        }
-        # Also remove group prefix from _ARRAY_DIMENSIONS
-        for k, v in groupdict.items():
-            if isinstance(v, str):
-                groupdict[k] = v.replace("\\/", "/").replace(group, "")
 
-        vds_refs["refs"] = groupdict
+    # Ensure supplied group kwarg is consistent with kerchunk keys
+    if not group.endswith("/"):
+        group += "/"
+    if group.startswith("/"):
+        group = group.removeprefix("/")
+
+    if group not in hdf_groups:
+        raise ValueError(f'Group "{group}" not found in {hdf_groups}')
+
+    # Filter by group prefix and remove prefix from all keys
+    groupdict = {
+        k.removeprefix(group): v
+        for k, v in vds_refs["refs"].items()
+        if k.startswith(group)
+    }
+    # Also remove group prefix from _ARRAY_DIMENSIONS
+    for k, v in groupdict.items():
+        if isinstance(v, str):
+            groupdict[k] = v.replace("\\/", "/").replace(group, "")
+
+    vds_refs["refs"] = groupdict
 
-        return KerchunkStoreRefs(vds_refs)
+    return KerchunkStoreRefs(vds_refs)
 
 
 def virtual_vars_from_kerchunk_refs(
@@ -222,9 +223,17 @@ def find_var_names(ds_reference_dict: KerchunkStoreRefs) -> list[str]:
     """Find the names of zarr variables in this store/group."""
 
     refs = ds_reference_dict["refs"]
-    found_var_names = {key.split("/")[0] for key in refs.keys() if "/" in key}
 
-    return list(found_var_names)
+    found_var_names = []
+    for key in refs.keys():
+        # has to capture "foo/.zarray", but ignore ".zgroup", ".zattrs", and "subgroup/bar/.zarray"
+        # TODO this might be a sign that we should introduce a KerchunkGroupRefs type and cut down the references before getting to this point...
+        if key not in (".zgroup", ".zattrs", ".zmetadata"):
+            first_part, second_part, *_ = key.split("/")
+            if second_part == ".zarray":
+                found_var_names.append(first_part)
+
+    return found_var_names
 
 
 def extract_array_refs(


Test patch:
diff --git a/conftest.py b/conftest.py
index e86b924..0be0a89 100644
--- a/conftest.py
+++ b/conftest.py
@@ -24,6 +24,17 @@ def pytest_runtest_setup(item):
         )
 
 
+@pytest.fixture
+def empty_netcdf4_file(tmpdir):
+    # Set up example xarray dataset
+    ds = xr.Dataset()  # Save it to disk as netCDF (in temporary directory)
+    filepath = f"{tmpdir}/empty.nc"
+    ds.to_netcdf(filepath, format="NETCDF4")
+    ds.close()
+
+    return filepath
+
+
 @pytest.fixture
 def netcdf4_file(tmpdir):
     # Set up example xarray dataset
@@ -37,6 +48,18 @@ def netcdf4_file(tmpdir):
     return filepath
 
 
+@pytest.fixture
+def netcdf4_file_with_data_in_multiple_groups(tmpdir):
+    filepath = str(tmpdir / "test.nc")
+
+    ds1 = xr.DataArray([1, 2, 3], name="foo").to_dataset()
+    ds1.to_netcdf(filepath)
+    ds2 = xr.DataArray([4, 5], name="bar").to_dataset()
+    ds2.to_netcdf(filepath, group="subgroup", mode="a")
+
+    return filepath
+
+
 @pytest.fixture
 def netcdf4_files_factory(tmpdir) -> callable:
     def create_netcdf4_files(
diff --git a/virtualizarr/tests/test_backend.py b/virtualizarr/tests/test_backend.py
index 414ba80..0c054a3 100644
--- a/virtualizarr/tests/test_backend.py
+++ b/virtualizarr/tests/test_backend.py
@@ -5,7 +5,7 @@ import numpy as np
 import pytest
 import xarray as xr
 import xarray.testing as xrt
-from xarray import open_dataset
+from xarray import Dataset, open_dataset
 from xarray.core.indexes import Index
 
 from virtualizarr import open_virtual_dataset
@@ -309,6 +309,43 @@ class TestReadFromURL:
         xrt.assert_equal(dsXR, dsV)
 
 
+@requires_kerchunk
+def test_open_empty_group(empty_netcdf4_file):
+    vds = open_virtual_dataset(empty_netcdf4_file, indexes={})
+    assert isinstance(vds, xr.Dataset)
+    expected = Dataset()
+    xrt.assert_identical(vds, expected)
+
+
+@requires_kerchunk
+class TestOpenVirtualDatasetHDFGroup:
+    def test_open_subgroup(self, netcdf4_file_with_data_in_multiple_groups):
+        vds = open_virtual_dataset(
+            netcdf4_file_with_data_in_multiple_groups, group="subgroup", indexes={}
+        )
+        assert list(vds.variables) == ["bar"]
+        assert isinstance(vds["bar"].data, ManifestArray)
+        assert vds["bar"].shape == (2,)
+
+    def test_open_root_group_manually(self, netcdf4_file_with_data_in_multiple_groups):
+        vds = open_virtual_dataset(
+            netcdf4_file_with_data_in_multiple_groups, group="", indexes={}
+        )
+        assert list(vds.variables) == ["foo"]
+        assert isinstance(vds["foo"].data, ManifestArray)
+        assert vds["foo"].shape == (3,)
+
+    def test_open_root_group_by_default(
+        self, netcdf4_file_with_data_in_multiple_groups
+    ):
+        vds = open_virtual_dataset(
+            netcdf4_file_with_data_in_multiple_groups, indexes={}
+        )
+        assert list(vds.variables) == ["foo"]
+        assert isinstance(vds["foo"].data, ManifestArray)
+        assert vds["foo"].shape == (3,)
+
+
 @requires_kerchunk
 class TestLoadVirtualDataset:
     @pytest.mark.parametrize("hdf_backend", [HDF5VirtualBackend, HDFVirtualBackend])
@@ -356,8 +393,6 @@ class TestLoadVirtualDataset:
                     hdf5_groups_file, group="doesnt_exist", backend=hdf_backend
                 )
         if hdf_backend == HDF5VirtualBackend:
-            with pytest.raises(ValueError, match="Multiple HDF Groups found"):
-                open_virtual_dataset(hdf5_groups_file)
             with pytest.raises(ValueError, match="not found in"):
                 open_virtual_dataset(hdf5_groups_file, group="doesnt_exist")
 
diff --git a/virtualizarr/tests/test_integration.py b/virtualizarr/tests/test_integration.py
index 09a9fd8..71de89f 100644
--- a/virtualizarr/tests/test_integration.py
+++ b/virtualizarr/tests/test_integration.py
@@ -13,7 +13,6 @@ from virtualizarr.readers.hdf import HDFVirtualBackend
 from virtualizarr.tests import requires_kerchunk
 from virtualizarr.translators.kerchunk import (
     dataset_from_kerchunk_refs,
-    find_var_names,
 )
 from virtualizarr.zarr import ZArray
 
@@ -49,12 +48,6 @@ def test_kerchunk_roundtrip_in_memory_no_concat():
     xrt.assert_equal(roundtrip, ds)
 
 
-def test_no_duplicates_find_var_names():
-    """Verify that we get a deduplicated list of var names"""
-    ref_dict = {"refs": {"x/something": {}, "x/otherthing": {}}}
-    assert len(find_var_names(ref_dict)) == 1
-
-
 @requires_kerchunk
 @pytest.mark.parametrize(
     "inline_threshold, vars_to_inline",