-
Notifications
You must be signed in to change notification settings - Fork 6
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 linking for FSSpec #138
Conversation
Should I add a test? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #138 +/- ##
==========================================
+ Coverage 86.10% 86.24% +0.13%
==========================================
Files 13 14 +1
Lines 3189 3221 +32
==========================================
+ Hits 2746 2778 +32
Misses 443 443 ☔ View full report in Codecov by Sentry. |
Added one using the Allen Institute dataset. Not sure if and where you guys want to add it to the CI, since it's currently quite slow. Maybe it would be better to keep this as a place holder and run the test once we have a small test file stored somewhere? |
Thanks for taking a stab at this! We can work on getting a small file for testing on DANDI hopefully while we are at SfN. The PR looks overall good to me. |
@alejoe91 When running the test locally, I get the following error:
Is there a way to avoid the need for credentials in the test and/or would using a file on DANDI fix this? I made a few minor changes to fix flake8 warnings (to avoid unused import warnings) and adding fsspec and s3fs as optional requirements. With regard to the time it takes for the test, I think this is Ok for now. I.e., I would be in favor of merging the PR and creating a separate issue to for creating a smaller test file to speed up the test. I.e., aside from the credential issues with the test, this PR looks good to me. |
The credential error also occured in the GitHub action here: https://github.com/hdmf-dev/hdmf-zarr/actions/runs/6838973516/job/18596637194?pr=138#step:7:119 |
I see! I think I haven't propagated the |
Just for reference, to help create a test with DANDI: Example code how to get a S3 URL for Zarr read from DANDI
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alejoe91 !
Motivation
The
ZarrIO.resolve_ref()
function assumes paths are local. This PR adds a methodis_remote()
to check if thezarr.store
isFSStore
and handles remote paths and links correctly.Fixes #134
How to test the behavior?
Checklist
ruff
from the source directory.