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 driver argument to new validation functions #1588

Merged
merged 12 commits into from
Jan 18, 2023

Conversation

CodyCBakerPhD
Copy link
Collaborator

When working on NeurodataWithoutBorders/nwbinspector#265 (the integration of the recent validation changes into the NWB Inspector) I ran into an issue getting the cached namespaces for files when using the streaming mode of the Inspector.

Traced it back to these particular function calls - thankfully, NWBHDF5IO.load_namespaces(...) already supports a driver keyword argument, we just need to add control and propagate it as shown here.

@rly What would your recommendations be for adding tests for this?

@CodyCBakerPhD CodyCBakerPhD self-assigned this Nov 9, 2022
@CodyCBakerPhD
Copy link
Collaborator Author

I'll also add the file argument propagation in a separate PR for fsspec based streaming. That will require just a little bit more since it would need to not conflict with the io vs. path usage and just be an alternate usage of the path

@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #1588 (08f33c5) into dev (2930095) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##              dev    #1588   +/-   ##
=======================================
  Coverage   91.31%   91.31%           
=======================================
  Files          25       25           
  Lines        2534     2534           
  Branches      481      481           
=======================================
  Hits         2314     2314           
  Misses        139      139           
  Partials       81       81           
Flag Coverage Δ
integration 70.44% <100.00%> (ø)
unit 84.37% <50.00%> (ø)

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

Impacted Files Coverage Δ
src/pynwb/validate.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@CodyCBakerPhD CodyCBakerPhD requested a review from rly November 9, 2022 20:09
@CodyCBakerPhD CodyCBakerPhD added category: enhancement improvements of code or code behavior topic: validator issues related to validation of files labels Nov 9, 2022
@rly
Copy link
Contributor

rly commented Nov 9, 2022

This looks good. You can add to https://github.com/NeurodataWithoutBorders/pynwb/tree/dev/tests/integration/ros3 which is the only other place in the tests where we test different drivers.

src/pynwb/validate.py Outdated Show resolved Hide resolved
@bendichter
Copy link
Contributor

This looks like a fix specifically for ros3. It's good to support, but let's try to shift our default over to fsspec for all streaming stuff

src/pynwb/validate.py Outdated Show resolved Hide resolved
@CodyCBakerPhD
Copy link
Collaborator Author

This looks like a fix specifically for ros3. It's good to support, but let's try to shift our default over to fsspec for all streaming stuff

#1588 (comment)

I'll also add the file argument propagation in a separate PR for fsspec based streaming. That will require just a little bit more since it would need to not conflict with the io vs. path usage and just be an alternate usage of the path

@CodyCBakerPhD
Copy link
Collaborator Author

This looks good. You can add to https://github.com/NeurodataWithoutBorders/pynwb/tree/dev/tests/integration/ros3 which is the only other place in the tests where we test different drivers.

OK - added tests for the new keyword injections

@CodyCBakerPhD
Copy link
Collaborator Author

Oh wow, forgot about this one. Thanks @rly

@rly rly merged commit 4a38196 into dev Jan 18, 2023
@rly rly deleted the allow_driver_specification_in_validation branch January 18, 2023 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: enhancement improvements of code or code behavior topic: validator issues related to validation of files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants