forward write_empty_chunks kwarg in group.require_dataset

[d-v-b]

Currently when using a Group to get an existing Array via Group.require_dataset, there's no way to control the "write_empty_chunksness" of the array (since Array.write_empty_chunks is not part of the array metadata). This means that you cannot call group.require_dataset('foo', shape=10, dtype='i4', write_empty_chunks=False) and get an array that has the desired .write_empty_chunks property (instead the write_empty_chunks kwarg is ignored).

A few other array properties are similar (synchronizer, cache_metadata, and cache_attrs), and in main these keyword arguments are extracted from the **kwargs argument to Group.require_dataset before being passed to the Array constructor. This PR expands this behavior to include write_empty_chunks, thereby enabling the control of the write_empty_chunks property of the arrays returned by Group.require_dataset.

I also added a lot of type annotations.

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
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[d-v-b]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

[joshmoore]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

• Happy to hear opinions but depending on when you are targeting this for, I'd be inclined to hold off on another the drop page.
• Cannot Import Literal python/typing#707 suggests adding typing_extensions as a dependency.
• Alternatively, we hold off on the use of Literal.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (5c602cb) to head (89f78a7).
Report is 790 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #1051 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 34 34 Lines 13846 13865 +19 =======================================
+ Hits 13839 13858 +19 
Misses 7 7 

Files with missing lines	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/_storage/v3.py	100.00% <100.00%> (ø)
zarr/convenience.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/creation.py	100.00% <ø> (ø)
zarr/hierarchy.py	99.79% <100.00%> (+<0.01%)	⬆️
zarr/meta.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[pep8speaks]

Hello @d-v-b! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-19 17:27:41 UTC

[d-v-b]

mypy is almost happy. A few things that I would appreciate input on:

• The _ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None, which contradicts the docstrings for those methods -- as advertised, these methods should return valid store instances. Should they instead return some default store when the input is None? This is causing some mypy issues that I can get around with calls to cast, but it struck me as odd that the docstring mismatches the implementation here. cc @grlee77
• there are a few places where we call __init__ directly (namely setstate methods), and mypy hates that. Is there an alternative to calling __init__ directly, or should I just blind mypy to that code?

[joshmoore]

_ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None

My guess is that they should throw on None.

https://github.com/zarr-developers/zarr-python/blob/main/zarr/storage.py#L787

Extracting the entire contents of __init__ out to an __initialize(). In that case, I do wonder what happens to the lock though....

[d-v-b]

@grlee77 can you shed some light on whether the _ensure_store methods defined in _storage/store.py should handle None? Returning None is contrary to the docstrings, but there's a test for this exact behavior here. If I remove the None-handling logic from _ensure_store and the test for this behavior, no other tests fail...

[grlee77]

That is how _ensure_store was original implemented in #612, but the test case wasn't there at that time. Most likely, I added the test case later to complete test coverage, but probably should have just removed that case instead. Perhaps it was being used at some point in an earlier draft, but does not currently seem to be needed. I will make a PR to remove it.

[joshmoore]

I was thinking merging @grlee77's PR would clear up your mypy woes, @d-v-b, but there look to be a few more.

[jhamman]

I'm going to close this as stale. Folks should feel free to reopen if there is interest in continuing this work.

[d-v-b]

Currently when using a Group to get an existing Array via Group.require_dataset, there's no way to control the "write_empty_chunksness" of the array (since Array.write_empty_chunks is not part of the array metadata). This means that you cannot call group.require_dataset('foo', shape=10, dtype='i4', write_empty_chunks=False) and get an array that has the desired .write_empty_chunks property (instead the write_empty_chunks kwarg is ignored).

A few other array properties are similar (synchronizer, cache_metadata, and cache_attrs), and in main these keyword arguments are extracted from the **kwargs argument to Group.require_dataset before being passed to the Array constructor. This PR expands this behavior to include write_empty_chunks, thereby enabling the control of the write_empty_chunks property of the arrays returned by Group.require_dataset.

I also added a lot of type annotations.

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
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[d-v-b]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

[joshmoore]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

