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

Add .nwb to known Zarr suffixes; add tests for NWB Zarr backend #1312

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

CodyCBakerPhD
Copy link
Contributor

@CodyCBakerPhD CodyCBakerPhD commented Jul 18, 2023

fix #1310

This seems to have worked fine for me on a local upload attempt to staging (validation had to be suppressed until we get NeurodataWithoutBorders/nwbinspector#388 and the integration follow-up through)

At least getting the seeds of this PR started

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (72816ab) 88.51% compared to head (a7d16aa) 67.93%.

Files Patch % Lines
dandi/tests/fixtures.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1312       +/-   ##
===========================================
- Coverage   88.51%   67.93%   -20.58%     
===========================================
  Files          77       77               
  Lines       10492    10480       -12     
===========================================
- Hits         9287     7120     -2167     
- Misses       1205     3360     +2155     
Flag Coverage Δ
unittests 67.93% <80.00%> (-20.58%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@CodyCBakerPhD
Copy link
Contributor Author

The current CI error

TypeError: Can't instantiate abstract class NWBZarrIO with abstract method can_read

has a known cause and will likely need to await the upcoming release of hdmf_zarr (this week sometime)

dandi/consts.py Outdated Show resolved Hide resolved
@CodyCBakerPhD
Copy link
Contributor Author

CodyCBakerPhD commented Aug 11, 2023

@yarikoptic Trying the main branch (not this one) with .nwb.zarr gave the following error during dandi upload -i dandi-staging to destination https://gui-staging.dandiarchive.org/dandiset/204919/draft/files?location=test_read_nwbfile

501 Server Error: Not Implemented for url: https://dandi-api-staging-dandisets.s3.amazonaws.com/zarr/d...

Worth mentioning it began upload and proceeded find until 99% done when the error happened

log: 20230811152021Z-372.log

@yarikoptic
Copy link
Member

eh -- I spent some time trying to see smth in the logs

  • your activity came at unfortunate time when I had to relogin the client so I do not have logs for
-rw------- 1 dandi dandi-internal 112444 Aug 11 11:00 20230811-1001.log
-rw------- 1 dandi dandi-internal      0 Aug 11 11:01 20230811-1101.log
-rw------- 1 dandi dandi-internal      0 Aug 11 12:01 20230811-1201.log
-rw------- 1 dandi dandi-internal      0 Aug 11 12:20 20230811-1220.log
-rw------- 1 dandi dandi-internal  45857 Aug 11 12:37 20230811-1224.log

today

  • in papertrail -- nothing informative if searched by dd868c2b-0a3b-44cb-afdb-eb62f07a701b , nothing for "implemented" or "exception" (only older ones). @dandi/archive-admin might have better skills/tools to see if anything in the logs.

or if you retry again -- I might spot smth in the fresh dumps for heroku which I should be getting now.

@yarikoptic
Copy link
Member

The current CI error

TypeError: Can't instantiate abstract class NWBZarrIO with abstract method can_read

has a known cause and will likely need to await the upcoming release of hdmf_zarr (this week sometime)

FWIW, in dev-deps CI run we see the same Error and we do

        pip install git+https://github.com/jaraco/keyring
        pip install git+https://github.com/NeurodataWithoutBorders/nwbinspector
        pip install git+https://github.com/NeurodataWithoutBorders/pynwb

so we do not install development hdmf as well. Do you think we should?

yarikoptic added a commit that referenced this pull request Aug 11, 2023
Inspired by #1312 and ongoing work for
Zarr based backend for NWB.
@CodyCBakerPhD
Copy link
Contributor Author

CodyCBakerPhD commented Aug 11, 2023

I just tried again (from the save environment) and did not get the previous error

Asset is now visible: https://gui-staging.dandiarchive.org/dandiset/204919/draft/files?location=test_read_nwbfile

@CodyCBakerPhD
Copy link
Contributor Author

BTW the 'environment' I'm using right now is just the default/base on the DANDI Hub

@yarikoptic
Copy link
Member

so we do not install development hdmf as well. Do you think we should?

trying in #1320 and it leads to an error (posted there)

I just tried again (from the save environment) and did not get the previous error

well, would need 1 more fresh .nwb.zarr I guess ;-)

yarikoptic added a commit that referenced this pull request Nov 1, 2023
Inspired by #1312 and ongoing work for
Zarr based backend for NWB.
dandi/consts.py Outdated Show resolved Hide resolved
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.

[Feature] Support upload of Zarr-backend NWB files
3 participants