Skip to content

Record the actual NWB Inspector version in validation results - #1966

Merged
CodyCBakerPhD merged 2 commits into
masterfrom
claude/nwbinspector-version-cache-1xmfb8
Oct 16, 2026
Merged

CodyCBakerPhD merged 2 commits into
masterfrom
claude/nwbinspector-version-cache-1xmfb8

Conversation

@CodyCBakerPhD

Copy link
Copy Markdown
Contributor

User-visible effect

  • NWB Inspector validation results now carry the real nwbinspector version. Every NWB Inspector record from dandi validate (and from validation during dandi upload) now has origin.validator_version set to the installed nwbinspector version, e.g. "0.7.2". Before this change it was always "". The archive can now match these records to the nwbinspector version that produced them.
  • The "outdated NWB Inspector" warning can now actually appear. If etelemetry reports a newer nwbinspector release than the one installed, dandi validate logs the existing "NWB Inspector version X is installed - please use the latest release…" warning once per process. Before this change that code never ran.
    • If the etelemetry server can't be reached (e.g. offline), the existing Failed to retrieve NWB Inspector version due to … warning is now logged once per process.
    • Setting NO_ET (etelemetry's own opt-out) skips the check silently.

The bug

_get_nwb_inspector_version() in dandi/files/bases.py cached the version in a module global that started as "", but the cache check was is not None. Since "" is not None, the function always returned "" right away: the version was never looked up and the etelemetry check below it never ran.

Changes

  • dandi/files/bases.py
    • The cache now starts as None, typed as Version | None. That matches what nwbinspector.utils.get_package_version() returns (packaging.version.Version), so the existing < max_version comparison still works. _get_nwb_inspector_version() is annotated -> Version, and the caller still does str(...).
    • The lookup runs under a Lock. dandi upload validates assets in up to 5 threads at once, and without the lock several threads could each run the etelemetry lookup and log duplicate warnings. Most of this diff is re-indentation from the lock; it reads best with whitespace ignored.
    • The etelemetry query moved into a small helper, _get_latest_nwb_inspector_version(). It returns None when etelemetry.get_project() does, which happens when NO_ET is set (as in our CI). Without that, users with NO_ET set (and CI) would get a confusing TypeError: 'NoneType' object is not subscriptable warning. The existing try/except around the check is unchanged.
  • dandi/tests/fixtures.py: new autouse fixture no_nwbinspector_etelemetry that stubs out the helper. Tests that validate NWB files then never query etelemetry over the network and never log "please upgrade" or "failed to retrieve" warnings. It patches only this dandi-private helper, not etelemetry or NO_ET globally, so other projects that load the dandi pytest plugin aren't affected.
  • dandi/tests/test_files.py: new tests, all marked @pytest.mark.ai_generated:
    • test_validate_nwb_records_nwbinspector_version: validates the real simple4_nwb fixture and checks that every NWB Inspector result's origin.validator_version equals the installed nwbinspector version. Both sides are normalized with packaging.version.Version.
    • test_get_nwb_inspector_version_cached[...]: runs in a fresh interpreter, so the cache starts in the same state as for dandi validate, with etelemetry.get_project replaced by a counting fake (no network). It checks that two calls return the same installed version and etelemetry is queried exactly once. It is parametrized over the NO_ET case (no warning) and a newer upstream release (exactly one outdated warning). Resetting the cache in-process wouldn't catch this bug, because the bug was in the initial value.

The synthetic validator_version="" placeholders in dandi/cli/tests/test_cmd_validate.py (formatter tests) are deliberately left alone.

Testing

  • New tests fail on master ({''} == {'0.7.2'} and ['', ''] == ['0.7.2', '0.7.2']) and pass with the fix.
  • dandi/cli/tests/test_cmd_validate.py, dandi/tests/test_files.py, and dandi/validate/: 134 passed, 3 skipped.
  • mypy dandi: no issues. pre-commit (black, isort, codespell, flake8, reuse): all passed.
  • End-to-end: dandi validate -f json_lines on an NWB file with best-practice violations now shows "validator_version": "0.7.2" on the NWBI.* records.

🤖 Generated with Claude Code

https://claude.ai/code/session_011mgA1x4jnEavPHXQkyrnEk


Generated by Claude Code

`_get_nwb_inspector_version()` cached the version in a module global that
started out as "" but was guarded with `is not None`, so it always returned
"" without ever looking the version up.  As a result every NWB Inspector
validation result carried an empty `origin.validator_version`, and the
etelemetry check warning about an outdated NWB Inspector never ran.

Use a `None` sentinel (typed as `packaging.version.Version | None`, which is
what `nwbinspector.utils.get_package_version()` returns), and hold a lock
while looking the version up, since `dandi upload` validates assets
concurrently, so that the lookup and the etelemetry check happen only once
per process.  The etelemetry query is moved into a small helper, which also
skips the check quietly when etelemetry is disabled via `NO_ET` (in which
case `etelemetry.get_project()` returns `None`) instead of warning about a
`TypeError`.

In tests, an autouse fixture stubs out that helper so that validating NWB
files never queries etelemetry over the network or emits "please upgrade"
warnings.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011mgA1x4jnEavPHXQkyrnEk
@CodyCBakerPhD CodyCBakerPhD added patch Increment the patch version when merged cmd-validate labels Oct 16, 2026 — with Claude
@CodyCBakerPhD CodyCBakerPhD self-assigned this Oct 16, 2026
Comment thread dandi/files/bases.py Fixed
Use the module global directly instead of a local copy (as the code did
before), so that CodeQL no longer reports the assignment to it as unused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011mgA1x4jnEavPHXQkyrnEk
@codecov

codecov Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.67442% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.89%. Comparing base (0d5f96e) to head (57a2f15).

Files with missing lines Patch % Lines
dandi/files/bases.py 94.44% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1966      +/-   ##
==========================================
+ Coverage   90.76%   90.89%   +0.13%     
==========================================
  Files          92       92              
  Lines       14159    14190      +31     
==========================================
+ Hits        12851    12898      +47     
+ Misses       1308     1292      -16     
Flag Coverage Δ
unittests 90.89% <97.67%> (+0.13%) ⬆️

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

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@CodyCBakerPhD
CodyCBakerPhD marked this pull request as ready for review October 16, 2026 02:20
@CodyCBakerPhD
CodyCBakerPhD merged commit 44fd1a6 into master Oct 16, 2026
41 checks passed
@CodyCBakerPhD
CodyCBakerPhD deleted the claude/nwbinspector-version-cache-1xmfb8 branch October 16, 2026 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd-validate patch Increment the patch version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants