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

Debug the use of relative links #46

Merged
merged 34 commits into from
Dec 21, 2022
Merged

Debug the use of relative links #46

merged 34 commits into from
Dec 21, 2022

Conversation

oruebel
Copy link
Contributor

@oruebel oruebel commented Dec 14, 2022

Fix #44

  • Add tutorial to show how to create and NWB file using Zarr (the file includes links so we can use it for debugging the issue with links)
  • Add use of extlinks in Sphinx
  • Fix bug in requirements defined in setup.py
  • Add tests to check reading of links where we change the current directory (using relative path to create the file and then reading from a different current directory)

@oruebel
Copy link
Contributor Author

oruebel commented Dec 14, 2022

Currently, references/links use the path provided by the user to ZarrIO as the source path for the current file. Because of this, links can break even without moving the data to another system. If we create the file with a relative path (e.g., just the filename "ecephys_tutorial.nwb.zarr" in the tutorial), then the filepath (and hence all links) are in effect relative to the current working directory. To break links, we then only need to change the current working directory with os.chdir. Here an example of how a link currently looks in the Zarr file create by the new tutorial:

Screen Shot 2022-12-14 at 2 45 30 AM

Instead of listing source: ecephys_tutorial.nwb.zarr the source should be relative to the Zarr file itself, i.e., if the link is to an object within the same file the source should be either empty string or ".".

@oruebel
Copy link
Contributor Author

oruebel commented Dec 14, 2022

@mavaylon1 can you check on the failing test pipelines. There was an error in setup.py that I fixed, but there are still several errors in the pipelines that seem to be unrelated to this PR, but seem to be due to the setup of the pipelines.

ERROR: Cannot install hdmf-zarr 0.post0.dev1 (from /home/runner/work/hdmf-zarr/hdmf-zarr) and hdmf-zarr 0.post0.dev1 (from /home/runner/work/hdmf-zarr/hdmf-zarr/.tox/.tmp/package/1/hdmf_zarr-0.post0.dev1-0.editable-py3-none-any.whl) because these package versions have conflicting dependencies.

I think this may be same error with tox4 that @rly fixed for PyNWB in this PR NeurodataWithoutBorders/pynwb#1608

For (at least some of) the failing 3.7 tests it looks like the gallery tests are failing because the gallery files are not found. Maybe there is an issue with setting the working directory in the pipeline?

@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2022

Codecov Report

Base: 82.11% // Head: 82.15% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (6f67d68) compared to base (439df57).
Patch coverage: 92.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev      #46      +/-   ##
==========================================
+ Coverage   82.11%   82.15%   +0.03%     
==========================================
  Files           9        9              
  Lines        2360     2365       +5     
==========================================
+ Hits         1938     1943       +5     
  Misses        422      422              
Impacted Files Coverage Δ
src/hdmf_zarr/backend.py 87.66% <90.47%> (+0.09%) ⬆️
tests/unit/test_io_zarr.py 99.14% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mavaylon1 mavaylon1 closed this Dec 14, 2022
@mavaylon1 mavaylon1 deleted the debug/use_relative_links branch December 14, 2022 16:14
@mavaylon1 mavaylon1 restored the debug/use_relative_links branch December 14, 2022 16:21
@mavaylon1 mavaylon1 reopened this Dec 14, 2022
@mavaylon1
Copy link
Contributor

@oruebel I merged the pipeline updates but for some reason the paths are throwing errors still with the windows path using "\" instead of "/". I'll continue to dig around and hopefully can get this resolved by the end of tomorrow for a release this week.

@oruebel
Copy link
Contributor Author

oruebel commented Dec 21, 2022

@mavaylon1 thanks for taking a look at this and for keeping me posted

@mavaylon1
Copy link
Contributor

mavaylon1 commented Dec 21, 2022

@rly I've fixed the user warnings issue in the gallery tests and updated to use test_gallery.py. However, now I am faced with the original issue again of certain docs not being able to be found.

Screen Shot 2022-12-21 at 11 01 36 AM

I've printed out the files in the list test_gallery.py iterates over and the files are there. What is weird is that the two files consist of one old doc and one new doc.

@mavaylon1
Copy link
Contributor

What is weird is that it is the addition of plot_nwb_zarrio.py that throws the path issues. When you remove it, plot_zarr_datasetio.py can be found and plot_convert_nwb.py can also be found and returns an error regarding hdmf build (tbd later)

@mavaylon1
Copy link
Contributor

mavaylon1 commented Dec 21, 2022

If we use test.py --example in tox.ini instead of test_gallery.py then the gallery-3.10 tests pass which are required but not the 3.7. We could merge using the test.py --example and create another PR to update and figure out the issue of the gallery tests that are not required. That way the fix isn't held up.

I'll also note running python test.py --example passes in my python 3.7 environment but not here.

@oruebel @rly what are your thoughts?

@oruebel
Copy link
Contributor Author

oruebel commented Dec 21, 2022

Since we know this is only an issue of the test pipeline. I think we could merge this PR and do the release and fix the pipeline in a separate PR.

@mavaylon1
Copy link
Contributor

I'll review the PR today and we can merge before the day is over. I'll set up the separate PR for the gallery.

@mavaylon1 mavaylon1 self-requested a review December 21, 2022 22:06
Copy link
Contributor

@mavaylon1 mavaylon1 left a comment

Choose a reason for hiding this comment

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

Removed test_gallery,py to keep it reserved for the upcoming PR.

@oruebel oruebel merged commit 5d2811d into dev Dec 21, 2022
@oruebel oruebel deleted the debug/use_relative_links branch December 21, 2022 22:32
@alejoe91
Copy link
Collaborator

Thanks guys! It works perfectly on our side!

@oruebel
Copy link
Contributor Author

oruebel commented Dec 22, 2022

Thanks guys! It works perfectly on our side!

That is great to hear!

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.

Zarr links should be relative to root
4 participants