Fix bug in ZarrIO.__open_consolidated

oruebel commented 3 months ago

Motivation

ZarrIO.__open_consolidated`` currently used the following checkif 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

[x] Did you update CHANGELOG.md with your changes?
[X] Have you checked our Contributing document?
[X] Have you ensured the PR clearly describes the problem and the solution?
[X] Is your contribution compliant with our coding style? This can be checked running ruff from the source directory.
[X] Have you checked to ensure that there aren't other open Pull Requests for the same change?
[X] 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.

codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

oruebel commented 3 months ago

@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 https://github.com/hdmf-dev/hdmf-zarr/issues/185 to address this gap in our tests separately .

hdmf-dev / hdmf-zarr