WIP: Make DictStore the default Array store

[jakirkham]

Based on discussion in issue ( #349 ), it sounds like we are ok moving to DictStore to back Arrays. This makes that change.

As the change depends somewhat on PR ( #350 ), this has been marked as WIP until we resolve that one. Can revisit afterwards.

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• Docs build locally (e.g., run tox -e docs)
• AppVeyor and Travis CI passes
• Test coverage is 100% (Coveralls passes)

[jakirkham]

Something to consider here is what we do when a different in-memory MutableMapping is provided (e.g. dict or OrderedDict). One thought is that we construct a new DictStore and copy the data over. This should catch any non-conforming cases and ensure that everything conforms to the spec afterwards.

Another thing to consider is a copy was introduced in commit ( 3c00d52 ) to ensure the underlying data of an in-memory store is not mutated. However if we always use a DictStore for in-memory data and that store always guarantees to contain immutable blobs of data ( e.g. bytes as in PR #350 ), then we can drop this copy as it will be taken care of for us.

This latter thought re-raises the point that we should allow bytes passed through by filters/compressors to remain untouched so as to avoid a copy later.

[alimanfoo]

Something to consider here is what we do when a different in-memory MutableMapping is provided (e.g. dict or OrderedDict). One thought is that we construct a new DictStore and copy the data over. This should catch any non-conforming cases and ensure that everything conforms to the spec afterwards.

Not sure what to do in that case, need to mull it over. Might be a bit odd for the user if we ended up copying data across and using a different object as the store, i.e., user would be expecting to see data in the object they provided as store, but we ended up using something else. The alternative would be to just try and use whatever object is provided as a store. If it doesn't fully conform to the spec (e.g., plain dict), so be it, the user may get unexpected behaviour.

Another thing to consider is a copy was introduced in commit ( 3c00d52 <3c00d52> ) to ensure the underlying data of an in-memory store is not mutated. However if we always use a DictStore for in-memory data and that store always guarantees to contain immutable blobs of data ( e.g. bytes as in PR #350 <#350> ), then we can drop this copy as it will be taken care of for us.

Agree we could probably drop that copy if we know that DictStore will be ensuring bytes.

This latter thought re-raises the point <#348 (comment)> that we should allow bytes passed through by filters/compressors to remain untouched so as to avoid a copy later.

+1 to restore bytes output from compression codecs.

[jakirkham]

Good point. Some random thoughts.

1. What if we had a generic wrapper class for MutableMappings that exposed a MutableMapping API overriding a couple function to ensure they behaved safely (e.g. __setitem__)?
2. What if we merely enforce that Array provides bytes to the store?
3. Should we validate input stores meet the spec somehow?

[alimanfoo]

My 2c...

1. What if we had a generic wrapper class for MutableMappings that exposed a MutableMapping API overriding a couple function to ensure they behaved safely (e.g. __setitem__)?

I'm not sure it's necessary. E.g., we already normalise storage paths within the core and hierarchy modules before we ever interact with the storage layer, so we can be sure we'll never send an invalid key to a store.

2. What if we merely enforce that Array provides bytes to the store?

I'm not sure it's necessary to be so strict. Although I suppose that we could validate that whatever value comes out the end of the chunk encoding pipeline is an object that supports the buffer protocol, and raise some kind of encoding error if not, prior to sending the value to the storage layer.

3. Should we validate input stores meet the spec somehow?

Personally I think it would be good to provide some support for developers to test their storage class complies with the spec. But at runtime, let people provide whatever type of object they choose to provide as a store, and hope it quacks like the right kind of duck.

[alimanfoo]

One other thought, I wondered if it might be worth renaming DictStore to MemoryStore so it was more obvious to new users what kind of storage this was (leaving DictStore as an alias for backwards-compatibility).

[jakirkham]

One other thought, I wondered if it might be worth renaming DictStore to MemoryStore so it was more obvious to new users what kind of storage this was (leaving DictStore as an alias for backwards-compatibility).

+1 Could also do MemStore if we want to be succinct.

Raised as issue ( #356 ).

[jakirkham]

This should be ready for a closer look now that PR ( #350 ) is in.

[jakirkham]

There are still some test cases that use dict-based chunk storage for Zarr Arrays. So have added a workaround at the Zarr Array level to special case dict handling.

Though I'm not sure if we shouldn't just go for those two changes alone to fix this issue. Have pulled them out in PR ( #359 ) just in case.

Edit: To be clear, the dict-based chunk store workaround is no longer included in this PR.

[jakirkham]

We can certainly go ahead with this as discussed. Though we may also want to consider adding a workaround for dict to ensure immutable values as done in PR ( #359 ) either in addition to this PR or instead of it.

[jakirkham]

Added to the v2.3 milestone just to track it. Happy to change this as needed.

[jakirkham]

Have pushed some changes that may be considered breaking. So have removed it from the milestone for now.

[joshmoore]

@jakirkham @grlee77 : is it fair to say this has been superceded?

[jakirkham]

Yeah I think so. We can always revisit if needed (likely we would need a new PR at this point)

Edit: For context PR ( #789 ) addressed this.