Codecov Report

Attention: Patch coverage is 96.85039% with 4 lines in your changes missing coverage. Please review.

Project coverage is 94.69%. Comparing base (18f41d4) to head (3540434).
Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3257      +/-   ##
==========================================
+ Coverage   94.62%   94.69%   +0.06%     
==========================================
  Files          78       80       +2     
  Lines        8690     8817     +127     
==========================================
+ Hits         8223     8349     +126     
- Misses        467      468       +1     

this is awesome! A few high-level suggestions:

• can we have the name of the cli be just zarr, where convert is a subcommand:

  $ zarr --help
   # prints info including available commands
  $ zarr convert --help
  # info about the convert operation
  

• can the 2->3 migration just be a particular invocation of a more general command, like
  zarr convert source/data.zarr --in-place --zarr-format 3

  I'm not sure how to express the "clean up a broken conversion" process through this API, but it should be doable

• is "convert" the best name here? "resave" also seems good. I'm not opposed to "convert", just interested in exploring the space of names a bit.

Thanks for the comments @d-v-b !

• Happy to change the top-level name to zarr

• Just to check - are you suggesting it would be best to combine the convert and clear commands into one command? If so, then it should be possible with something like zarr convert source/data.zarr --in-place --allow-overwrite where:

  • --in-place would create v3 metadata + remove any v2 metadata (without this specified, both v2/v3 metadata would remain)
  • --allow-overwrite would remove any existing v3 metadata, then create new (otherwise it errors if an existing zarr.json is encountered)
  • I think the only situation this doesn't cover is if the conversion fails part way through + you just want to remove any zarr.json that were written. Perhaps it would be best to prevent this scenario from occurring in the first place? I could always run convert_v2_to_v3 inside a try / except, then run remove_metadata if any exceptions are encountered. This could be optional behaviour with something like --clear

• As for the naming - I don't have a strong preference here. I'd probably prefer convert as I think it's a simpler / easier to understand term, but happy to change this.

I think the only situation this doesn't cover is if the conversion fails part way through + you just want to remove any zarr.json that were written. Perhaps it would be best to prevent this scenario from occurring in the first place? I could always run convert_v2_to_v3 inside a try / except, then run remove_metadata if any exceptions are encountered. This could be optional behaviour with something like --clear

Attempting to clean up on failure is a good idea, but that can also fail. It would still be possible for the program to be interrupted while remove_metadata is being run, so I don't think there's anything we can do prevent conversion from failing. But as long as we are only creating json documents, and we aren't deleting the originals (the v2 metadata), then re-running should be cheap. This seems like a detail that we can iron out later on.

--in-place would create v3 metadata + remove any v2 metadata (without this specified, both v2/v3 metadata would remain)

There might actually be situations where people want the v2 and v3 metadata in the same hierarchy, and this is not against the rules of the spec. So I'd say that creating v3 metadata should be a separate (and precedent) operation from deleting the v2 metadata. --in-place should be considered shorthand for this:

$ zarr convert foo.zarr/bar foo.zar/bar

i.e., taking a v2 hierarchy from foo.zarr/bar, and creating a v3 hierarchy at foo.zarr/bar. Thus I think

$ zarr convert foo.zarr baz.zarr

i.e., resaving the hierarchy somewhere else, should also be supported. For now users would be responsible for copying chunks in this case.

--allow-overwrite would remove any existing v3 metadata, then create new (otherwise it errors if an existing zarr.json is encountered)

that seems right, but instead of --allow-overwrite I would just do --overwrite.

And a broader request: I am sure that while working on this kind of low-level tool, you find tasks that should be direct, but are made difficult by design choices in zarr-python, or the lack of low-level features. If you have the patience for it, please keep track of the stuff we could change in the core library to make this kind of tool easier to write, and feel free to open issues to track these features.

Ok - that all sounds good. How about the following for an updated CLI:

The base command is zarr convert path/to/input.zarr - this creates v3 metadata, writing it in-place back to input.zarr, leaving the v2 metadata as-is. There are then the following optional commands on top:

• --output path/to/output.zarr - if provided, v3 metadata will be written to this separate output location (leaving input.zarr completely unchanged). Only the metadata will be created (no copying of chunks).
• --overwrite: allows overwrite of v3 metadata at the output location (either path/to/input.zarr or path/to/output.zarr if provided)
• --remove-v2: removes any v2 metadata at the output location

I think it's probably worth also keeping the clear command to give maximum flexibility. I've certainly used it a lot during testing of the CLI e.g. when converting in-place, then doing some testing of the v3 output to check it looks ok, then later removing the v2 metadata when it's no longer needed.

• --output path/to/output.zarr - if provided, v3 metadata will be written to this separate output location (leaving input.zarr completely unchanged). Only the metadata will be created (no copying of chunks).

This design would make the in-place conversion the default, and writing the hierarchy to a new path would be an opt-in kind of thing. Speaking for myself, I think I would expect the convert-to-somewhere-else mode to be the default, and the in-place conversion to be opt-in. It might be helpful to sample opinions from other folks here (cc @zarr-developers/python-core-devs). In any case this is a minor thing.

I think it's probably worth also keeping the clear command to give maximum flexibility. I've certainly used it a lot during testing of the CLI e.g. when converting in-place, then doing some testing of the v3 output to check it looks ok, then later removing the v2 metadata when it's no longer needed.

That makes perfect sense. Since "clear" is kind of ambiguous on its own, and since the convert command is pretty narrowly scoped to converting from v2 to v3 data, I wonder if we should rename this command to something like convert-v3, migrate-v3, etc, something that makes it really obvious that all the options will be scoped to the process of moving from v2 data to v3 data. This would free up the convert command for later use, e.g. things like changing chunking, compression, etc.

also, in case anyone wants to try this tool out, here's a 1-liner with uvx:

uvx --with typer --from git+https://github.com/K-Meech/zarr-python/@km/v2-v3-conversion zarr-converter --help