Preserve legacy ndx-pose video references during organize

[AtomicGlance]

Fixes #1817

Older ndx-pose files can store source-video paths in the deprecated PoseEstimation.original_videos field. dandi organize --update-external-file-paths previously renamed the linked ImageSeries.external_file values but left those legacy strings pointing at the old location.

This change reuses the exact source-video old-to-new mapping while an NWB file is open and updates only matching, non-URL original_videos entries. It identifies the container by its neurodata_type, so ndx-pose is not a required runtime dependency. Newer source_video links and unrelated containers are left untouched.

Tests cover path-separator normalization, byte/string values, remote URLs, and unrelated containers.

Validation: focused regression test and flake8 pass.

[codecov]

Codecov Report

❌ Patch coverage is 97.29730% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.44%. Comparing base (d91a041) to head (f1a3e99).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
dandi/pynwb_utils.py	91.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1916      +/-   ##
==========================================
+ Coverage   77.35%   77.44%   +0.09%     
==========================================
  Files          89       89              
  Lines       13325    13397      +72     
==========================================
+ Hits        10307    10375      +68     
- Misses       3018     3022       +4     

Flag	Coverage Δ
unittests	77.44% <97.29%> (+0.09%)	⬆️

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.

[AtomicGlance]

I added the remaining regression coverage before asking for review: a close/reopen HDF5 persistence test, coverage through rename_nwb_external_files(), and cases for empty mappings, missing/scalar values, and mixed matched/unmatched paths. Byte-valued references are also decoded before URL detection now.

The full test_pynwb_utils.py module, lint, typing, every CI test environment (including NFS), and both Codecov checks are green. I kept the change limited to the deprecated original_videos field. @h-mayorquin, would you prefer labeled_videos to be handled as part of the same compatibility fix, or left for a separate follow-up? If this scope looks right, could a maintainer add the patch label and review it?

[yarikoptic-gitmate]

Nice, targeted fix. I checked it beyond just reading the diff: built a real NWB file with an actual ndx-pose PoseEstimation + ImageSeries, ran rename_nwb_external_files end-to-end, and confirmed original_videos and external_file both get renamed and persisted correctly — including with ndx-pose uninstalled (pynwb reconstructs PoseEstimation dynamically from the file's cached namespace spec), which validates the "not a required runtime dependency" claim. The unit tests are thorough on the edge cases that matter (byte/str values, path-separator normalization, URLs left alone, unrelated containers, longer replacement strings for variable-length HDF5 strings).

Two small things before this is good to merge:

1. Were the new tests in test_pynwb_utils.py written with AI assistance? If so, per DEVELOPMENT.md they should carry the @pytest.mark.ai_generated marker, as is done elsewhere in the suite (e.g. test_cmd_validate.py, test_base.py).
2. Could you extend the rename_nwb_external_files docstring to mention that it now also updates legacy PoseEstimation.original_videos references (not just ImageSeries.external_file)? Would help future readers who don't dig into _rename_pose_estimation_original_videos.

Otherwise this looks solid to me.

---

Generated by Claude Code

[AtomicGlance]

Addressed both points from the review:

• Marked the four new tests in dandi/tests/test_pynwb_utils.py with @pytest.mark.ai_generated, as required by DEVELOPMENT.md.
• Expanded rename_nwb_external_files()’s docstring to describe updates to legacy PoseEstimation.original_videos references alongside ImageSeries.external_file.

The focused utility tests pass locally (7 passed), and the touched files pass the targeted flake8 check. The update is in commit 4cc085d5; the branch is ready for another look.

[AtomicGlance]

The code and required checks for #1916 and #1917 are green; the remaining check_labels failures are the repository label gate. When convenient, could a maintainer apply patch to #1916 and minor to #1917 and review them together? No additional code commit is needed for the label check. @yarikoptic

[yarikoptic-gitmate]

Re-reviewed at f1a3e99. Both prior points are addressed — the docstring now documents the original_videos side effect, and all four new tests carry @pytest.mark.ai_generated. The core logic in _rename_pose_estimation_original_videos/_external_reference_key is unchanged from the version I already exercised end-to-end (real ndx-pose file, both with the extension installed and with it uninstalled via pynwb's dynamic-class path), so that verification still stands.

On the performance question specifically: _rename_pose_estimation_original_videos walks nwb.objects.values(), which is the container graph pynwb already fully materializes during the pre-existing io.read() call in this same function — so the scan adds no extra I/O or re-parsing, just an in-memory dict iteration with a cheap getattr per container. I benchmarked this directly: a synthetic file with 5000 containers took ~18s to read() (dominated by pynwb/hdmf construction, unrelated to this PR) versus ~0.5ms to run the new scan over all 5003 resulting objects — over 4 orders of magnitude smaller than the cost the feature already pays to open the file. It's also gated by if not renames: return, so files with no non-URL external-file renames (the common case) skip the scan entirely, and it runs once per file rather than once per renamed path. I don't see an avoidable hit here.

Logic-wise nothing new to flag: matching is still by normalized path string (backslash→forward-slash, bytes decoded) against the exact old→new map built from the ImageSeries rename pass, URLs are skipped on both sides, and non-list original_videos values (missing/str/bytes) are safely skipped.

LGTM.

---

Generated by Claude Code

[github-actions]

🚀 PR was released in 0.79.0 🚀