Fix frame extraction failure with pathlib video selections - #3449
Fix frame extraction failure with pathlib video selections#3449C-Achard wants to merge 5 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.
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
| def normalize_video_path(video: str | Path) -> Path: | ||
| return Path(video) |
There was a problem hiding this comment.
To me, this helper feels like an unnecessary abstraction of inline Path with is perfectly readable (even shorter).
| def _filter_config_videos( | ||
| configured_videos, | ||
| selected_videos, | ||
| ) -> list: |
There was a problem hiding this comment.
would be helpful to add type hints. Also, let's add/adjust the type hints of the extract_frames videos_list parameter.
| if selected_videos is None: | ||
| return configured_videos | ||
|
|
||
| selected = {normalize_video_path(video) for video in selected_videos} |
There was a problem hiding this comment.
I would prefer normalizing videos_list at the beginning of extract_frames instead of doing it inside this helper.
|
Thanks for addressing this! And great that you added tests 💪 |
|
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. |
|
@deruyter92 Thanks for the review ! As discussed, I think we're better off implementing additional validation for e.g project layout and keypoint data. |
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