Fix frame extraction failure with pathlib video selections - #3449
Open
C-Achard wants to merge 7 commits into
Open
Fix frame extraction failure with pathlib video selections#3449C-Achard wants to merge 7 commits into
C-Achard wants to merge 7 commits into
Conversation
Improve frame extraction and GUI cropping to match selected videos against config entries using normalized `Path` values instead of raw string equality. This preserves original config keys for updates, prevents silent no-op runs by raising explicit errors when no selected videos match or none are processed, and ensures GUI-selected files are passed as strings while still resolving path-format differences.
Adds a new test module for `generate_training_dataset.frame_extraction` to cover video path normalization behavior. The tests verify that `extract_frames` accepts `Path` objects in `videos_list`, that `_filter_config_videos` correctly matches `str` and `Path` values while preserving original config keys/types, and that edge cases like `None`, non-matching selections, and Windows case-insensitive matching behave as expected.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a regression where frame extraction could silently process zero videos when videos_list contained pathlib.Path objects while config video-set keys were strings (leading to incorrect “corrupted video” reporting), and adds regression coverage.
Changes:
- Add path normalization +
_filter_config_videos()to compare selected videos against config keys while preserving original config keys for lookups. - Improve error handling when no videos are processed / when a selection matches none of the configured videos.
- Update GUI cropping + extraction to match videos via normalized paths and to pass
videos_listas strings; add new tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
deeplabcut/generate_training_dataset/frame_extraction.py |
Adds normalization/filtering helpers and updates extraction logic + empty-selection handling (but currently introduces a mode=="match" filtering bug). |
deeplabcut/gui/tabs/extract_frames.py |
Uses normalized path matching when updating crop entries and ensures videos_list passed to extraction is stringified. |
tests/generate_training_dataset/test_frame_extraction.py |
Adds regression tests for Path-vs-string filtering (but currently contains import/monkeypatch issues). |
Suppressed comments (1)
tests/generate_training_dataset/test_frame_extraction.py:63
- This monkeypatch targets
deeplabcut.utils.io.imsave, butextract_frames()writes viaskimage.io.imsave(imported inside the function). As written, the test will still write PNGs to disk and won’t isolate I/O as intended.
# Avoid writing an actual PNG.
monkeypatch.setattr(io, "imsave", lambda *args, **kwargs: None)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Update `test_frame_extraction.py` to import `io` from `skimage` and remove the incorrect `deeplabcut.utils` `io` import. This aligns the test with the intended image I/O dependency and avoids using the wrong module.
…b.com/DeepLabCut/DeepLabCut into cy/fix-frame-extraction-path-matching
deruyter92
reviewed
Aug 18, 2026
Collaborator
|
Thanks for addressing this! And great that you added tests 💪 |
Collaborator
|
Side note: when do we think would be a good moment to update the config schema a bit? I think we all agree that having the file paths as mapping keys in the configs has been the cause of a lot of issues. |
Collaborator
Author
|
@deruyter92 Thanks for the review ! As discussed, I think we're better off implementing additional validation for e.g project layout and keypoint data. |
Refactors video filtering to use explicit `Iterable`/`Collection` and `Path`-aware type hints, and removes the redundant `normalize_video_path` helper. `extract_frames` now normalizes `videos_list` to `Path` objects up front, making selected-video matching more consistent and type-safe.
Collaborator
Author
|
All comments addressed, thank you! |
C-Achard
marked this pull request as ready for review
August 26, 2026 11:25
Replace normalized string-based video path matching in `select_cropping_area` with direct `Path` object comparison, and remove the now-unused `normalize_video_path` import. This simplifies lookup logic for finding the project config key associated with a selected video.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Frame extraction could process zero videos when
videos_listcontainedPathobjects and configuration keys were strings.This caused valid videos to be incorrectly reported as corrupted.
Closes #3448.
Fix