Skip to content

Fix broken external_file paths when organizing on Windows#1832

Merged
CodyCBakerPhD merged 3 commits intodandi:masterfrom
h-mayorquin:fix_organize_bug
Apr 10, 2026
Merged

Fix broken external_file paths when organizing on Windows#1832
CodyCBakerPhD merged 3 commits intodandi:masterfrom
h-mayorquin:fix_organize_bug

Conversation

@h-mayorquin
Copy link
Copy Markdown
Contributor

@h-mayorquin h-mayorquin commented Apr 9, 2026

When dandi organize --update-external-file-paths runs on Windows, the external_file paths written into NWB files use backslashes instead of forward slashes.

Expected:

ses-2026-02-12-1_image/21b52ce3_external_file_0.mp4

On Windows before this fix:

ses-2026-02-12-1_image\21b52ce3_external_file_0.mp4

Downstream tools expecting forward slashes (as DANDI/S3 asset keys always use) fail silently when resolving these paths against the archive. This was observed in Dandiset 001771 and reported in catalystneuro/nwb-video-widgets#33, where get_dandi_video_info() returned empty results. A defensive workaround was merged in catalystneuro/nwb-video-widgets#38, but the root cause is here in dandi organize.

The fix has two parts:

  • Write side (organize.py): _create_external_file_names() used os.path.join to build external file paths. On Windows that produces backslashes. Replaced with posixpath.join since these are DANDI/S3 keys, not local filesystem paths.
  • Read side (pynwb_utils.py): _get_image_series() wrapped external file strings in Path(), which on Windows converts forward slashes back to backslashes. Replaced with PurePosixPath.

I have also added a regression assertion in test_video_organize that external_file values never contain backslashes. The initial push included only the write-side fix; Windows CI caught that Path() on the read side was re-introducing backslashes, confirming the assertion works as intended.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 9, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.27%. Comparing base (fa33e8d) to head (b885ad6).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
dandi/organize.py 50.00% 1 Missing ⚠️
dandi/pynwb_utils.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1832      +/-   ##
==========================================
- Coverage   76.28%   76.27%   -0.02%     
==========================================
  Files          87       87              
  Lines       12482    12484       +2     
==========================================
  Hits         9522     9522              
- Misses       2960     2962       +2     
Flag Coverage Δ
unittests 76.27% <66.66%> (-0.02%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 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.

@h-mayorquin
Copy link
Copy Markdown
Contributor Author

I can't add labels (no write access), a maintainer will need to add patch. The codecov checks are failing but the actual changes shouldn't reduce coverage.

@CodyCBakerPhD CodyCBakerPhD added bug Something isn't working cmd-organize patch Increment the patch version when merged labels Apr 10, 2026
@CodyCBakerPhD
Copy link
Copy Markdown
Contributor

@h-mayorquin Perhaps worth an NWB Inspector check as well?

@h-mayorquin
Copy link
Copy Markdown
Contributor Author

@h-mayorquin Perhaps worth an NWB Inspector check as well?

Yes:
NeurodataWithoutBorders/nwbinspector#697

@CodyCBakerPhD
Copy link
Copy Markdown
Contributor

LGTM

Thanks for the catch

@h-mayorquin
Copy link
Copy Markdown
Contributor Author

what's up with the code coverage?

@CodyCBakerPhD
Copy link
Copy Markdown
Contributor

what's up with the code coverage?

Something to do with how it counts import lines

All source code lines look to be hit

@CodyCBakerPhD CodyCBakerPhD merged commit 0af950f into dandi:master Apr 10, 2026
36 of 38 checks passed
@h-mayorquin h-mayorquin deleted the fix_organize_bug branch April 10, 2026 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cmd-organize patch Increment the patch version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants