Check for usage of latest NWB Inspector release #1108

CodyCBakerPhD · 2022-08-24T15:29:55Z

Hey all,

as a companion to the upcoming enforcement of more rigorous subject metadata requirements for DANDI upload (all taken care of by NeurodataWithoutBorders/nwbinspector#247) it occurred to me that in order to both (i) prevent users from bypassing these requirements by downgrading their NWB Inspector version or (ii) prevent fewer future lower bound version bumps of the nwbinspector package on the DANDI requirements side; it would be best to simply check that the person is using the latest PyPI/conda-forge release of the package at time of validation.

I'm not really sure how to add a test for this though, let me know if you have ideas for that.

codecov · 2022-08-24T16:26:44Z

Codecov Report

Merging #1108 (2274902) into master (e74c5ac) will increase coverage by 0.06%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##           master    #1108      +/-   ##
==========================================
+ Coverage   88.59%   88.66%   +0.06%     
==========================================
  Files          76       76              
  Lines        9108     9119      +11     
==========================================
+ Hits         8069     8085      +16     
+ Misses       1039     1034       -5

Flag	Coverage Δ
unittests	`88.66% <63.63%> (+0.06%)`	⬆️

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

Impacted Files	Coverage Δ
dandi/tests/fixtures.py	`98.06% <ø> (ø)`
dandi/files/bases.py	`77.34% <63.63%> (-0.62%)`	⬇️
dandi/metadata.py	`87.02% <0.00%> (+1.01%)`	⬆️
dandi/_version.py	`45.96% <0.00%> (+1.75%)`	⬆️

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

dandi/files/bases.py

setup.cfg

CodyCBakerPhD · 2022-08-25T15:59:45Z

.github/workflows/test.yml

+    - name: Install dev version of nwbinspector
+      if: matrix.mode == 'dev-deps'
+      run: |
+        pip install git+https://github.com/NeurodataWithoutBorders/nwbinspector


Went ahead and added the NWB Inspector dev branch to the dev-deps testing mode alongside PyNWB.

Let me know if you'd prefer me to revert this, though.

Now, what is a bit odd here, when I ran the testing suite on a local clone of dandi-cli with the dev branch of nwbinspector also installed in the same virtualenv, all the tests passed.

Reason I find that odd, is the fixtures define metadata that ought to be flagged if those fixtures are being used to test dandi validation, specifically this line: https://github.com/dandi/dandi-cli/blob/master/dandi/tests/fixtures.py#L117, which should be "mus musculus" to pass the NWB Inspector.

I could not tell at a glance, but where in the testing suite are those fixtures used to test validation of NWB assets?

I would fold this pip install into previous step and just adjust description that it is dev version of both pynwb and nwbinspector.

Rationale

do not duplicate conditioning on dev-deps

shorter

I could not tell at a glance, but where in the testing suite are those fixtures used to test validation of NWB assets?

$> git grep simple2_nwb dandi/cli/tests/test_command.py:def test_smoke(simple2_nwb, command): dandi/cli/tests/test_command.py: r = runner.invoke(command, [simple2_nwb]) dandi/tests/fixtures.py:def simple2_nwb( dandi/tests/fixtures.py: simple2_nwb: str, tmp_path_factory: pytest.TempPathFactory dandi/tests/fixtures.py: organize, ["-f", "copy", "--dandiset-path", str(tmp_path), str(simple2_nwb)] dandi/tests/fixtures.py: simple2_nwb: str, dandi/tests/fixtures.py: shutil.copy(simple2_nwb, tmp_path) dandi/tests/test_files.py:def test_validate_simple2(simple2_nwb): dandi/tests/test_files.py: errors = dandi_file(simple2_nwb).get_validation_errors() dandi/tests/test_files.py:def test_validate_simple2_new(simple2_nwb): dandi/tests/test_files.py: errors = dandi_file(simple2_nwb).get_validation_errors( dandi/tests/test_organize.py:def test_ambiguous(simple2_nwb: str, tmp_path: Path) -> None: dandi/tests/test_organize.py: copy2 = copy_nwb_file(simple2_nwb, tmp_path) dandi/tests/test_organize.py: args = ["--files-mode", "copy", "-d", outdir, simple2_nwb, copy2] dandi/tests/test_organize.py: for f in [simple2_nwb, copy2]

and since used in those other fixtures:

$> git grep organized_nwb_dir | grep test_ dandi/tests/test_fixtures.py:def test_organized_nwb_dir(organized_nwb_dir: Path) -> None: dandi/tests/test_fixtures.py:def test_organized_nwb_dir2(organized_nwb_dir2: Path) -> None: dandi/tests/test_upload.py: new_dandiset: SampleDandiset, organized_nwb_dir: str, tmp_path: Path dandi/tests/test_upload.py: p for p in list_paths(organized_nwb_dir) if p.name != dandiset_metadata_file dandi/tests/test_upload.py: parent, name = nwb_file.relative_to(organized_nwb_dir).parts

I would fold this pip install into previous step and just adjust description that it is dev version of both pynwb and nwbinspector.

Rationale

do not duplicate conditioning on dev-deps
shorter

Sure thing

and since used in those other fixtures:

Ah, thanks so much! I've updated the fixtures to the new requirements as well.

CodyCBakerPhD · 2022-08-31T17:51:48Z

@yarikoptic @jwodder Ready for review

yarikoptic

Must not error out if outdated or forbid validation if offline

setup.cfg

yarikoptic · 2022-08-31T18:18:54Z

dandi/files/bases.py

@@ -453,6 +456,14 @@ def get_metadata(
        metadata.path = self.path
        return metadata

+    def _get_inspector_versions(self):
+        url = "https://pypi.org/pypi/nwbinspector/json"
+        data = json.loads(requests.get(url=url).text)


consider using etelemetry from @satra et al: https://pypi.org/project/etelemetry/ which we already use for dandi-cli itself. Doesn't need to be in the same PR

Tried,

etelemetry.get_project(repo="NeurodataWithoutBorders/nwbinspector")

results in

RuntimeError: Connection to server could not be made

indeed still down, eh sensein/etelemetry-client#35 attn @satra

should be back up now.

Using etelemetry now

It simplified enough lines so that there's really no point to having it be a separate function, so I moved all the logic into the try of the logger-wrapping section

yarikoptic · 2022-08-31T18:22:02Z

dandi/files/bases.py

+                # Ensure latest version of NWB Inspector is installed and used client-side
+                current_version, max_version = self._get_inspector_versions()
+                if current_version < max_version:
+                    errors.extend(


I would not make demands such strong as to make it The only error and overall error out.

Please

add a dedicated try/except around the point of your interaction with the Internet. I might be offline and I still want to be able to run validation!

make it into a lgr.warning and proceed "as usual" without return.

Out of curiosity, how does dandi upload handle internet connectivity? Would it error out here or somewhere below:

dandi-cli/dandi/upload.py

Lines 67 to 69 in ef097cc

with DandiAPIClient(api_url) as client:

client.check_schema_version()

client.dandi_authenticate()

Fixed as requested, though.

out here or somewhere below:

I bet it should fail somewhere "here" -- just try? ;)

dandi/files/bases.py

jwodder · 2022-08-31T19:10:38Z

dandi/files/bases.py

@@ -12,12 +12,15 @@
 from threading import Lock
 from typing import Any, BinaryIO, Generic, Optional
 from xml.etree.ElementTree import fromstring
+import json
+from packaging import version


I can tell that you didn't run pre-commit on this code. Please do pip install pre-commit, then run pre-commit install in your clone of dandi, and then pre-commit run -a to ensure all your contributions so far have been formatted, linted, & isorted.

Done

Have y'all thought about using https://pre-commit.ci/ to automatically resolve such things? Since we've started using it it's so nice to never having to think about

dandi/files/bases.py

jwodder · 2022-08-31T19:14:56Z

dandi/files/bases.py

+                        self.filepath,
+                        type(e).__name__,
+                        str(e),


These arguments are just going to be ignored, as there are no placeholders in the log message to display them. Did you mean to include format placeholders?

Oh, sorry - copied/pasted from https://github.com/catalystneuro/dandi-cli/blob/force_usage_of_latest_nwb_inspector_release/dandi/files/bases.py#L446-L451 and didn't look very closely - I had assumed they were extra arguments to some kind of logger warning class that captured type/message as separate properties rather than just a single string.

Updated it to be a proper format-string now.

You should use %s placeholders instead of f-strings for log messages, in order to avoid an error if a stringified exception contains a %.

Can you give an example of that?

The code you linked to in your previous comment does that; note the %s's in the first argument to log.warning().

The code you linked to in your previous comment does that; note the %s's in the first argument to log.warning().

I mean, can you provide an example of the justification for

instead of f-strings for log messages, in order to avoid an error if a stringified exception contains a %.

If str(e) equals, say, "Bad format string: '%s %q'", then using it in an f-string will cause it to be a substring of the first argument of log.warning(), and so log.warning() will try to fill in the %s with an argument that isn't there and/or choke on the %q. Yes, this is an unlikely scenario in this particular instance, but it costs you nothing to do it the correct, foolproof way from the start.

dandi/files/bases.py

CodyCBakerPhD · 2022-09-01T14:12:55Z

Unable to view what's off with the CI right now, but I'm assuming it's the keyring issue. #1111

yarikoptic · 2022-09-02T14:17:26Z

dandi/support/bids/utils.py

@@ -0,0 +1,200 @@
+"""


why these files are added back in this PR? (likely some rebase went a bit "dirty")

Hmmm... Yesterday I

-> implemented etelemetry
-> synched catalystneuro:master to dandi-cli:master
-> merged catalystneuro:master into this branch

will look into this to see what went wrong

OK whatever caused it it must have happened before the etelemetry change. It's been manually removed from this branch now

CodyCBakerPhD · 2022-09-02T16:30:05Z

dandi/support/pyout.py

@@ -36,7 +36,7 @@ def naturalsize(v):
    return humanize.naturalsize(v)


-def datefmt(v, fmt=u"%Y-%m-%d/%H:%M:%S"):
+def datefmt(v, fmt="%Y-%m-%d/%H:%M:%S"):


pre-commit will not let me restore these changes - I tried manually putting them back to the way they were but every time I try to commit it, pre-commit triggers and undoes them.

I would have done

poor man git stash: did git diff setup.cfg dandi/files/bases.py .github/ >| /tmp/p

git reset --hard origin/master # forgot the history of 26!!! commits for the desired change to happen

cat /tmp/p | patch -p1 # apply changes of interest

git commit -m "Check ...." -a # commit changes of history

git push -f # make this PR look glorious 1 commit without any side effects etc

I would recommend reading https://github.blog/2020-04-07-celebrating-15-years-of-git-an-interview-with-git-maintainer-junio-hamano/ in particular the last question/section ;)

I can certainly try it, but you can always squash it on merge to the same effect, though, right?

E.g.,

A bit less work that way IMO

this PR still has unrelated changes we do not want to merge/squash in, right?

Not since I forcibly restored the changes to supper/pyout.py. Now the changelog represents only the related changes to the NWB Inspector and NWB asset validation.

Just wanted you to be aware that following jwodder's instructions to the letter resulted in pre-commit automatically doing all of the changes to that file.

dandi/tests/fixtures.py is still in the diff and to be gone if you like to continue your fight!

The changes to dandi/tests/fixtures.py are intentional updates to keep them compatible with the new validation requirements

The git stash step didn't work out for me, but

git reset $(git merge-base master $(git branch --show-current)) git add -A git commit --no-verify -m "manual squash" # Need --no-verify to skip the pyout.py changes git push -f

did

yarikoptic · 2022-09-03T16:21:04Z

Thank you! Let's see how it goes

github-actions · 2022-09-19T16:20:40Z

🚀 PR was released in 0.46.3 🚀

CodyCBakerPhD mentioned this pull request Aug 24, 2022

Update dandi.inspector_config.yaml NeurodataWithoutBorders/nwbinspector#247

Merged

4 tasks

CodyCBakerPhD marked this pull request as ready for review August 24, 2022 16:31

CodyCBakerPhD commented Aug 25, 2022

View reviewed changes

dandi/files/bases.py Outdated Show resolved Hide resolved

CodyCBakerPhD commented Aug 25, 2022

View reviewed changes

setup.cfg Show resolved Hide resolved

CodyCBakerPhD commented Aug 25, 2022

View reviewed changes

yarikoptic requested changes Aug 31, 2022

View reviewed changes

jwodder requested changes Aug 31, 2022

View reviewed changes

yarikoptic reviewed Sep 2, 2022

View reviewed changes

CodyCBakerPhD force-pushed the force_usage_of_latest_nwb_inspector_release branch from 6d879e7 to 5e356fe Compare September 2, 2022 16:20

CodyCBakerPhD commented Sep 2, 2022

View reviewed changes

manual squash

2274902

CodyCBakerPhD force-pushed the force_usage_of_latest_nwb_inspector_release branch from 2870bff to 2274902 Compare September 2, 2022 20:17

CodyCBakerPhD requested a review from yarikoptic September 2, 2022 21:39

yarikoptic merged commit 17e0b04 into dandi:master Sep 3, 2022

CodyCBakerPhD deleted the force_usage_of_latest_nwb_inspector_release branch September 3, 2022 18:46

github-actions bot added the released label Sep 19, 2022

This was referenced Nov 9, 2022

file fails validation #1031

Closed

Update test_validate_nwb_path_grouping test #1157

Merged

	with DandiAPIClient(api_url) as client:
	client.check_schema_version()
	client.dandi_authenticate()

Check for usage of latest NWB Inspector release #1108

Check for usage of latest NWB Inspector release #1108

Conversation

CodyCBakerPhD commented Aug 24, 2022

codecov bot commented Aug 24, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodyCBakerPhD commented Aug 31, 2022

yarikoptic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodyCBakerPhD Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodyCBakerPhD commented Sep 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarikoptic commented Sep 3, 2022

github-actions bot commented Sep 19, 2022

codecov bot commented Aug 24, 2022 •

edited

Loading

CodyCBakerPhD Aug 31, 2022 •

edited

Loading