From 4af9da255011ccd359d403ffd7f06a09c1300c43 Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 21:17:31 +0000 Subject: [PATCH] fix: address 10 code review findings (round 3) - diversity: scale face bbox to thumbnail space before quality check so check_face_size uses actual thumbnail pixels, not original-image coords - diversity: skip asset when face bbox exists but crop guard rejects it, preventing InsightFace from picking the wrong person in a group photo - diversity: add _scale_bbox_to_thumbnail helper (extracted from crop logic) - diversity: use set for medoid membership test in _kmedoids (O(n) not O(n*k)) - diversity: remove dead np.unique in _select_time_spread (linspace produces strictly increasing indices; unique is a no-op and implies wrong semantics) - embeddings: move os.open/os.dup calls inside try in _suppress_output so EMFILE during setup does not leak already-allocated fds - immich_api: count and log assets with missing/unparseable fileCreatedAt in filter_recent_assets instead of silently discarding them - executor: capture pre_run_count before stale-mapping cleanup so the "first run" coaching message doesn't fire after manual file deletion - cli: use p['id'] (KeyError-safe) instead of p.get('id') in fallback path to match all other access sites on the same people list - cache: narrow except to (OSError, ValueError) in EmbeddingCache.get so MemoryError propagates instead of converting OOM to a silent cache miss --- winnow/cache.py | 2 +- winnow/cli.py | 2 +- winnow/diversity.py | 39 ++++++++++++++++++++++++++++++++++++--- winnow/embeddings.py | 23 +++++++++++++---------- winnow/executor.py | 2 +- winnow/immich_api.py | 9 ++++++++- winnow/quality.py | 3 ++- 7 files changed, 62 insertions(+), 18 deletions(-) diff --git a/winnow/cache.py b/winnow/cache.py index 183c3a2..db7c4ab 100644 --- a/winnow/cache.py +++ b/winnow/cache.py @@ -75,7 +75,7 @@ class EmbeddingCache: if os.path.exists(path): try: return np.load(path) - except Exception: + except (OSError, ValueError): return None return None diff --git a/winnow/cli.py b/winnow/cli.py index e6a6549..379be13 100644 --- a/winnow/cli.py +++ b/winnow/cli.py @@ -136,7 +136,7 @@ def _handle_duplicate_people(people: list[dict]) -> list[dict]: "Re-fetch after merge returned no people" " — possible transient error; proceeding with pre-merge list" ) - return [p for p in people if p.get("id") not in skip_ids] + return [p for p in people if p["id"] not in skip_ids] # Filter out the smaller duplicate from any group whose merge failed — those # IDs still exist in Immich and would produce two jobs for the same folder. # IDs from groups that merged successfully are already gone from Immich, so diff --git a/winnow/diversity.py b/winnow/diversity.py index 36ffa47..81fe471 100644 --- a/winnow/diversity.py +++ b/winnow/diversity.py @@ -181,6 +181,28 @@ def _crop_face_from_thumbnail( return crop +def _scale_bbox_to_thumbnail( + bbox: tuple[float, float, float, float], + img: Image.Image, + asset: dict, + person_id: str | None = None, +) -> tuple[float, float, float, float]: + """Scale a face bbox from original detection-image space to thumbnail-pixel space.""" + x1, y1, x2, y2 = bbox + img_w, img_h = img.size + for person in asset.get("people", []): + if person_id and person.get("id") != person_id: + continue + faces = person.get("faces", []) + if faces: + meta_w = faces[0].get("imageWidth") or img_w + meta_h = faces[0].get("imageHeight") or img_h + scale_x, scale_y = img_w / meta_w, img_h / meta_h + return (x1 * scale_x, y1 * scale_y, x2 * scale_x, y2 * scale_y) + break + return bbox + + # ============================================================================= # Embedding Collection # ============================================================================= @@ -258,9 +280,13 @@ def _select_by_embedding( confidence = _get_face_confidence(asset, person_id=person_id) face_bbox = _get_face_bbox(asset, person_id=person_id) + thumbnail_bbox = ( + _scale_bbox_to_thumbnail(face_bbox, img, asset, person_id) + if face_bbox is not None else None + ) quality = assess_quality( img, - face_bbox=face_bbox, + face_bbox=thumbnail_bbox, confidence=confidence, blur_threshold=Config.BLUR_THRESHOLD, min_face_px=Config.MIN_FACE_WIDTH, @@ -273,6 +299,12 @@ 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"], + ) + continue embed_img = face_crop if face_crop is not None else img emb = get_embedding(embed_img, asset_id=asset["id"]) @@ -408,7 +440,8 @@ def _kmedoids(dist_matrix: np.ndarray, k: int, max_iter: int = 50) -> tuple[list for _ in range(max_iter): improved = False # Try swapping each medoid with a random non-medoid - non_medoids = [i for i in range(n) if i not in medoids] + medoid_set = set(medoids) + non_medoids = [i for i in range(n) if i not in medoid_set] if not non_medoids: break @@ -576,4 +609,4 @@ def _select_time_spread(assets: list, limit: int | str) -> list: return assets indices = np.linspace(0, len(assets) - 1, limit, dtype=int) - return [assets[i] for i in np.unique(indices)] + return [assets[i] for i in indices] diff --git a/winnow/embeddings.py b/winnow/embeddings.py index efa154c..d55d23b 100644 --- a/winnow/embeddings.py +++ b/winnow/embeddings.py @@ -26,22 +26,25 @@ logger = logging.getLogger(__name__) @contextmanager def _suppress_output(): """Suppress stdout/stderr at the file-descriptor level, silencing C extension noise.""" - devnull_fd = os.open(os.devnull, os.O_WRONLY) - saved_out, saved_err = os.dup(1), os.dup(2) + devnull_fd = None + saved_out = None + saved_err = None try: + devnull_fd = os.open(os.devnull, os.O_WRONLY) + saved_out = os.dup(1) + saved_err = os.dup(2) os.dup2(devnull_fd, 1) os.dup2(devnull_fd, 2) yield finally: - try: + if saved_out is not None: os.dup2(saved_out, 1) - finally: - try: - os.dup2(saved_err, 2) - finally: - os.close(devnull_fd) - os.close(saved_out) - os.close(saved_err) + os.close(saved_out) + if saved_err is not None: + os.dup2(saved_err, 2) + os.close(saved_err) + if devnull_fd is not None: + os.close(devnull_fd) # Lazy-loaded singleton diff --git a/winnow/executor.py b/winnow/executor.py index ef235b9..a4b0e61 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -327,6 +327,7 @@ def upload_to_frigate(jobs: list[dict]) -> None: # TODO(frigate-api): if Frigate exposes per-file embeddings, compute # diversity against the full training set (tracked + manual) rather than # relying solely on the Frigate score as a proxy signal. + pre_run_count = get_tracked_frigate_file_count(name) _snapshot = ( all_frigate_files.get(name, []) if all_frigate_files is not None else get_frigate_person_files(name) @@ -358,7 +359,6 @@ def upload_to_frigate(jobs: list[dict]) -> None: " (file(s) no longer in Frigate)[/dim]" ) effective_count = get_tracked_frigate_file_count(name) - pre_run_count = effective_count quality_replacement = job.get("config", {}).get("quality_replacement", False) if Config.ENABLE_FRIGATE_SCORES and pre_run_count == 0: progress.console.print( diff --git a/winnow/immich_api.py b/winnow/immich_api.py index 935b62b..fbe0b3a 100644 --- a/winnow/immich_api.py +++ b/winnow/immich_api.py @@ -285,10 +285,11 @@ def filter_recent_assets(assets: list[dict], years: int | None = None) -> list[d logger.debug("Filtering assets older than %s years (%s)", years, cutoff) - recent, skipped = [], 0 + recent, skipped, bad_timestamp = [], 0, 0 for asset in assets: created_at_str = asset.get("fileCreatedAt") if not isinstance(created_at_str, str) or not created_at_str: + bad_timestamp += 1 continue try: @@ -299,8 +300,14 @@ def filter_recent_assets(assets: list[dict], years: int | None = None) -> list[d else: skipped += 1 except ValueError: + bad_timestamp += 1 continue + if bad_timestamp: + logger.warning( + "filter_recent_assets: %s asset(s) had missing or unparseable fileCreatedAt" + " and were excluded from the pool.", bad_timestamp + ) logger.debug("Retained %s assets (filtered %s old assets).", len(recent), skipped) return recent diff --git a/winnow/quality.py b/winnow/quality.py index 1becded..39f58c6 100644 --- a/winnow/quality.py +++ b/winnow/quality.py @@ -155,7 +155,8 @@ def blur_score_from_image(img: Image.Image, max_dim: int = 1440) -> float | None try: score_img = img.convert("RGB") if img.mode != "RGB" else img if score_img.width > max_dim or score_img.height > max_dim: - score_img = score_img.copy() + if score_img is img: + score_img = score_img.copy() score_img.thumbnail((max_dim, max_dim), Image.LANCZOS) return _laplacian_var(np.array(score_img)) except Exception as exc: