Group.nmembers returns incorrect value for consolidated metadata

[csparker247]

Zarr version

v3.1.0

Numcodecs version

v0.16.1

Python Version

3.11

Operating System

Mac

Installation

Using pip into a virtual environment

Description

When loading a group of groups where the top-level group has consolidated metadata, the result of Group.nmembers() includes nested members. This appears to be some issue with ConsolidatedMetadata.flattened_metadata as flattened_metadata.keys() returns children of the first-level members as well.

Steps to reproduce

# /// script
# requires-python = ">=3.11"
# dependencies = [
#   "zarr@git+https://github.com/zarr-developers/zarr-python.git@main",
# ]
# ///
#
# This script automatically imports the development branch of zarr to check for issues

import zarr

# create the group on disk
z = zarr.group('foo', zarr_version=3)
for idx in range(0, 10):
    idx_str = f'{idx:05d}'
    ds = z.create_group(name=idx_str)
    ds.create_array(f'data', dtype='uint8', shape=(10, 10))
zarr.consolidate_metadata(z.store)
print('num members at creation:', z.nmembers())

# load from disk
z = zarr.open('foo')
print('num members at load:', z.nmembers())

# print the flattened metadata keys
print()
print(z.metadata.consolidated_metadata.flattened_metadata.keys())

Additional output

No response

[TomAugspurger]

Thanks for the report. I think that we could update

zarr-python/src/zarr/core/group.py

Line 1310 in 9ae874c

return len(self.metadata.consolidated_metadata.flattened_metadata)

to consider the number of / in the paths, since that's the analog for the depth. Something like return len([x for x in self.metadata.consolidated_metadata.flattened_metadata if x.count("/") <= max_depth])

@csparker247 are you interested in making a PR with that + a test?

[csparker247]

@TomAugspurger Looks good. To confirm, the zarr spec explicitly treats all / as a hierarchy delineation correct? You can't create a key that's something like this/that. The spec looks like that way to me, so just making sure as I've primarily operated as a user.

Not sure if I'll have the time soon. I'll see what I can do in the next day or so.

[TomAugspurger]

Correct: node names can't contain a / so it should be safe to treat the number of / as the depth.

[csparker247]

@TomAugspurger I had some time today, so I implemented your fix + what I think is a good test.