• Happy to hear opinions but depending on when you are targeting this for, I'd be inclined to hold off on another the drop page.
• Cannot Import Literal python/typing#707 suggests adding typing_extensions as a dependency.
• Alternatively, we hold off on the use of Literal.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (5c602cb) to head (89f78a7).
Report is 790 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #1051 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 34 34 Lines 13846 13865 +19 =======================================
+ Hits 13839 13858 +19 
Misses 7 7 

Files with missing lines	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/_storage/v3.py	100.00% <100.00%> (ø)
zarr/convenience.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/creation.py	100.00% <ø> (ø)
zarr/hierarchy.py	99.79% <100.00%> (+<0.01%)	⬆️
zarr/meta.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[pep8speaks]

Hello @d-v-b! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-19 17:27:41 UTC

[d-v-b]

mypy is almost happy. A few things that I would appreciate input on:

• The _ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None, which contradicts the docstrings for those methods -- as advertised, these methods should return valid store instances. Should they instead return some default store when the input is None? This is causing some mypy issues that I can get around with calls to cast, but it struck me as odd that the docstring mismatches the implementation here. cc @grlee77
• there are a few places where we call __init__ directly (namely setstate methods), and mypy hates that. Is there an alternative to calling __init__ directly, or should I just blind mypy to that code?

[joshmoore]

_ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None

My guess is that they should throw on None.

https://github.com/zarr-developers/zarr-python/blob/main/zarr/storage.py#L787

Extracting the entire contents of __init__ out to an __initialize(). In that case, I do wonder what happens to the lock though....

[d-v-b]

@grlee77 can you shed some light on whether the _ensure_store methods defined in _storage/store.py should handle None? Returning None is contrary to the docstrings, but there's a test for this exact behavior here. If I remove the None-handling logic from _ensure_store and the test for this behavior, no other tests fail...

[grlee77]

That is how _ensure_store was original implemented in #612, but the test case wasn't there at that time. Most likely, I added the test case later to complete test coverage, but probably should have just removed that case instead. Perhaps it was being used at some point in an earlier draft, but does not currently seem to be needed. I will make a PR to remove it.

[joshmoore]

I was thinking merging @grlee77's PR would clear up your mypy woes, @d-v-b, but there look to be a few more.

[jhamman]

I'm going to close this as stale. Folks should feel free to reopen if there is interest in continuing this work.

[d-v-b]

Currently when using a Group to get an existing Array via Group.require_dataset, there's no way to control the "write_empty_chunksness" of the array (since Array.write_empty_chunks is not part of the array metadata). This means that you cannot call group.require_dataset('foo', shape=10, dtype='i4', write_empty_chunks=False) and get an array that has the desired .write_empty_chunks property (instead the write_empty_chunks kwarg is ignored).

A few other array properties are similar (synchronizer, cache_metadata, and cache_attrs), and in main these keyword arguments are extracted from the **kwargs argument to Group.require_dataset before being passed to the Array constructor. This PR expands this behavior to include write_empty_chunks, thereby enabling the control of the write_empty_chunks property of the arrays returned by Group.require_dataset.

I also added a lot of type annotations.

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
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[d-v-b]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

[joshmoore]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

• Happy to hear opinions but depending on when you are targeting this for, I'd be inclined to hold off on another the drop page.
• Cannot Import Literal python/typing#707 suggests adding typing_extensions as a dependency.
• Alternatively, we hold off on the use of Literal.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (5c602cb) to head (89f78a7).
Report is 790 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #1051 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 34 34 Lines 13846 13865 +19 =======================================
+ Hits 13839 13858 +19 
Misses 7 7 

Files with missing lines	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/_storage/v3.py	100.00% <100.00%> (ø)
zarr/convenience.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/creation.py	100.00% <ø> (ø)
zarr/hierarchy.py	99.79% <100.00%> (+<0.01%)	⬆️
zarr/meta.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[pep8speaks]

Hello @d-v-b! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-19 17:27:41 UTC

[d-v-b]

mypy is almost happy. A few things that I would appreciate input on:

• The _ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None, which contradicts the docstrings for those methods -- as advertised, these methods should return valid store instances. Should they instead return some default store when the input is None? This is causing some mypy issues that I can get around with calls to cast, but it struck me as odd that the docstring mismatches the implementation here. cc @grlee77
• there are a few places where we call __init__ directly (namely setstate methods), and mypy hates that. Is there an alternative to calling __init__ directly, or should I just blind mypy to that code?

[joshmoore]

_ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None

My guess is that they should throw on None.

https://github.com/zarr-developers/zarr-python/blob/main/zarr/storage.py#L787

Extracting the entire contents of __init__ out to an __initialize(). In that case, I do wonder what happens to the lock though....

[d-v-b]

@grlee77 can you shed some light on whether the _ensure_store methods defined in _storage/store.py should handle None? Returning None is contrary to the docstrings, but there's a test for this exact behavior here. If I remove the None-handling logic from _ensure_store and the test for this behavior, no other tests fail...

[grlee77]

That is how _ensure_store was original implemented in #612, but the test case wasn't there at that time. Most likely, I added the test case later to complete test coverage, but probably should have just removed that case instead. Perhaps it was being used at some point in an earlier draft, but does not currently seem to be needed. I will make a PR to remove it.

[joshmoore]

I was thinking merging @grlee77's PR would clear up your mypy woes, @d-v-b, but there look to be a few more.

[jhamman]

I'm going to close this as stale. Folks should feel free to reopen if there is interest in continuing this work.

[d-v-b]

Currently when using a Group to get an existing Array via Group.require_dataset, there's no way to control the "write_empty_chunksness" of the array (since Array.write_empty_chunks is not part of the array metadata). This means that you cannot call group.require_dataset('foo', shape=10, dtype='i4', write_empty_chunks=False) and get an array that has the desired .write_empty_chunks property (instead the write_empty_chunks kwarg is ignored).

A few other array properties are similar (synchronizer, cache_metadata, and cache_attrs), and in main these keyword arguments are extracted from the **kwargs argument to Group.require_dataset before being passed to the Array constructor. This PR expands this behavior to include write_empty_chunks, thereby enabling the control of the write_empty_chunks property of the arrays returned by Group.require_dataset.

I also added a lot of type annotations.

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
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[d-v-b]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

[joshmoore]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

• Happy to hear opinions but depending on when you are targeting this for, I'd be inclined to hold off on another the drop page.
• Cannot Import Literal python/typing#707 suggests adding typing_extensions as a dependency.
• Alternatively, we hold off on the use of Literal.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (5c602cb) to head (89f78a7).
Report is 790 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #1051 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 34 34 Lines 13846 13865 +19 =======================================
+ Hits 13839 13858 +19 
Misses 7 7 

Files with missing lines	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/_storage/v3.py	100.00% <100.00%> (ø)
zarr/convenience.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/creation.py	100.00% <ø> (ø)
zarr/hierarchy.py	99.79% <100.00%> (+<0.01%)	⬆️
zarr/meta.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[pep8speaks]

Hello @d-v-b! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-19 17:27:41 UTC

[d-v-b]

mypy is almost happy. A few things that I would appreciate input on:

• The _ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None, which contradicts the docstrings for those methods -- as advertised, these methods should return valid store instances. Should they instead return some default store when the input is None? This is causing some mypy issues that I can get around with calls to cast, but it struck me as odd that the docstring mismatches the implementation here. cc @grlee77
• there are a few places where we call __init__ directly (namely setstate methods), and mypy hates that. Is there an alternative to calling __init__ directly, or should I just blind mypy to that code?

[joshmoore]

_ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None

My guess is that they should throw on None.

https://github.com/zarr-developers/zarr-python/blob/main/zarr/storage.py#L787

Extracting the entire contents of __init__ out to an __initialize(). In that case, I do wonder what happens to the lock though....

[d-v-b]

@grlee77 can you shed some light on whether the _ensure_store methods defined in _storage/store.py should handle None? Returning None is contrary to the docstrings, but there's a test for this exact behavior here. If I remove the None-handling logic from _ensure_store and the test for this behavior, no other tests fail...

[grlee77]

That is how _ensure_store was original implemented in #612, but the test case wasn't there at that time. Most likely, I added the test case later to complete test coverage, but probably should have just removed that case instead. Perhaps it was being used at some point in an earlier draft, but does not currently seem to be needed. I will make a PR to remove it.

[joshmoore]

I was thinking merging @grlee77's PR would clear up your mypy woes, @d-v-b, but there look to be a few more.

[jhamman]

I'm going to close this as stale. Folks should feel free to reopen if there is interest in continuing this work.

[d-v-b]

