Icechunk should fail early if user tries to write with consolidated metadata

[paraseba]

What happened?

A user set consolidated metadata at the root group by mistake. Icechunk accepted the change. Now new groups are invisible.

What did you expect to happen?

Failure when consolidated metadata is attempted

Environment

icechunk 0.2.15

[dcherian]

Context

"Consolidated metadata" means Zarr adds a new key to the groups' zarr.json that contains the metadata for all children. This allows quick reads of stores with large number of nodes.

Here is an example:

{'attributes': {},
'consolidated_metadata': {'kind': 'inline',
                         'metadata': {'zoo': {'attributes': {},
                                            'consolidated_metadata': {'kind': 'inline',
                                                                     'metadata': {},
                                                                     'must_understand': False},
                                            'node_type': 'group',
                                            'zarr_format': 3},
                                     'zoo/foo': {'attributes': {},
                                                'chunk_grid': {'configuration': {'chunk_shape': [3]},
                                                              'name': 'regular'},
                                                'chunk_key_encoding': {'configuration': {'separator': '/'},
                                                                      'name': 'default'},
                                                'codecs': [{'configuration': {'endian': 'little'},
                                                          'name': 'bytes'},
                                                          {'configuration': {'checksum': False,
                                                                           'level': 0},
                                                          'name': 'zstd'}],
                                                'data_type': 'int64',
                                                'dimension_names': ['x'],
                                                'fill_value': 0,
                                                'node_type': 'array',
                                                'shape': [6],
                                                'storage_transformers': [],
                                                'zarr_format': 3},
                                     'zoo/x': {'attributes': {},
                                              'chunk_grid': {'configuration': {'chunk_shape': [3]},
                                                            'name': 'regular'},
                                              'chunk_key_encoding': {'configuration': {'separator': '/'},
                                                                    'name': 'default'},
                                              'codecs': [{'configuration': {'endian': 'little'},
                                                        'name': 'bytes'},
                                                        {'configuration': {'checksum': False,
                                                                         'level': 0},
                                                        'name': 'zstd'}],
                                              'data_type': 'int64',
                                              'dimension_names': ['x'],
                                              'fill_value': 0,
                                              'node_type': 'array',
                                              'shape': [6],
                                              'storage_transformers': [],
                                              'zarr_format': 3}},
                         'must_understand': False},
'node_type': 'group',
'zarr_format': 3}

1. Consolidated metadata is technically out-of-spec (Added consolidated metadata to spec zarr-developers/zarr-specs#309) but is quite useful in practice.
2. Icechunk "conslidates metadata" by default --- it's all in the snapshot file.
3. Xarray calls zarr.consolidate_metadata by default and will not change (Consider changing default consolidated=None to False for zarr_version=3 in to_zarr() pydata/xarray#10122)
4. Consolidated metadata can get out of sync. I managed to get this to happen by (a) write some data + commit. (b) append some data with consolidated=False.

Possible Solutions

Error

We could raise when Zarr attempts to set a key consolidated_metadata on a Group's zarr.json. This is bad UX given Xarray's behaviour to consolidate metadata by default.

A blocker here (IMO) is that Xarray's behaviour to consolidate by default will result in really bad outcomes in pipelines, where the write session is now irrecoverable.

Warn

We could warn instead and prompt the user to set consolidated=False or just use to_icechunk (which sets consolidated=False).

Ignore

We could do the "right thing" and simply ignore any "consolidated_metadata" keys when set. Ideally Zarr would allow us to opt-out of consolidated but this hasn't been implemented yet (zarr-developers/zarr-python#2937)

[dcherian]

My vote is to "ignore" and then push on zarr-developers/zarr-python#2937 so we can explicitly opt-out.

[paraseba]

This makes sense to me @dcherian . So, can we simply clear the consolidated_metadata key in every group write? Just don't set the key?

[dcherian]

Yes i believe so. https://github.com/zarr-developers/zarr-python/blob/d19f3f0a825b5be986fa6e2f797aa4ddec0b90ed/src/zarr/core/group.py#L407-L409

[jhamman]

A few comments:

1. I prefer that we error or warn. Silently ignoring seems like it could end badly.
2. I'm not convinced the behavior in Xarray can't be corrected. I know there was some push back on the issue linked above but could we consider alternative approaches to solving the problem upstream?

[dcherian]

I think the correct upstream fix is: zarr-developers/zarr-python#2937. If that was implemented there would be no warning, which is why i prefer not warning now.

Zarr designs for the dumbest possible store, which Icechunk is emphatically not.

So I don't think we should ever set consolidated_metadata. By doing so we avoid a whole class of consistency bugs that Icechunk designs against.

We could choose to always return the consolidated_metadata field, since the metadata are 'consolidated' by default in Icechunk.

[paraseba]

I'm thinking for now to error with a nice error message, and then change behavior if we want when upstream is done. I think erroring would be much better than current. I don't like warning because of unattended jobs