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

Fix bug in ZarrIO.__open_consolidated #184

Merged
merged 3 commits into from
Apr 26, 2024
Merged

Conversation

oruebel
Copy link
Contributor

@oruebel oruebel commented Apr 26, 2024

Motivation

ZarrIO.__open_consolidated`` currently used the following check if os.path.isfile(path+'/.zmetadata'):` to determine whether the file should be open with consolidated metadata or without. This check, however, assumes that the file is local and will not work for remote files stored on S3.

Changes

To address this issue this PR changes to a try/except logic instead. zarr.open_consolidated will raise a KeyError if the .zmetata is missing so we can just try to open the file this way first and the fall back to the regular method if it fails.

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running ruff from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

@oruebel oruebel requested a review from mavaylon1 April 26, 2024 01:44
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.05%. Comparing base (afaab5f) to head (c00e4bc).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #184      +/-   ##
==========================================
- Coverage   86.08%   86.05%   -0.03%     
==========================================
  Files           5        5              
  Lines        1164     1162       -2     
  Branches      289      287       -2     
==========================================
- Hits         1002     1000       -2     
  Misses        107      107              
  Partials       55       55              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oruebel
Copy link
Contributor Author

oruebel commented Apr 26, 2024

@mavaylon1 could you please review this PR as well for the next release.

Note, I unfortunately could not add a unit test for this issue, but I filed #185 to address this gap in our tests separately .

@oruebel oruebel merged commit f331a2e into dev Apr 26, 2024
24 checks passed
@oruebel oruebel deleted the fix/consolidated_open_bug branch April 26, 2024 16:10
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

Successfully merging this pull request may close these issues.

3 participants