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.

Sign up for GitHub

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

Jump to bottom

Upgrade validation methods #1911

Open
wants to merge 35 commits into
base: dev
Choose a base branch
from
Open

Upgrade validation methods #1911

wants to merge 35 commits into from

Conversation

stephprince
Copy link
Contributor

@stephprince stephprince commented May 29, 2024 •
edited
Loading

Motivation

Addresses several issues summarized in #1808. This is a breaking change for the next major release.

This PR also modifies the validate method so that it

  1. accepts a single path as input (the CLI still accepts multiple paths)
  2. no longer returns a status code but will return errors if the function fails in the process of performing validation. (The CLI still returns an exit code).

TODO

  • test with nwbinspector (as of now should only require changes to this line)
  • check test coverage
  • finish testing with ZarrIO
  • when publishing the next release on conda-forge, the recipe file should also be updated to add the pynwb-validate entry point

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 flake8 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.

stephprince
stephprince commented May 29, 2024
View reviewed changes
src/pynwb/validate.py Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented May 29, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.77419% with 4 lines in your changes missing coverage. Please review.

Project coverage is 91.88%. Comparing base (a63387d) to head (11cc232).

Files with missing lines Patch % Lines
src/pynwb/__init__.py 78.57% 2 Missing and 1 partial ⚠️
src/pynwb/validation.py 98.55% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##              dev    #1911   +/-   ##
=======================================
  Coverage   91.88%   91.88%           
=======================================
  Files          27       28    +1     
  Lines        2711     2737   +26     
  Branches      707      710    +3     
=======================================
+ Hits         2491     2515   +24     
- Misses        144      145    +1     
- Partials       76       77    +1
Flag Coverage Δ
integration 73.10% <96.77%> (+0.25%) ⬆️
unit 82.42% <44.35%> (-0.43%) ⬇️

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.

@rly rly added this to the 3.0 milestone Oct 10, 2024
@stephprince stephprince changed the title Upgrade validation methods - make pynwb.validate behave similarly for io and paths Upgrade validation methods Nov 23, 2024
stephprince
stephprince commented Nov 23, 2024
View reviewed changes
src/pynwb/__init__.py Outdated
Comment on lines 359 to 372
def _get_backend(path: str):
from hdmf_zarr import NWBZarrIO

backend_io_classes = [NWBHDF5IO, NWBZarrIO]
backend_options = [b for b in backend_io_classes if b.can_read(path=path)]
if len(backend_options) == 0:
warn(f"Could not find an IO to read the file '{path}'."
f"This may be due to an older file version or invalid file."
f"Defaulting to NWBHDF5IO.", UserWarning)
return NWBHDF5IO
elif len(backend_options) > 1:
raise ValueError(f"Multiple backends found for file '{path}': {backend_options}")
else:
return backend_options[0]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have this temporary private method to start testing support for validation of Zarr NWB files as well. This can eventually be replaced by methods from the read_nwb PR

@stephprince stephprince marked this pull request as ready for review December 17, 2024 00:20
@stephprince stephprince requested a review from rly December 17, 2024 00:20
@stephprince
Copy link
Contributor Author

@rly this should be ready for review

There are a couple remaining TODO items related to validation support for NWBZarr files that could potentially be bumped to a separate PR:

  1. I started to try to implement support for NWBZarr file validation by allowing the ZarrIO.load_namespaces method to accept a ZarrIO.file object as input, but ran into this issue - [Bug]: .specloc not saved in consolidated metadata hdmf-dev/hdmf-zarr#243.
  2. We would also want to either rename the ZarrIO.file or HDF5IO._file property for consistent naming across backends:

    pynwb/src/pynwb/validation.py

    Lines 58 to 61 in 11cc232

    # TODO update HDF5IO to have .file property to make consistent with ZarrIO
    # then update input arguments here
    namespace_dependencies = io.load_namespaces(namespace_catalog=catalog,
    file=io._file)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rly rly Awaiting requested review from rly

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0
2 participants
@stephprince @rly