Confusion about the dimension_separator keyword

[constantinpape]

I don't really understand how to use the new dimension_separator keyword, in particular:

1. Creating a DirectoryStore(dimension_separator="/") does not have the effect I would expect (see code and problem description below).
2. Why does zarr still have the NestedDirectoryStore? Shouldn't it be the same as DirectoryStore(dimension_separator="/")? Hence I would assume that NestedDirectoryStore could either be removed or (if to be kept for legacy purposes) should just map to DirectoryStore(dimension_seperator="/").

Minimal, reproducible code sample, a copy-pastable example if possible

import zarr
store = zarr.DirectoryStore("test.zarr", dimension_separator="/")
g = zarr.open(store, mode="a")
ds = g.create_dataset("test", shape=(10, 10, 10))
ds[:] = 1

Problem description

Now, I would assume that the chunks are nested, but I get:

$ ls test.zarr/test
0.0.0

but to, to my confusion, also this:

$ cat test.zarr/test/.zarray
...
"dimension_separator": "/",
...

If I use NestedDirectoryStore instead, the chunks are nested as expected.

Version and installation information

Please provide the following:

• Value of zarr.__version__: 2.8.3
• Value of numcodecs.__version__: 0.7.3
• Version of Python interpreter: 3.8.6
• Operating system: Linux
• How Zarr was installed: using conda

[joshmoore]

2. Why does zarr still have the NestedDirectoryStore?

To prevent a breaking change.

I would assume that the chunks are nested, but I get:

$ ls test.zarr/test
0.0.0

Agreed. This surprises me as well.

[joshmoore]

So, I think DirectoryStore(dimension_separator="/") is probably just untested. Likely since NestedDirectoryStore exists. So if we do want to deprecate NDS, then DS will need to make use of this helper:

def _nested_map_ckey(key):
    segments = list(key.split('/'))
    if segments:
        last_segment = segments[-1]
        if _prog_ckey.match(last_segment):
            last_segment = last_segment.replace('.', '/')
            segments = segments[:-1] + [last_segment]
            key = '/'.join(segments)
    return key

An alternative would be to disallow "/" as a separator with the error message, "use NestedDirectoryStore".

[constantinpape]

Thanks for looking into this, @joshmoore. In my opinion, the issue with DirectoryStore(dimension_separator="/") should be fixed rather than disallowing it.
I would also vote for deprecating NestedDirectoryStore or simplifying it and making sure that it uses the same implementation as DirectoryStore, e.g. something like

class NestedDirectoryStore(DirectoryStore):
  def __init__(...):
     super().__init__(..., dimension_separator="/")

[constantinpape]

To add to this issue, the same problem exists when reading the data.
Given a zarr array at test.zarr/test with dimension_separator=/, the following does not work correctly:

import zarr
f = zarr.open('test.zarr')
data = f['test'][:]

The data returned will be all zeros (or more generally all fill_value).
Conversely, this works as expected:

import zarr
store = zarr.storage.NestedDirectoryStore('test.zarr')
f = zarr.open(store)
data = f['test'][:]

[joshmoore]

Yeah, that definitely counts as a bug. I almost wonder if it wouldn't be easiest to use FSStore to re-implement (Nested)DirectoryStore.

cc: @martindurant

[martindurant]

Maybe, but also it can be tricky to have an implementation that does too many things, with a load of options. Is that better or worse than a set of specific implementations?

When I initially trialled FSStore, it did have more options, that I removed for want of testing. fsspec is a light dependency, but I imagine people would still be surprised to need it for local files (unless zarr were to have a hard requirement).

[joshmoore]

(Working on a failing test & a fix now)

[joshmoore]

So the failing test is pretty straight-forward, but I'm beginning to fear that this use case shows that the Array and not the Store should be in charge of this information. (Not to mention what happens when a single store is asked to open multiple arrays with different separators)

Edit: pondering out loud, an alternative would be to have a Key type which could carry this metadata, but other Store implementations wouldn't know what to do with this by default.

[martindurant]

Agreed, we need to sort out to what extent zarr metadata should describe not just how to turn bytes into arrays, but details of how it is laid out in the storage backend. I am ambivalent - maybe the user just "needs to know", so that the metadata of a copy on a different backed is the same.

Actually, an over-layer describing how to open a dataset works and is arguably the right place to separate out this kind of option. I would propose that Intake does this well.

[joshmoore]

Just thinking what that would mean for the likes of dask/dask#7673, @martindurant, would you pass an Intake URL then to from_zarr?

[martindurant]

Oh no! I mean, I'd love it, but Dask doesn't want that...

I mean that the intake prescription could include url="file:///local/path", dimension_separator="/" to ensure picking the right thing. To the user, it's not obvious that including "file://" picks FSStore, and that this store can make use of the extra parameter, which is why you'd want to hide this kind of archena in a catalogue.

The most you'd want to do directly on the dask side is to pass on arguments to zarr as faithfully as you can, and provide good documentation around it. If the user needs to instantiate their own store instances, the'll have to do some digging.

[joshmoore]

Ok. It's seeming then like "encoded at a higher-level" ends up being a reversal of all the work that has gone in since #715. If there are others that are similarly worried about the addition of dimension_separator and are pondering a rollback, then it'd be good to know. cc: @zarr-developers/core-devs

Otherwise, I'm going to push forward with trying to make it behave sanely as currently implemented with the goal of having it behave the same across language implementations and without the user needing to know that an array was saved with nested vs. flat.

[martindurant]

I am not particularly advocating the "higher level", sorry if it sounded like it; we can surely fix the code in dask and anywhere else to "detect" the situation too. The Intake thing is like a workaround for cases where we don't want to change the implementation. As extra arguments for storage go, this one is pretty low-level, almost at the C/fortran layout layer. The argument was, that since it affects the names of the contained files, you couldn't simply copy the whole dataset anyway.

Other storage-oriented arguments are more divorced from that layer or specific to the storage, such as "consolidated" (which we support as an optional argument, but is not in the spec; it does survive copies) and "requester-pays" for an S3 dataset I happen to be looking at today.

[joshmoore]

"requester-pays" for an S3 dataset I happen to be looking at today.

Interesting! Please ping me on related issues when the time comes.

I am not particularly advocating the "higher level", sorry if it sounded like it

Understood. Thanks for the clarification.