Currently when using a Group to get an existing Array via Group.require_dataset, there's no way to control the "write_empty_chunksness" of the array (since Array.write_empty_chunks is not part of the array metadata). This means that you cannot call group.require_dataset('foo', shape=10, dtype='i4', write_empty_chunks=False) and get an array that has the desired .write_empty_chunks property (instead the write_empty_chunks kwarg is ignored).

A few other array properties are similar (synchronizer, cache_metadata, and cache_attrs), and in main these keyword arguments are extracted from the **kwargs argument to Group.require_dataset before being passed to the Array constructor. This PR expands this behavior to include write_empty_chunks, thereby enabling the control of the write_empty_chunks property of the arrays returned by Group.require_dataset.

I also added a lot of type annotations.

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
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[d-v-b]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

[joshmoore]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

• Happy to hear opinions but depending on when you are targeting this for, I'd be inclined to hold off on another the drop page.
• Cannot Import Literal python/typing#707 suggests adding typing_extensions as a dependency.
• Alternatively, we hold off on the use of Literal.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (5c602cb) to head (89f78a7).
Report is 790 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #1051 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 34 34 Lines 13846 13865 +19 =======================================
+ Hits 13839 13858 +19 
Misses 7 7 

Files with missing lines	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/_storage/v3.py	100.00% <100.00%> (ø)
zarr/convenience.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/creation.py	100.00% <ø> (ø)
zarr/hierarchy.py	99.79% <100.00%> (+<0.01%)	⬆️
zarr/meta.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[pep8speaks]

Hello @d-v-b! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-19 17:27:41 UTC

[d-v-b]

mypy is almost happy. A few things that I would appreciate input on:

• The _ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None, which contradicts the docstrings for those methods -- as advertised, these methods should return valid store instances. Should they instead return some default store when the input is None? This is causing some mypy issues that I can get around with calls to cast, but it struck me as odd that the docstring mismatches the implementation here. cc @grlee77
• there are a few places where we call __init__ directly (namely setstate methods), and mypy hates that. Is there an alternative to calling __init__ directly, or should I just blind mypy to that code?

[joshmoore]

_ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None

My guess is that they should throw on None.

https://github.com/zarr-developers/zarr-python/blob/main/zarr/storage.py#L787

Extracting the entire contents of __init__ out to an __initialize(). In that case, I do wonder what happens to the lock though....

[d-v-b]

@grlee77 can you shed some light on whether the _ensure_store methods defined in _storage/store.py should handle None? Returning None is contrary to the docstrings, but there's a test for this exact behavior here. If I remove the None-handling logic from _ensure_store and the test for this behavior, no other tests fail...

[grlee77]

That is how _ensure_store was original implemented in #612, but the test case wasn't there at that time. Most likely, I added the test case later to complete test coverage, but probably should have just removed that case instead. Perhaps it was being used at some point in an earlier draft, but does not currently seem to be needed. I will make a PR to remove it.

[joshmoore]

I was thinking merging @grlee77's PR would clear up your mypy woes, @d-v-b, but there look to be a few more.

[jhamman]

I'm going to close this as stale. Folks should feel free to reopen if there is interest in continuing this work.

[d-v-b]

Currently when using a Group to get an existing Array via Group.require_dataset, there's no way to control the "write_empty_chunksness" of the array (since Array.write_empty_chunks is not part of the array metadata). This means that you cannot call group.require_dataset('foo', shape=10, dtype='i4', write_empty_chunks=False) and get an array that has the desired .write_empty_chunks property (instead the write_empty_chunks kwarg is ignored).

A few other array properties are similar (synchronizer, cache_metadata, and cache_attrs), and in main these keyword arguments are extracted from the **kwargs argument to Group.require_dataset before being passed to the Array constructor. This PR expands this behavior to include write_empty_chunks, thereby enabling the control of the write_empty_chunks property of the arrays returned by Group.require_dataset.

I also added a lot of type annotations.

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
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[d-v-b]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

[joshmoore]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

• Happy to hear opinions but depending on when you are targeting this for, I'd be inclined to hold off on another the drop page.
• Cannot Import Literal python/typing#707 suggests adding typing_extensions as a dependency.
• Alternatively, we hold off on the use of Literal.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (5c602cb) to head (89f78a7).
Report is 790 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #1051 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 34 34 Lines 13846 13865 +19 =======================================
+ Hits 13839 13858 +19 
Misses 7 7 

