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

Update dandi.inspector_config.yaml #247

Merged
merged 24 commits into from
Aug 24, 2022
Merged

Update dandi.inspector_config.yaml #247

merged 24 commits into from
Aug 24, 2022

Conversation

bendichter
Copy link
Contributor

@bendichter bendichter commented Aug 3, 2022

Motivation

What was the reasoning behind this change? Please explain the changes briefly.

How to test the behavior?

Show here how to reproduce the new behavior (can be a bug fix or a new feature)

Checklist

  • Have you ensured the PR description clearly describes the problem and solutions?
  • Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
  • Is your code contribution compliant with black format? If not, simply pip install black and run black . inside your fork.
  • If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

@CodyCBakerPhD
Copy link
Contributor

Oh wow, is it finally time to elevate these to be DANDI upload requirements? 😲

Note the test here: https://github.com/NeurodataWithoutBorders/nwbinspector/blob/dev/tests/test_check_configuration.py#L68-L81

will have to be updated accordingly.

CodyCBakerPhD and others added 7 commits August 17, 2022 12:48
* propose changes to best practices

* Update docs/best_practices/nwbfile_metadata.rst

Co-authored-by: Cody Baker <[email protected]>

* Update docs/best_practices/nwbfile_metadata.rst

Co-authored-by: Cody Baker <[email protected]>

Co-authored-by: Cody Baker <[email protected]>
@bendichter
Copy link
Contributor Author

The way we have it now, check_subject_proper_age_range is not included in the dandi config. Do we want to add it?

@CodyCBakerPhD
Copy link
Contributor

@bendichter Good catch: #255

* Update dandi.inspector_config.yaml

* Update test_check_configuration.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@bendichter
Copy link
Contributor Author

Let's go ahead and merge this. I'd rather be ahead of DANDI's requirements than behind them

@CodyCBakerPhD
Copy link
Contributor

There is one possible change we might need to make to DANDI validation before going forward with this, which is to require (at time of performing dandi validate) the user to have the latest PyPI release of the NWB Inspector installed.

Otherwise, someone can simply install an earlier version of the Inspector and pass the new validation requirements without issue.

Some pseudo-code

import json
import urllib2
from packaging import version
from nwbinspector.utils import get_package_version

def check_current_version():
    url = "https://pypi.org/pypi/nwbinspector/json"
    data = json.load(urllib2.urlopen(urllib2.Request(url)))
    versions = data["releases"].keys()
    #  get max_version from the `versions` list
    return get_package_version(package_name="nwbinspector") == max_version

@bendichter
Copy link
Contributor Author

Yeah that makes sense

@CodyCBakerPhD
Copy link
Contributor

CodyCBakerPhD commented Aug 18, 2022

@bendichter OK do you think this deserves to be more on the DANDI side of things like in this dev branch: dandi/dandi-cli@master...catalystneuro:dandi-cli:force_usage_of_latest_nwb_inspector_release

OR we should make this a check function (or just a general-purpose assertion for all core inspection functions) for the NWBInspector in and of itself (which could warn people to update ahead of attempting dandi validate if they are following the user guide instructions and using the NWB Inspector early in the process)? [EDIT: obviously together with a simple lower bound version bump on the DANDI side, which is simpler for them to review]

@CodyCBakerPhD
Copy link
Contributor

@bendichter How about this - in NWB Inspector, we always check if user is using most recent version of the NWB Inspector and include it in the report/message stream if not.

Then on DANDI, bump the minimal version of the NWB Inspector to the version that enforces that?

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review August 24, 2022 15:20
@CodyCBakerPhD CodyCBakerPhD self-requested a review August 24, 2022 15:20
@codecov-commenter
Copy link

Codecov Report

Merging #247 (44e052a) into dev (0fafb09) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #247      +/-   ##
==========================================
+ Coverage   94.28%   94.39%   +0.11%     
==========================================
  Files          18       19       +1     
  Lines         962      982      +20     
==========================================
+ Hits          907      927      +20     
  Misses         55       55              
Flag Coverage Δ
unittests 94.39% <100.00%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
nwbinspector/__init__.py 100.00% <100.00%> (ø)
nwbinspector/checks/icephys.py 100.00% <100.00%> (ø)
nwbinspector/checks/nwbfile_metadata.py 98.98% <100.00%> (+0.15%) ⬆️

@CodyCBakerPhD
Copy link
Contributor

@bendichter OK since that last part is reliant on the DANDI side (see dandi/dandi-cli#1108) we can go ahead and merge this when you're ready

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.

3 participants