Skip to content
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

Compatibility with Zarr v3b2 #9795

Merged
merged 9 commits into from
Nov 21, 2024
Merged

Compatibility with Zarr v3b2 #9795

merged 9 commits into from
Nov 21, 2024

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian commented Nov 18, 2024

Closes #9277

@@ -1126,13 +1128,11 @@ def _create_cf_dataset():

def test_grid_mapping_and_bounds_are_not_coordinates_in_file(self) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of these CF convention handling test really only need to be run with one backend.

cc @kmuehlbauer in case that interests you. Similarly with the "coordinates" roundtripping

@dcherian dcherian added the run-upstream Run upstream CI label Nov 18, 2024
@TomNicholas TomNicholas added the topic-zarr Related to zarr storage library label Nov 18, 2024
@dcherian
Copy link
Contributor Author

dcherian commented Nov 18, 2024

I did some refactoring to fix a test. As an upside, I think we now more efficient when overwriting existing variables.

@dcherian dcherian mentioned this pull request Nov 18, 2024
6 tasks
@@ -867,16 +868,27 @@ def store(
"""
import zarr

existing_keys = tuple(self.zarr_group.array_keys())
if self._mode == "w":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

logic change. We now skip all these checks for mode="w". i think earlier, zarr would erase the group so all the logic below was not activated. Now that zarr does not erase the group, we must skip all the logic below.

@@ -964,6 +971,70 @@ def store(
def sync(self):
pass

def _open_existing_array(self, *, name) -> ZarrArray:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just moved it around so set_variables is easier to follow


return zarr_array

def _create_new_array(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just moved it around so set_variables is easier to follow

@@ -3397,7 +3395,7 @@ def test_region_write(self) -> None:
"set": 5,
"get": 10,
"list_dir": 3,
"list_prefix": 0,
"list_prefix": 4,
Copy link
Contributor Author

@dcherian dcherian Nov 18, 2024

Choose a reason for hiding this comment

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

wild. zarr is calling list_prefix(prefix='') twice on group open; and then listing each variable:

listing ''
listing ''
listing 'x/'
listing 'foo/'

cc @d-v-b

Copy link

Choose a reason for hiding this comment

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

weird! that's definitely not needed

@dcherian
Copy link
Contributor Author

Remaining upstream failures are zarr-developers/zarr-python#2501 and that we seem to now have 4 new list_prefix calls (I've bumped the count, we can optimize later). :/

@dcherian
Copy link
Contributor Author

Merging tomorrow if there are no comments. There are no functional changes here just adapting to zarr's mode="w" no longer clearing the group

@dcherian dcherian added the plan to merge Final call for comments label Nov 21, 2024
@dcherian dcherian merged commit e510a9e into pydata:main Nov 21, 2024
31 of 35 checks passed
@dcherian dcherian deleted the zarr-v3-b2 branch November 21, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments run-upstream Run upstream CI topic-zarr Related to zarr storage library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

⚠️ Nightly upstream-dev CI failed ⚠️
3 participants