Make the moderation gate reject what it never cleared - #4
Merged
Merged
Conversation
A scan that classified nothing now fails the gate instead of passing it: the CLI exits 4 when every frame errored, and 2 when a file decoded to zero frames. Both used to exit 0, so `pyframe upload.gif || reject` accepted every upload while the backend was down or the file was truncated. --fail-on never still exits 0 throughout, as the explicit opt out. is_nsfw is now derived from the verdict rather than computed alongside it. A short circuited cascade takes max_score from the screen verdicts, which no classified frame backed, so the two could disagree, and the CLI gates on is_nsfw. - scanner.py: zero decoded frames raise MediaDecodeError instead of aggregating to a confident clean, and max_escalations below 1 is rejected before backend weights load, because a non-positive budget uncapped escalation rather than disabling it. _ensure_min_frames ranks filler frames by screen score with motion as a tiebreak, matching SuspicionSampler, where the old flat key compared a 0 to 1 score against a pixel diff sum reaching 1e6. - cli.py: exit 4 for an error verdict, exit 2 for an unknown backend or an out of range option, and the failure reason on stderr beneath the line it explains. - output.md, README.md: exit code 4 documented, and is_nsfw restated as true if and only if the verdict is nsfw. - tests: the CLI exit code matrix, which had no coverage at all, plus the cascade case where is_nsfw and verdict diverged, and the filler ranking. Every new assertion was run against the previous revision and fails there.
The uniform sampler now always includes the final frame. A strided slice lands on it only when the frame count minus one divides evenly by the stride, so up to stride minus one frames at the end of each clip sat outside the floor that the motion sampler exists to preserve. An NSFW event running to the end of a GIF had no sampled frame able to catch it. - sampling.py: append the last frame when the stride skipped it, compared by index because Frame holds an array and does not compare cleanly. - tests: the tail is present for a clip whose stride misses it, and is not duplicated for one whose stride already lands on it.
The cv2 decode path now has tests. iter_frames, the VideoCapture route every file scan takes, had no coverage at all, while an unbounded opencv-python-headless floor resolves to OpenCV 5 today, so a green run proved nothing about the decoder users actually reach. The bench scripts are linted alongside the package, and the matrix reaches the Python this is developed on. - ci.yml: lint scripts next to src and tests, which already carry their own ruff noqa markers and were only ever excluded by omission, and add 3.14 to the matrix now that all three base dependencies ship wheels for it. - pyproject.toml: pin ruff's current default rule set explicitly so a future release changing those defaults cannot quietly move what CI enforces, and declare the 3.14 classifier. - tests: decoding a real GIF from disk, frame count, timestamps, motion measured against the previous frame, the still image path, and the full extension map with its rejection cases.
Long videos scan in constant memory rather than exhausting the machine. Sampling now runs against per-frame metadata, which is tens of bytes a frame, and pixels are fetched in a second pass for the selected frames only. A 900 frame 640x480 clip holds 12 MB of frame data where it previously held 444 MB, and the 11 GB a minute of 1080p used to require never happens, so the mp4 and mkv support the README advertises works on real files. The cost is two sequential decodes instead of one, taken unconditionally. The condition that would make it conditional cannot be evaluated: deciding from a frame count means trusting CAP_PROP_FRAME_COUNT, which is exactly as unreliable as the seeking this deliberately avoids, and a rarely taken fast path is the one that rots. - media.py: FrameMeta carries index, timestamp and motion without pixels, and FrameLike is the structural type the samplers read. iter_frame_meta walks the whole file holding one frame at a time; iter_frames_at re-walks it and yields only the selected frames, skipping the rest with grab(). It never seeks, since CAP_PROP_POS_FRAMES is an approximate keyframe seek on long GOP codecs, VFR containers and GIF, and it carries motion from the meta rather than recomputing it, which would otherwise measure across the skipped gap. iter_frames and iter_frames_from_bytes keep their signatures and are rebuilt on the same private generators, so the two halves cannot drift. - scanner.py: scan and scan_bytes pass metadata plus a fetch callable. The cascade streams its screen pass, because that set is screen_fps times duration rather than max_frames and would still have held a long clip whole. max_frames below 1 is now rejected, for the same reason max_escalations is: the samplers read a non-positive budget as keep everything. - sampling.py, base.py: samplers take a type variable bound to FrameLike so metadata and frames both flow through unchanged, and classify_batch accepts any iterable, with the streaming contract stated for anyone overriding it. - performance.md, README.md: the measured before and after, and an honest note that a stubbed backend makes the second decode look worse than it is. - tests: peak live Frame count under a scan, with a positive control asserting the old path still peaks at the full length, so the bound cannot pass vacuously. Metadata and fetched pixels are asserted equal to a single full decode, frame for frame, on both the file and bytes paths.
Minor, because default runtime behaviour moved in several user visible ways: a scan that classified nothing exits 4 instead of 0, decoding holds the sample rather than the whole clip, the uniform sampler always includes a clip's final frame, and max_frames and max_escalations below 1 are rejected rather than silently uncapping.
ehewes
force-pushed
the
review-and-test-improvements
branch
from
September 16, 2026 15:27
f930e9d to
320a524
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
1 participant
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.
Review pass over the package. Several paths reported an outcome nothing had earned, and
pyframe upload.gif || rejectaccepted the file in each case.--backend aws, bad credentials[error], exit 0[clean], exit 0--prescreen --max-escalations 0max_escalationsis the one worth reading twice:SuspicionSamplertreatsbudget <= 0as keep everything, so the flag documented as a hard cap produced the worst case
available.
--fail-on neverstill exits 0 throughout.Also fixed:
is_nsfwcould read false whileverdictread nsfw, on a short circuited cascade. Thedocs call
is_nsfwauthoritative and the CLI gates on it. Now derived from the verdict.frames[::stride]missed the tail of every clip, so an event running to the end of aGIF had no sampled frame to catch it.
_ensure_min_framescompared screen scores (0 to 1) against motion scores (up to 1e6)in one sort key, so any moving frame outranked one that nearly flagged.
--backend bogusexited with a traceback.Video memory
Scanner.scanheld every decoded frame at full resolution, so the advertised mp4 and mkvsupport OOM'd on anything real. Sampling now runs on per-frame metadata and a second pass
fetches pixels for the selected frames only. On a 900 frame 640x480 clip, peak RSS goes
504 MB to 82 MB (444 MB of growth down to 12 MB). A minute of 1080p was ~11 GB and did not
complete.
Cost is two sequential decodes, three when the cascade escalates. The 3.4s to 6.0s on that
clip is worst case, since a stubbed backend makes it purely decode bound.
Two things not to undo later:
iter_frames_atwalks forward withgrab()and never usesCAP_PROP_POS_FRAMES, which is an approximate keyframe seek on long GOP codecs, VFRcontainers and GIF; and it carries
motion_scorefrom the metadata rather thanrecomputing it, which would otherwise diff across the skipped gap.
Frame,iter_framesand
iter_frames_from_bytesare unchanged, with a smoke test pinning that.max_framesbelow 1 is rejected now too, same reason asmax_escalations.Coverage and CI
The CLI and
iter_frameshad no tests between them, which is why most of this survived.Every assertion for a fix fails against the previous revision. The memory bound is
asserted as peak live
Framecount with a control that fails if the tracker breaks;counting objects is exact where
tracemallocmay not see cv2's buffers andru_maxrssis bytes on macOS and kilobytes on Linux.
Two things worth knowing independently of the fixes:
opencv-python-headless>=4.8has no ceiling, so installs already pull OpenCV 5, and thecv2.VideoCapturepath had no coverage. It is tested now and OpenCV 5 passes.dev = ["ruff"]is unpinned and ruff's defaults have widened since main last ran, somain fails lint on its next CI run. Pinning the rule set fixes that.
Matrix gains 3.14. Version bumped to 0.5.0, minor because default behaviour moved.