Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Fix some typing #9988

Merged
merged 9 commits into from
Jan 27, 2025
Merged

Fix some typing #9988

merged 9 commits into from
Jan 27, 2025

Conversation

dcherian
Copy link
Contributor

No description provided.

@dcherian
Copy link
Contributor Author

Looks like we need to update the type on store in to_zarr:

xarray/core/datatree_io.py:133: error: Argument 1 to "consolidate_metadata" has incompatible type "MutableMapping[Any, Any] | str | PathLike[str]"; expected "Store | StorePath | Path | str | dict[str, Buffer]"  [arg-type]
xarray/tests/test_backends_datatree.py: note: In member "test_to_zarr_zip_store" of class "TestZarrDatatreeIO":
xarray/tests/test_backends_datatree.py:417: error: Argument 1 to "open_datatree" has incompatible type "ZipStore"; expected "str | PathLike[Any] | ReadBuffer[Any] | AbstractDataStore"  [arg-type]

Are all of these expected to be used publicly? (Store | StorePath | Path | str | dict[str, Buffer])

cc @jhamman @d-v-b

headtr1ck
headtr1ck approved these changes Jan 27, 2025
View reviewed changes
Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

xarray/backends/zarr.py Outdated
@@ -1066,6 +1066,8 @@ def _open_existing_array(self, *, name) -> ZarrArray:
else:
zarr_array = self.zarr_group[name]

if TYPE_CHECKING:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the error here was, but simpler use cast(ZarrArray, zarr_array).

@d-v-b
Copy link
Contributor

d-v-b commented Jan 27, 2025

Looks like we need to update the type on store in to_zarr:

xarray/core/datatree_io.py:133: error: Argument 1 to "consolidate_metadata" has incompatible type "MutableMapping[Any, Any] | str | PathLike[str]"; expected "Store | StorePath | Path | str | dict[str, Buffer]"  [arg-type]
xarray/tests/test_backends_datatree.py: note: In member "test_to_zarr_zip_store" of class "TestZarrDatatreeIO":
xarray/tests/test_backends_datatree.py:417: error: Argument 1 to "open_datatree" has incompatible type "ZipStore"; expected "str | PathLike[Any] | ReadBuffer[Any] | AbstractDataStore"  [arg-type]

Are all of these expected to be used publicly? (Store | StorePath | Path | str | dict[str, Buffer])

cc @jhamman @d-v-b

Yes, these are supposed to be all the things we can coerce to a store + path. That being said, I think we can simplify this if it's causing problems. For example, I think we could widen dict to MutableMapping, which would help with one of your errors.

@dcherian
Copy link
Contributor Author

zarr.storage.StoreLike looks like what we need.

@dcherian dcherian enabled auto-merge (squash) January 27, 2025 16:15
@dcherian dcherian merged commit d62b276 into pydata:main Jan 27, 2025
28 checks passed
@max-sixty
Copy link
Collaborator

Thanks a lot @dcherian

@dcherian dcherian deleted the fix-typing branch January 27, 2025 19:09
dcherian added a commit that referenced this pull request Jan 30, 2025
@dcherian
Merge branch 'main' into backend-indexing
dffc05c 
* main: (79 commits)
  fix mean for datetime-like using the respective time resolution unit (#9977)
  Add `time_unit` argument to `CFTimeIndex.to_datetimeindex` (#9965)
  remove gate and add a test (#9958)
  Remove repetitive that (replace it with the) (#9994)
  add shxarray to the xarray ecosystem list (#9995)
  Add `shards` to `valid_encodings` to enable sharded Zarr writing (#9948)
  Use flox for grouped first, last (#9986)
  Bump the actions group with 2 updates (#9989)
  Fix some typing (#9988)
  Remove unnecessary a article (#9980)
  Fix test_doc_example on big-endian systems (#9949)
  fix weighted polyfit for arrays with more than 2 dimensions (#9974)
  Use zarr-fixture to prevent thread leakage errors (#9967)
  remove dask-expr from CI runs, fix related tests (#9971)
  Update time coding tests to assert exact equality (#9961)
  cast type to PDDatetimeUnitOptions (#9963)
  Suggest the correct name when no key matches in the dataset (#9943)
  fix upstream dev issues (#9953)
  Relax nanosecond datetime restriction in CF time decoding (#9618)
  Remove outdated quantile test. (#9945)
  ...
dcherian added a commit that referenced this pull request Jan 30, 2025
@dcherian
Merge branch 'main' into backend-indexing
1ffe5e9 
* main: (79 commits)
  fix mean for datetime-like using the respective time resolution unit (#9977)
  Add `time_unit` argument to `CFTimeIndex.to_datetimeindex` (#9965)
  remove gate and add a test (#9958)
  Remove repetitive that (replace it with the) (#9994)
  add shxarray to the xarray ecosystem list (#9995)
  Add `shards` to `valid_encodings` to enable sharded Zarr writing (#9948)
  Use flox for grouped first, last (#9986)
  Bump the actions group with 2 updates (#9989)
  Fix some typing (#9988)
  Remove unnecessary a article (#9980)
  Fix test_doc_example on big-endian systems (#9949)
  fix weighted polyfit for arrays with more than 2 dimensions (#9974)
  Use zarr-fixture to prevent thread leakage errors (#9967)
  remove dask-expr from CI runs, fix related tests (#9971)
  Update time coding tests to assert exact equality (#9961)
  cast type to PDDatetimeUnitOptions (#9963)
  Suggest the correct name when no key matches in the dataset (#9943)
  fix upstream dev issues (#9953)
  Relax nanosecond datetime restriction in CF time decoding (#9618)
  Remove outdated quantile test. (#9945)
  ...
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@headtr1ck headtr1ck headtr1ck approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dcherian @d-v-b @max-sixty @headtr1ck