Skip to content

Read + Write Zarr with consolidated metadata - #3066

Closed
mannreis wants to merge 46 commits into
Unidata:mainfrom
mannreis:zarr-csl
Closed

Read + Write Zarr with consolidated metadata#3066
mannreis wants to merge 46 commits into
Unidata:mainfrom
mannreis:zarr-csl

Conversation

@mannreis

@mannreis mannreis commented Dec 17, 2024

Copy link
Copy Markdown
Contributor

This PR is motivated by #2987 and it is a follow up on the closed PR2992. It infers if the dataset is consolidated and acts accordingly. The implementations was inspired on the developments in Zarr3 support (by @DennisHeimbigner) which could simplify adding the same feature on the next version.

In short, this PR adds a layer (NCZMD, for NetCDF ZarrMetaData) that implements:

  • Listing of variables, groups and attributes
  • Fetching content of the several Zarr metadata json files (.zattrs,.zgroup,.zarray)

This layer would be extended in the same way for writing (updating internal consolidated json and sync it on closure)

Depending on the existence of /.zmetadata the operations above are either process from it's content, or done directly on the storage, via zmap.

The feature above allows the S3 client implementation to be used against to vanilla HTTP servers, when authentication is out of the picture. But such is only possible because of 0832d450d207223fe43a9ee619bb722f9a29bff8, which avoids the S3 ListObjects.


As an example on how to produce a consolidated dataset in python:

import zarr
import numpy as np

name = f'test-{zarr.__version__}'
z = zarr.open(name, mode='w')
print(name)
z.attrs['Description'] = 'Consolidated zarr test'
G1 = z.create_group('G1')
G1.attrs['Details'] = 'Variables are chunked'
v1 = G1.create_group('subg1')
v1.array('myarray', np.arange(90, dtype='i4').reshape(6, 15), chunks=(6,15), compressor=None)
G2 = z.create_group('G2')
G2.array('other variable with spaces',np.arange(15).reshape(3, 5), compressor=None)
zarr.consolidate_metadata(z.store)

This can be used to check if the reading output remains the same after (re)moving the .zmetadata