Files with missing lines	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/_storage/v3.py	100.00% <100.00%> (ø)
zarr/convenience.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/creation.py	100.00% <ø> (ø)
zarr/hierarchy.py	99.79% <100.00%> (+<0.01%)	⬆️
zarr/meta.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[pep8speaks]

Hello @d-v-b! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-19 17:27:41 UTC

[d-v-b]

mypy is almost happy. A few things that I would appreciate input on:

• The _ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None, which contradicts the docstrings for those methods -- as advertised, these methods should return valid store instances. Should they instead return some default store when the input is None? This is causing some mypy issues that I can get around with calls to cast, but it struck me as odd that the docstring mismatches the implementation here. cc @grlee77
• there are a few places where we call __init__ directly (namely setstate methods), and mypy hates that. Is there an alternative to calling __init__ directly, or should I just blind mypy to that code?

[joshmoore]

_ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None

My guess is that they should throw on None.

https://github.com/zarr-developers/zarr-python/blob/main/zarr/storage.py#L787

Extracting the entire contents of __init__ out to an __initialize(). In that case, I do wonder what happens to the lock though....

[d-v-b]

@grlee77 can you shed some light on whether the _ensure_store methods defined in _storage/store.py should handle None? Returning None is contrary to the docstrings, but there's a test for this exact behavior here. If I remove the None-handling logic from _ensure_store and the test for this behavior, no other tests fail...

[grlee77]

That is how _ensure_store was original implemented in #612, but the test case wasn't there at that time. Most likely, I added the test case later to complete test coverage, but probably should have just removed that case instead. Perhaps it was being used at some point in an earlier draft, but does not currently seem to be needed. I will make a PR to remove it.

[joshmoore]

I was thinking merging @grlee77's PR would clear up your mypy woes, @d-v-b, but there look to be a few more.

[jhamman]

I'm going to close this as stale. Folks should feel free to reopen if there is interest in continuing this work.

[d-v-b]

Currently when using a Group to get an existing Array via Group.require_dataset, there's no way to control the "write_empty_chunksness" of the array (since Array.write_empty_chunks is not part of the array metadata). This means that you cannot call group.require_dataset('foo', shape=10, dtype='i4', write_empty_chunks=False) and get an array that has the desired .write_empty_chunks property (instead the write_empty_chunks kwarg is ignored).

A few other array properties are similar (synchronizer, cache_metadata, and cache_attrs), and in main these keyword arguments are extracted from the **kwargs argument to Group.require_dataset before being passed to the Array constructor. This PR expands this behavior to include write_empty_chunks, thereby enabling the control of the write_empty_chunks property of the arrays returned by Group.require_dataset.

I also added a lot of type annotations.

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
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[d-v-b]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

[joshmoore]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

• Happy to hear opinions but depending on when you are targeting this for, I'd be inclined to hold off on another the drop page.
• Cannot Import Literal python/typing#707 suggests adding typing_extensions as a dependency.
• Alternatively, we hold off on the use of Literal.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (5c602cb) to head (89f78a7).
Report is 790 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #1051 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 34 34 Lines 13846 13865 +19 =======================================
+ Hits 13839 13858 +19 
Misses 7 7 

Files with missing lines	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/_storage/v3.py	100.00% <100.00%> (ø)
zarr/convenience.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/creation.py	100.00% <ø> (ø)
zarr/hierarchy.py	99.79% <100.00%> (+<0.01%)	⬆️
zarr/meta.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[pep8speaks]

Hello @d-v-b! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-19 17:27:41 UTC

[d-v-b]

mypy is almost happy. A few things that I would appreciate input on:

• The _ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None, which contradicts the docstrings for those methods -- as advertised, these methods should return valid store instances. Should they instead return some default store when the input is None? This is causing some mypy issues that I can get around with calls to cast, but it struck me as odd that the docstring mismatches the implementation here. cc @grlee77
• there are a few places where we call __init__ directly (namely setstate methods), and mypy hates that. Is there an alternative to calling __init__ directly, or should I just blind mypy to that code?

[joshmoore]

_ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None

My guess is that they should throw on None.

https://github.com/zarr-developers/zarr-python/blob/main/zarr/storage.py#L787

Extracting the entire contents of __init__ out to an __initialize(). In that case, I do wonder what happens to the lock though....

[d-v-b]

@grlee77 can you shed some light on whether the _ensure_store methods defined in _storage/store.py should handle None? Returning None is contrary to the docstrings, but there's a test for this exact behavior here. If I remove the None-handling logic from _ensure_store and the test for this behavior, no other tests fail...

[grlee77]

That is how _ensure_store was original implemented in #612, but the test case wasn't there at that time. Most likely, I added the test case later to complete test coverage, but probably should have just removed that case instead. Perhaps it was being used at some point in an earlier draft, but does not currently seem to be needed. I will make a PR to remove it.

[joshmoore]

I was thinking merging @grlee77's PR would clear up your mypy woes, @d-v-b, but there look to be a few more.

[jhamman]

I'm going to close this as stale. Folks should feel free to reopen if there is interest in continuing this work.

[d-v-b]

Currently when using a Group to get an existing Array via Group.require_dataset, there's no way to control the "write_empty_chunksness" of the array (since Array.write_empty_chunks is not part of the array metadata). This means that you cannot call group.require_dataset('foo', shape=10, dtype='i4', write_empty_chunks=False) and get an array that has the desired .write_empty_chunks property (instead the write_empty_chunks kwarg is ignored).

A few other array properties are similar (synchronizer, cache_metadata, and cache_attrs), and in main these keyword arguments are extracted from the **kwargs argument to Group.require_dataset before being passed to the Array constructor. This PR expands this behavior to include write_empty_chunks, thereby enabling the control of the write_empty_chunks property of the arrays returned by Group.require_dataset.

I also added a lot of type annotations.

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
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[d-v-b]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

[joshmoore]

looks like CI is failing for python 3.7 due to some type annotation stuff... do we need to support 3.7 still?

• Happy to hear opinions but depending on when you are targeting this for, I'd be inclined to hold off on another the drop page.
• Cannot Import Literal python/typing#707 suggests adding typing_extensions as a dependency.
• Alternatively, we hold off on the use of Literal.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (5c602cb) to head (89f78a7).
Report is 790 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #1051 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 34 34 Lines 13846 13865 +19 =======================================
+ Hits 13839 13858 +19 
Misses 7 7 

Files with missing lines	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/_storage/v3.py	100.00% <100.00%> (ø)
zarr/convenience.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/creation.py	100.00% <ø> (ø)
zarr/hierarchy.py	99.79% <100.00%> (+<0.01%)	⬆️
zarr/meta.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[pep8speaks]

Hello @d-v-b! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-19 17:27:41 UTC

[d-v-b]

mypy is almost happy. A few things that I would appreciate input on:

• The _ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None, which contradicts the docstrings for those methods -- as advertised, these methods should return valid store instances. Should they instead return some default store when the input is None? This is causing some mypy issues that I can get around with calls to cast, but it struck me as odd that the docstring mismatches the implementation here. cc @grlee77
• there are a few places where we call __init__ directly (namely setstate methods), and mypy hates that. Is there an alternative to calling __init__ directly, or should I just blind mypy to that code?

[joshmoore]

_ensure_store methods of BaseStore and StoreV3 in _storage/store.py can return None

My guess is that they should throw on None.

https://github.com/zarr-developers/zarr-python/blob/main/zarr/storage.py#L787

Extracting the entire contents of __init__ out to an __initialize(). In that case, I do wonder what happens to the lock though....

[d-v-b]

@grlee77 can you shed some light on whether the _ensure_store methods defined in _storage/store.py should handle None? Returning None is contrary to the docstrings, but there's a test for this exact behavior here. If I remove the None-handling logic from _ensure_store and the test for this behavior, no other tests fail...

[grlee77]

That is how _ensure_store was original implemented in #612, but the test case wasn't there at that time. Most likely, I added the test case later to complete test coverage, but probably should have just removed that case instead. Perhaps it was being used at some point in an earlier draft, but does not currently seem to be needed. I will make a PR to remove it.

[joshmoore]

I was thinking merging @grlee77's PR would clear up your mypy woes, @d-v-b, but there look to be a few more.

[jhamman]

I'm going to close this as stale. Folks should feel free to reopen if there is interest in continuing this work.