From 25880ded91864ffc5c5f9e8cdcc894287ec610b8 Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 23:40:05 +0000 Subject: [PATCH] fix: address 2 code review findings (round 9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - executor: revert person_has_fscores=True back into try/except else branch; moving it outside in round 8 was a regression — when the tracker write fails on the first-ever upload (no prior frigate_scores in tracker), setting the flag True prematurely switches at-cap replacement into fscore mode, get_most_redundant_mapped_file returns None (no entries), and all replacements are silently skipped; the flag must only be set when the score is actually written - diversity: remove dead face_crop-None guard; any face that passes assess_quality (≥90 px MIN_FACE_WIDTH) produces a crop ≥135 px (face + 25% margin), which is always above the 30 px crop minimum, making the guard unreachable; _crop_face_from_thumbnail also calls _get_face_bbox internally, so face_bbox is not None guarantees the inner bbox check also passes --- winnow/diversity.py | 7 ------- winnow/executor.py | 5 +++-- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/winnow/diversity.py b/winnow/diversity.py index 7c308fb..2b55c2e 100644 --- a/winnow/diversity.py +++ b/winnow/diversity.py @@ -299,13 +299,6 @@ def _select_by_embedding( asset["quality_score"] = quality.blur_score face_crop = _crop_face_from_thumbnail(img, asset, person_id=person_id) - if face_crop is None and face_bbox is not None: - logger.warning( - "Face too small to crop for %s — skipping to avoid embedding wrong person", - asset["id"], - ) - quality_filtered += 1 - continue embed_img = face_crop if face_crop is not None else img emb = get_embedding(embed_img, asset_id=asset["id"]) diff --git a/winnow/executor.py b/winnow/executor.py index 65114ad..52b7401 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -534,8 +534,9 @@ def upload_to_frigate(jobs: list[dict]) -> None: " but asset may be re-selected next run: %s", fname, tracker_exc, ) - if pre_fscore is not None: - person_has_fscores = True + else: + if pre_fscore is not None: + person_has_fscores = True # Always record for reconcile so the Frigate filename→asset_id # mapping is created even when the tracker write fails. # Trade-off: if mark_uploaded failed, asset_id is absent from