Skip to content

Have a BaseStore class ? #600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Carreau opened this issue Sep 9, 2020 · 5 comments
Closed

Have a BaseStore class ? #600

Carreau opened this issue Sep 9, 2020 · 5 comments
Assignees

Comments

@Carreau
Copy link
Contributor

Carreau commented Sep 9, 2020

Thi is a suggestion to have a BaseStore class or ABC, that store implementations can inherit from to have basic functionality.

Problem description

Saying that stores are "just" MutableMapping may be insufficient, a number of places call methods on stores that are not available in MutableMapping, which:

  • Make static type Checking fails if one uses MutableMapping as a type annotation.
  • Would provide a default methods, like close() method that is no-op, and would avoid the need to check for method existence.
  • Avoid reimplementation of boilerplate :def __len__(self): return len(self.keys()), still allowing overwrite.
@jakirkham
Copy link
Member

Also issue ( #573 ) and issue ( #349 ) are related.

@jakirkham
Copy link
Member

One thing that might be nice is to have a store that wraps a minimal MutableMapping with the extra bits of this ABC. That way users still have a path for using a minimal MutableMapping in this new framework.

@joshmoore
Copy link
Member

@Carreau Carreau self-assigned this Sep 18, 2020
@Carreau
Copy link
Contributor Author

Carreau commented Sep 21, 2020

there seem to be a number of place where the test assumes that stores can do things quite differently than what I was expecting and where dict, pass the test, but MemoryStore does not.

Typically things like


def test_getsize():
    ...

    store = # Put dict() or MemoryStore() here
    store['boo'] = None
    assert -1 == getsize(store)

Will pass for dict, but MemoryStore will refuse to store None.

or

from zarr.storage import MemoryStore, init_array
from zarr import Array

store = # Put dict() or MemoryStore() here
init_array(store, path="foo/bar", shape=100, chunks=10)
z =  Array(
    store,
    path="foo/bar"
)

z[:] = 42
z.store['foo'] = b'bar'
z[:] = 43

Where MemoryStore will be quite unhappy because z.store['foo'] = b'bar' will mess-up the hierarchy, but dict won't call "requires parent" so works fine.

So i'm unsure which is the "correct" behavior.

@jakirkham
Copy link
Member

Done in PR ( #789 ) and preceding work

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

No branches or pull requests

3 participants