10000 forward write_empty_chunks kwarg in group.require_dataset by d-v-b · Pull Request #1051 · zarr-developers/zarr-python · GitHub
[go: up one dir, main page]
Skip to content

forward write_empty_chunks kwarg in group.require_dataset #1051

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 18 commits into from
Closed

forward write_empty_chunks kwarg in group.require_dataset #1051

wants to merge 18 commits into from

Conversation

d-v-b
Copy link
Contributor
@d-v-b d-v-b commented Jun 22, 2022

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
Copy link
Contributor Author
d-v-b commented Jun 22, 2022

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

@joshmoore
Copy link
Member

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
Copy link
codecov bot commented Jun 23, 2022 •
edited
Loading

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
Copy link
pep8speaks commented Jun 27, 2022 •
edited
Loading

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
Copy link
Contributor Author
d-v-b commented Jun 27, 2022

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
Copy link
Member

_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
Copy link
Contributor Author
d-v-b commented Jul 5, 2022

@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
Copy link
Contributor
grlee77 commented Jul 6, 2022 •
edited
Loading

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.

@grlee77 grlee77 mentioned this pull request Jul 6, 2022
Merged
3 tasks
@d-v-b d-v-b mentioned this pull request Jul 8, 2022
Closed
@joshmoore
Copy link
Member

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
Copy link
Member
jhamman commented Oct 11, 2024

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

@jhamman jhamman closed this Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@d-v-b @joshmoore @pep8speaks @grlee77 @jhamman
0