Confusion about the dimension_separator keyword

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

Creating a DirectoryStore(dimension_separator="/") does not have the effect I would expect (see code and problem description below).
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

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.

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".

@joshmoore

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="/")

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'][:]

@martindurant

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

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).

(Working on a failing test & a fix now)

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.

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.

@martindurant

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

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.

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.

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.

"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.

@d-v-b

@d-v-b, if you still have any of the state of this loaded into your working memory after #709, it'd be interesting to here your opinion, since I vaguely remember you arguing that it was in fact a Store concern rather than a core.py concern.

@joshmoore

True, but then we must worry about keeping the two in sync!

Yes, that's true. But that still sounds preferable to the other downsides discussed above. And I think that would be a good solution if we decide not to allow mixed separators per group, which I am in favor of.

Let's see what @joshmoore and @d-v-b think about this.

@joshmoore

if you still have any of the state of this loaded into your working memory after #769, it'd be interesting to here your opinion, since I vaguely remember you arguing that it was in fact a Store concern rather than a core.py concern.

@joshmoore this appears to link back to the current issue. Was there another issue this was intending to refer to? (just trying to get some missing context here)

@jakirkham

Thanks for catching that, @jakirkham. #709 was what I meant. Fixed above.

Introduce a more sophisticated Key concept (Key object or object with dictionary) to pass the dimension_separator from the Array

What about a length-2 tuple containing the array key and the chunk key, e.g. ('path/to/array/', '0.0.0') vs ('path/to/array/', '0/0/0')? This is explicit enough to allow the chunk renaming I need for n5.

In terms of new APIs that would certainly work for me, but it would be a pretty significant change.

WRT to the netcdf-c implementation of dimension separator.

Internally, I end up breaking the chunk key into two parts as mentioned above. Effectively (/a/b/a, 0D0D0) where D is the dimension separator.
I have to use a heuristic to figure out what part of the key is the chunk id. That heuristic looks for the trailing occurrence of [0-9]+D[0-9]+D... where D is the dimension sep. This has the bad consequence that if D = / then you cannot have an array whose name is a number.

@d-v-b

@d-v-b: sorry, I've been failing to find the time to really engage here, however the /-community is still quite broken. Do you have a branch/repository that I could test against my #773 and see if I can find a nice workaround for the N5 side of things?

@DennisHeimbigner : thanks for the info. If we're willing to break the API, then yet, I definitely see the value in clearly splitting the key into its individual parts.

In another issue (which I can dig up) there was a pretty strong vote against @jbms (#715 (comment)) that we not try the heuristics (as I had tried with a PR with @SabineEmbacher)

cc: @haileyajohnson

Actually splitting the key occurs completely internally, so the user never sees it.
As I mentioned, the price is that certain variable names become "illegal".

@joshmoore

@joshmoore I will put together a branch that you can test against. And I think I figured out a stupid hack that lets a store infer the key separator even if it changes on the array side.

To summarize this issue as I understand it:

different dimension separators are good for different storage backends.
It seems like "/" is better for most (but not all) storage backends than ".".
As far as I know, the informed choice of dimension separator is governed by the details of a given storage backend, therefore it is probably not important to support multiple dimension separators in the same zarr container. I.e., the dimension separator is a property of the whole container, not a property of all the arrays in that container.
However, at present, It looks like array metadata is the only place the dimension separator can be stored, because of the constraint that root.zarr/bar/baz/array be accessible without touching the metadata of root.zarr. Because we think of the dimension separator reflecting a container-wide constraint, and because we want arrays to be abstracted from the details of chunk storage, it's awkward to put the dimension separator in array metadata, because this leads to duplicated information and an abstraction leakage that complicates stuff like n5 support.

I would be interested in understanding the pros and cons of fixing this via storage metadata (i.e. some root metadata that contains information about storage for all elements of the container). If store-specific details (like dimension separator) were stored in root.zarr/.zgroup, and initializing a store involved reading that metadata, what would break?

If store-specific details (like dimension separator) were stored in root.zarr/.zgroup, and initializing a store involved reading that metadata, what would break?

The obvious one would be an attempt to move (or copy) the dataset/file hierarchy from one storage system to another. Such a move is not necessarily made by a thing that has any knowledge of zarr and its conventions.

I will put together a branch that you can test against. And I think I figured out a stupid hack that lets a store infer the key separator even if it changes on the array side.

💯

As far as I know, the informed choice of dimension separator is governed by the details of a given storage backend, therefore it is probably not important to support multiple dimension separators in the same zarr container. I.e., the dimension separator is a property of the whole container, not a property of all the arrays in that container.

At least at a specification level, this is neither the case in V2 nor V3 currently. It's an array concern.

I would be interested in understanding the pros and cons of fixing this via storage metadata (i.e. some root metadata that contains information about storage for all elements of the container). If store-specific details (like dimension separator) were stored in root.zarr/.zgroup

root.zarr is not necessarily a group but can be an array, which is another way of saying that in v2 there's no clear place for such store-level metadata.

Just chiming in to say I also hit the issue addressed by #773 !

... you should always be able to access the path of the array directly without knowing it is within a group.

This is certainly the current behaviour, but it's not the case in V3 is it?

The obvious one would be an attempt to move (or copy) the dataset/file hierarchy from one storage system to another. Such a move is not necessarily made by a thing that has any knowledge of zarr and its conventions.

Going into V3, we lose the ability to use such tools anyway, right (which is a shame)? Because all the metadata is stored in a directory hierarchy parallel to the groups/arrays.

@joshmoore

Another +1 and thanks to @joshmoore for #773, which addressed the behavior I observed when trying to use DirectoryStore and dimension_separator='/' with zarr and xarray.

Are there plans to integrate #773?

After a lengthy back and forth, I think this is a last call for opinions on the fix. I'd propose to get it out in 2.9.0 (noting that that also drops support for Python 3.6)

Navigation Menu

Repository navigation

Confusion about the dimension_separator keyword #769

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

Problem description

Version and installation information

10 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Confusion about the dimension_separator keyword #769

Description

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

Problem description

Version and installation information

Activity

joshmoore commented on Jun 5, 2021

joshmoore commented on Jun 5, 2021

constantinpape commented on Jun 6, 2021

constantinpape commented on Jun 10, 2021

joshmoore commented on Jun 10, 2021

martindurant commented on Jun 10, 2021

joshmoore commented on Jun 14, 2021

joshmoore commented on Jun 14, 2021

martindurant commented on Jun 14, 2021

joshmoore commented on Jun 14, 2021

martindurant commented on Jun 14, 2021

joshmoore commented on Jun 14, 2021

martindurant commented on Jun 14, 2021

joshmoore commented on Jun 14, 2021

joshmoore commented on Jun 14, 2021

10 remaining items

constantinpape commented on Jun 16, 2021

jakirkham commented on Jun 16, 2021

joshmoore commented on Jun 17, 2021

d-v-b commented on Jun 19, 2021

joshmoore commented on Jun 21, 2021

DennisHeimbigner commented on Jun 28, 2021

joshmoore commented on Jun 30, 2021

DennisHeimbigner commented on Jun 30, 2021

d-v-b commented on Jul 2, 2021

martindurant commented on Jul 2, 2021

joshmoore commented on Jul 2, 2021

clbarnes commented on Jul 22, 2021

thewtex commented on Aug 11, 2021

joshmoore commented on Aug 19, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions