fix(gui): omit --frames for entire-video inference (#2808)#2809
Open
tom21100227 wants to merge 1 commit into
Open
fix(gui): omit --frames for entire-video inference (#2808)#2809tom21100227 wants to merge 1 commit into
tom21100227 wants to merge 1 commit into
Conversation
Contributor
Docs Preview
API changes detected - View API Reference This preview will be removed when the PR is closed. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2809 +/- ##
========================================
Coverage 70.69% 70.70%
========================================
Files 103 103
Lines 23359 23374 +15
========================================
+ Hits 16514 16526 +12
- Misses 6845 6848 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The "Entire video" inference option encoded its selection as the range
[0, len(video)) and serialized it to a fragile `--frames 0,-N` argument.
That form only decodes as a range via a legacy `rstrip(",")` quirk in
sleap-nn's `frame_list`, and it pins the endpoint to the GUI's frame
count. When that count exceeds the number of frames the inference video
reader can decode (off-by-one indexing, or a video whose metadata
over-reports its length), inference requests a nonexistent frame and
crashes with `IndexError: Failed to read frame index N`.
Add `VideoItemForInference.is_entire_video` and omit `--frames` for that
case so the inference CLI predicts on all frames using its own frame
count (its documented default). Clip and explicit-frame selections are
unchanged.
Fixes #2808. Reported in #2807.
With the help of Claude.
tom21100227
force-pushed
the
triage/2807-frames
branch
from
July 1, 2026 22:20
7ab4981 to
9a5f1d0
Compare
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.
Problem
Running inference from the GUI with the frame range set to "Entire video" builds a command containing
--frames 0,-N(e.g.--frames 0,-10245). This was reported in discussion #2807, where it crashes withIndexError: Failed to read frame index N.Two things are wrong with the
0,-Nencoding:MainWindow._get_frames_for_predictionencodes "Entire video" as the half-open range[0, len(video))→[0, -len(video)], andVideoItemForInference.cli_argsserializes that to--frames 0,-N. That string only decodes back into a range because sleap-nn'sframe_listhappens to domin_max[0].rstrip(","). It reads like the two-frame list[0, -N](exactly how the reporter interpreted it).IndexError: Failed to read frame index N.Fix
Add
VideoItemForInference.is_entire_videoand, when it's true, omit--framesso the inference CLI predicts on all frames using its own frame count (the documented default when--framesis not passed). This drops the fragile encoding and decouples the run from the GUI's count.--frames 0,-N[a, b)--frames a,-(b-1)--frames 0,1,2,…--frames kTesting
tests/gui/learning/test_cli_construction.py::TestVideoItemForInferenceCLI— added 3 tests (entire-video omits--frames; a shorter clip still emits--frames; explicit lists are never treated as entire-video). All 9 tests in the class pass.Notes
If the crash still reproduces for a specific video after this change, the video's own metadata over-reports its decodable frame count (a corrupted / variable-frame-rate file). That is a separate reader-robustness concern in sleap-nn / sleap-io, and the plan is to confirm with the reporter and request the video if needed.
Fixes #2808. Reported in #2807.