ncdump file://test-2.18.2#mode=zarr > csl.out
mv test-2.18.2/.zmetadata .
diff csl.out <(ncdump file://test-2.18.2#mode=zarr)

Similar is done on 6346e91 taking into accound zip and file modes. Integrated tests exercising S3 are limited on my side (i'll try to add some here). However I have used it against my own endpoints and it seems to be functional.


Edit: set no compressor otherwise Blosc is used by default and adds dependency for the tests

@WardF

WardF commented Dec 17, 2024

Copy link
Copy Markdown
Member

@DennisHeimbigner Failures in the code preventing compilation aside, I'd be interested in your thoughts on this, particularly in advance of our scheduled conversation with @mannreis and Flo re: consolidated metadata. Thanks!

@WardF WardF left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mannreis I will take a look at the compilation failures in the next couple of days and pitch in where I can. I'm going to convert this to a draft PR for the time being, until we have the compilation and tests passing. Thanks!

@WardF
WardF marked this pull request as draft December 17, 2024 18:12
@DennisHeimbigner

Copy link
Copy Markdown
Contributor

In our meeting this morning, you indicated that you had modified ncjson
to make dict insertions faster. Can you point me to that code?

@mannreis

mannreis commented Dec 18, 2024

Copy link
Copy Markdown
Contributor Author

I mentioned that with respect to my developments on write operations. And the main concern wasn't speed but key-value duplication when inserting a value with an already existing key: 57bf0b9. I'll merge the write functionality to this branch and rename the PR to Read + Write then.

@mannreis mannreis changed the title Read Zarr with consolidated metadata Read + Write Zarr with consolidated metadata Dec 18, 2024
@WardF

WardF commented Dec 23, 2024

Copy link
Copy Markdown
Member

I've merged the latest main into this branch to capture the work done to incorporate various updates to the Github Action work.

@DennisHeimbigner

Copy link
Copy Markdown
Contributor

See draft pr #3068

@mannreis

Copy link
Copy Markdown
Contributor Author

@WardF this PR is already reaching if not crossed the limit of what I consider re-viewable.

Now I'd focus on removing, improving and documenting rather than adding.

Both read and write are supported but only if getenv("NCZARR_CONSOLIDATE") is truthy or if mode=consolidate so I'd consider this PR as safe.

Doing so would also align with the zarr-python functions open vs open_consolidated

I could further align the implementation with 3068 but I'm more comfortable going with this one for the moment and opening PRs solely for refactoring

Some minor but importatn things that need some attention are:

  • Nomeclatures (like NCZMD_* which I introduced but don't like as it increases cognitive load
  • Conventions in general, in functions and directives like NCZARR_CONSOLIDATED_*, Z2* and so on

@mannreis
mannreis marked this pull request as ready for review November 28, 2025 11:55
@mannreis
mannreis marked this pull request as draft November 28, 2025 11:55
@DennisHeimbigner

Copy link
Copy Markdown
Contributor

We need a better plan for this.
First, ignore 3068. We will move to V3 in the future.
Second, we need a list of relatively small steps to get the zarrmetadata stuff added.
Suppose we do something like this.

  1. add just the metadata dispatcher code plus the necessary changes to support current non-zmetadata case.
  2. then add the zarrmetadata code.

Does that work. Can you suggest other small steps?

@WardF WardF modified the milestones: 4.9.3, v4.10.0 Dec 2, 2025
@mannreis

mannreis commented Dec 2, 2025

Copy link
Copy Markdown
Contributor Author
  1. add just the metadata dispatcher code plus the necessary changes to support current non-zmetadata case.
  2. then add the zarrmetadata code.

Does that work. Can you suggest other small steps?

This feature is disabled by default, meaning it should be fairly safe to add.
If it makes it easier to proceed I would split 2. into:
2. Add consolidated read only
3. Add Consolidate write

Then create branches zarr-csl-{1...} with the respective parts taken from this PR.

Hopefully each PR could be easier and quicker to review. Let me know your take @WardF

The design won't change much @DennisHeimbigner. If you have already any suggestion feel free to start a "review" and highlight whatever you'd like modified

Comment thread libnczarr/zmetadata.h
int zarr_format; /* Zarr format version */
int dispatch_version; /* Dispatch table version*/
size64_t flags; /* Metadata handling flags */
NCjson *jcsl; // Consolidated JSON view or NULL

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DennisHeimbigner I merged NCZ_Metadata_Dispatcher and NCZ_Metadata, adding the NCjson* in the same struct. Was there any reason to keep it separate? This way we keep a single layer instead of two.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be ok. As a rule, I make the dispatch table itself constant, but its a small issue.

@DennisHeimbigner

Copy link
Copy Markdown
Contributor

I am thinking about miimizing the size of each PR. 3-4 small PRs may be preferable to 1 larger one.
Especially with respect to reviewing.

@mannreis

mannreis commented Dec 4, 2025

Copy link
Copy Markdown
Contributor Author

I am thinking about miimizing the size of each PR. 3-4 small PRs may be preferable to 1 larger one.

I went ahead and split this PR:

This was a bit of an puzzle for me but should make life easier for someone. I've rebased them on each other thus you see repeated commits that would be skipped if already merged. Here's a summary:

echo Part 1: $(git diff main...zarr-csl-1 --shortstat); 
for i in {2..7}; do
 echo Part $i: $(git diff zarr-csl-$((i-1))...zarr-csl-${i} --shortstat); 
done
Part 1: 9 files changed, 39 insertions(+), 33 deletions(-)
Part 2: 5 files changed, 156 insertions(+)
Part 3: 4 files changed, 287 insertions(+)
Part 4: 2 files changed, 9 insertions(+)
Part 5: 3 files changed, 55 insertions(+), 115 deletions(-)
Part 6: 1 file changed, 192 insertions(+), 6 deletions(-)
Part 7: 6 files changed, 72 insertions(+), 5 deletions(-)

Part 1 to 3 add dead code which starts being used in Part 4 and 5.
Parts 6 and 7 take tackle the consolidated.

Glad to get some feedback and get things moving :)

@DennisHeimbigner

Copy link
Copy Markdown
Contributor

Let me know when you reach a reasonably steady state of PRs so I can begin reviewing them.

@mannreis

mannreis commented Dec 4, 2025

Copy link
Copy Markdown
Contributor Author

Let me know when you reach a reasonably steady state of PRs so I can begin reviewing them.

They are ready! I need to rebase the later ones but so will I need, once there are changes.

Thanks a lot!!!

@DennisHeimbigner

Copy link
Copy Markdown
Contributor

I hate to propose it but you are staging each zarr-csl-xx against Unidata/main rather than the previous
zarr-cl-(xx-1). This is a bit irritating because I have to review the cumulative changes for every zarr-csl-xx
rather than just the delta changes against the previous zarr-csl-(xx-1).

@mannreis

mannreis commented Dec 4, 2025

Copy link
Copy Markdown
Contributor Author

I hate to propose it but you are staging each zarr-csl-xx against Unidata/main rather than the previous
zarr-cl-(xx-1). This is a bit irritating because I have to review the cumulative changes for every zarr-csl-xx
rather than just the delta changes against the previous zarr-csl-(xx-1).

I can do that but then the PR will be my fork as I can't create branches upstream.

Another option is to review one at a time and once you're happy and the merge takes place I rebase the next one and you get a clean history.

Yet another option is to trim each of zarr-csl-x to have only the their commits. This option may not even build in some cases...

I'm not so experienced with scaffolding PRs but I'm learning.

Tomorrow I'll address what you've reviewed so far. Many thanks Dennis!

@DennisHeimbigner

Copy link
Copy Markdown
Contributor

Good point. I guess I will do the deltas by hand rather than using the PR file changes.

@WardF

WardF commented Dec 5, 2025

Copy link
Copy Markdown
Member

I think at this point, maintaining PR's against main is going to be best so that we don't end up in the situation we ended up in last time, e.g. changes that have deviated too far from main. There will be other PR's from other contributors going into main after all, and depending how long this takes, that could result in pretty big deviations. Let's keep PR's against main and just continue to engage in active communication :)

@mannreis mannreis closed this Dec 11, 2025
@mannreis

Copy link
Copy Markdown
Contributor Author

Merged via #3225

@mannreis
mannreis deleted the zarr-csl branch July 27, 2026 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants