From 34fccf883969825682a052cbed148b9a724894fe Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 18:40:13 +0000 Subject: [PATCH 01/14] fix: address 10 full-codebase audit findings + lint Correctness: - jobs: cap auto-diversity limit for brand-new people (was never capped, could exceed MAX_AUTO_IMAGES on first run) - image_processing: separate None/0 guard for imageWidth/imageHeight so missing field is explicit rather than silently aliased to img_w - upload_tracker (_mark, update_frigate_count): copy-before-mutate so exceptions between cache access and _save don't corrupt in-process state - jobs: reject LIMIT=0 on no-embedding path (was silently empty run) - jobs: add STRATEGY=skip to strategy_map so env var is honoured - embeddings: convert to RGB before cvtColor so RGBA/grayscale thumbnails don't raise cv2.error and silently drop from diversity selection - config: use falsy guard for OUTPUT_DIR so blank env var falls through to config file value - reconcile: _ts() returns float("inf") on parse failure so unrecognised filenames sort last instead of collapsing to 0.0 and corrupting FIFO mapping - diversity: remove dead selected_set (never read; -np.inf sentinel already prevents re-selection) Lint (ruff): - executor: sort upload_tracker import block (I001) - executor: replace lambda is_better_than with operator.lt/gt (E731 x2) - executor, upload_tracker: wrap long logger.warning calls (E501 x4) --- winnow/config.py | 2 +- winnow/diversity.py | 2 -- winnow/embeddings.py | 5 +++-- winnow/executor.py | 28 ++++++++++++++++++++-------- winnow/image_processing.py | 11 +++++++---- winnow/jobs.py | 12 ++++++++---- winnow/reconcile.py | 2 +- winnow/upload_tracker.py | 18 +++++++++++++----- 8 files changed, 53 insertions(+), 27 deletions(-) diff --git a/winnow/config.py b/winnow/config.py index 37a99c7..2a33680 100644 --- a/winnow/config.py +++ b/winnow/config.py @@ -173,7 +173,7 @@ class _Config: data = json.loads(config_file.read_text()) if not self.IMMICH_URL: self.IMMICH_URL = data.get("IMMICH_URL") - if os.getenv("OUTPUT_DIR") is None: + if not os.getenv("OUTPUT_DIR"): self.OUTPUT_DIR = data.get("OUTPUT_DIR", self.OUTPUT_DIR) except (json.JSONDecodeError, OSError) as e: logging.warning("Failed to load config file: %s", e) diff --git a/winnow/diversity.py b/winnow/diversity.py index c412743..4e9ada2 100644 --- a/winnow/diversity.py +++ b/winnow/diversity.py @@ -508,7 +508,6 @@ def _cluster_aware_selection( medoid_indices, cluster_labels = _kmedoids(dist_matrix, k) selected = list(medoid_indices) - selected_set = set(selected) logger.debug("Selected %s cluster medoids as initial picks.", len(selected)) @@ -541,7 +540,6 @@ def _cluster_aware_selection( break selected.append(best_idx) - selected_set.add(best_idx) # Update min distances dists_to_new = dist_matrix[best_idx] diff --git a/winnow/embeddings.py b/winnow/embeddings.py index cab4294..efa154c 100644 --- a/winnow/embeddings.py +++ b/winnow/embeddings.py @@ -181,8 +181,9 @@ def get_face_embedding(img_pil: Image.Image) -> np.ndarray | None: return None try: - # InsightFace expects BGR cv2 image - img_bgr = cv2.cvtColor(np.asarray(img_pil), cv2.COLOR_RGB2BGR) + # InsightFace expects BGR cv2 image; normalise mode first so RGBA/grayscale don't + # raise a channel-count error inside cvtColor. + img_bgr = cv2.cvtColor(np.asarray(img_pil.convert("RGB")), cv2.COLOR_RGB2BGR) # Suppress scikit-image FutureWarning from InsightFace's face_align.py with warnings.catch_warnings(): diff --git a/winnow/executor.py b/winnow/executor.py index c5fee48..0290da0 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -1,6 +1,7 @@ """Execution phase: image processing and Frigate upload.""" import logging +import operator import os import shutil from io import BytesIO @@ -27,8 +28,10 @@ from .log_config import console from .quality import blur_score_from_image from .reconcile import enrich_asset_with_face_data, reconcile_frigate_mappings from .upload_tracker import ( - UPLOAD_TRACKER_FILE, REJECT_TRACKER_FILE, + UPLOAD_TRACKER_FILE, + begin_batch, + flush_batch, get_lowest_quality_mapped_file, get_most_redundant_mapped_file, get_tracked_frigate_file_count, @@ -36,8 +39,6 @@ from .upload_tracker import ( has_frigate_scores, mark_rejected, mark_uploaded, - begin_batch, - flush_batch, remove_frigate_file, remove_frigate_files_batch, ) @@ -452,13 +453,13 @@ def upload_to_frigate(jobs: list[dict]) -> None: get_target = get_most_redundant_mapped_file score_label, better_note = "frigate", " (more novel)" no_score_msg = "Frigate recognize unavailable, skipping replacement" - is_better_than = lambda c, t: c < t + is_better_than = operator.lt else: candidate_score = score_map.get(fname) get_target = get_lowest_quality_mapped_file score_label, better_note = "blur", "" no_score_msg = "no quality score, skipping replacement" - is_better_than = lambda c, t: c > t + is_better_than = operator.gt if candidate_score is None: progress.console.print(f" [dim]⏭ {fname}: {no_score_msg}[/dim]") @@ -489,7 +490,10 @@ def upload_to_frigate(jobs: list[dict]) -> None: effective_count -= 1 min_quality_score_for_slot = None if using_fscore else target_score else: - logger.warning("Failed to delete %s for %s, skipping replacement", target_frigate_file, name) + logger.warning( + "Failed to delete %s for %s, skipping replacement", + target_frigate_file, name, + ) failed_deletes.add(target_frigate_file) progress.advance(upload_task) continue @@ -603,11 +607,19 @@ def upload_to_frigate(jobs: list[dict]) -> None: try: flush_batch(UPLOAD_TRACKER_FILE) except Exception as _flush_exc: - logger.warning("flush_batch failed during cleanup — batch will be recovered on next begin_batch: %s", _flush_exc) + logger.warning( + "flush_batch failed during cleanup" + " — batch will be recovered on next begin_batch: %s", + _flush_exc, + ) try: flush_batch(REJECT_TRACKER_FILE) except Exception as _flush_exc: - logger.warning("flush_batch failed during cleanup — batch will be recovered on next begin_batch: %s", _flush_exc) + logger.warning( + "flush_batch failed during cleanup" + " — batch will be recovered on next begin_batch: %s", + _flush_exc, + ) # Batch-map Frigate filenames to asset IDs now that all uploads are done. if actually_uploaded and not _skip_reconcile: diff --git a/winnow/image_processing.py b/winnow/image_processing.py index 7ded632..23c3f0e 100644 --- a/winnow/image_processing.py +++ b/winnow/image_processing.py @@ -92,11 +92,14 @@ def process_face_mode( return None img_w, img_h = img.size - meta_w = face_info.get("imageWidth") or img_w - meta_h = face_info.get("imageHeight") or img_h + meta_w = face_info.get("imageWidth") or 0 + meta_h = face_info.get("imageHeight") or 0 - # Scale bounding box to actual image dimensions - scale_x, scale_y = img_w / meta_w, img_h / meta_h + # Scale bounding box from detection-image space to actual image dimensions. + # Fall back to 1.0 if Immich omits the field — bbox is assumed to already + # be in image space (correct for thumbnails, wrong for full-res). + scale_x = img_w / meta_w if meta_w else 1.0 + scale_y = img_h / meta_h if meta_h else 1.0 x1 = face_info["boundingBoxX1"] * scale_x y1 = face_info["boundingBoxY1"] * scale_y x2 = face_info["boundingBoxX2"] * scale_x diff --git a/winnow/jobs.py b/winnow/jobs.py index b8243c7..7344572 100644 --- a/winnow/jobs.py +++ b/winnow/jobs.py @@ -66,7 +66,11 @@ def _get_strategy_choice(has_embedding: bool) -> tuple[int | str, str]: def _resolve_strategy(strategy: str, has_embedding: bool) -> tuple[int | str, str]: """Resolve env var strategy to (limit, selection_mode) without prompts.""" if not has_embedding: - return _getenv_int("LIMIT", 30), "time" + limit = _getenv_int("LIMIT", 30) + if limit <= 0: + logger.warning("LIMIT=%s is invalid — ignoring and using default 30", limit) + limit = 30 + return limit, "time" custom_limit = _getenv_optional_int("LIMIT") if custom_limit is not None: @@ -77,6 +81,7 @@ def _resolve_strategy(strategy: str, has_embedding: bool) -> tuple[int | str, st strategy_map = { "adaptive": ("auto", "smart"), "auto": ("auto", "smart"), # legacy alias for adaptive + "skip": (0, "skip"), "standard": (30, "smart"), "broad": (100, "smart"), } @@ -297,9 +302,8 @@ def auto_configure(people: list[dict]) -> list[dict]: if limit == "auto": # Switch from open-ended auto to a fixed budget at remaining capacity # so the diversity selector itself stops at the right count instead of - # selecting MAX_AUTO_IMAGES and then discarding the excess by position. - if already_uploaded > 0: - limit = capacity + # selecting more than MAX_AUTO_IMAGES and overflowing the cap. + limit = capacity else: limit = min(limit, capacity) diff --git a/winnow/reconcile.py b/winnow/reconcile.py index 2a4f251..c012f87 100644 --- a/winnow/reconcile.py +++ b/winnow/reconcile.py @@ -61,7 +61,7 @@ def reconcile_frigate_mappings( try: return float(fname.rsplit("_", 1)[-1].rsplit(".", 1)[0]) except (ValueError, IndexError): - return 0.0 + return float("inf") logger.debug( "%s: mapping %s file(s) by filename timestamp — assumes Frigate processes" diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index 3b56678..b5b90fc 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -109,7 +109,11 @@ def begin_batch(filename: str) -> None: try: _write_to_disk(path, _cache[key]) except Exception: - logger.warning("begin_batch: could not flush leftover deferred state for %s — partial progress may be lost", path) + logger.warning( + "begin_batch: could not flush leftover deferred state for %s" + " — partial progress may be lost", + path, + ) _deferred.discard(key) _dirty.discard(key) _deferred.add(key) @@ -162,7 +166,7 @@ def _mark( logger.warning("_mark called with empty person_name for asset %s — asset not recorded", asset_id) return data = _load(filename) - by_person = data.setdefault("by_person", {}) + by_person = dict(data.get("by_person", {})) entry = _migrate_entry(by_person.get(person_name, {})) ids = set(entry["asset_ids"]) ids.add(asset_id) @@ -174,7 +178,9 @@ def _mark( if frigate_score is not None: entry["frigate_scores"][asset_id] = round(frigate_score, 4) by_person[person_name] = entry - _save(filename, data) + new_data = dict(data) + new_data["by_person"] = by_person + _save(filename, new_data) logger.debug("Marked %s in %s (%s)", asset_id, filename, person_name) @@ -371,11 +377,13 @@ def find_by_crop_dimension(size: int) -> list[dict]: def update_frigate_count(person_name: str, count: int) -> None: """Record Frigate's authoritative training image count for a person.""" data = _load(UPLOAD_TRACKER_FILE) - by_person = data.setdefault("by_person", {}) + by_person = dict(data.get("by_person", {})) entry = _migrate_entry(by_person.get(person_name, {})) entry["frigate_count"] = count by_person[person_name] = entry - _save(UPLOAD_TRACKER_FILE, data) + new_data = dict(data) + new_data["by_person"] = by_person + _save(UPLOAD_TRACKER_FILE, new_data) def reset_all_people() -> None: From 8bdce9253a723fddac99ebc59211204a453caca1 Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 20:51:14 +0000 Subject: [PATCH 02/14] =?UTF-8?q?fix:=20address=2010=20codebase=20audit=20?= =?UTF-8?q?findings=20=E2=80=94=20API=20guards,=20reconcile,=20merge=20fal?= =?UTF-8?q?lback,=20tracker=20guards?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - immich_api: guard resp.json() with isinstance(dict) check in get_people and fetch_all_assets so AttributeError doesn't escape on proxy/CDN non-dict responses - executor: move actually_uploaded.append outside try/else so Frigate filename→asset_id mapping is created via reconcile even when the tracker write fails - cli: fall back to pre-merge people list when re-fetch after merge returns empty (transient error) instead of silently dropping all people - cli: treat ENABLE_FRIGATE_SCORES=false / BLUR_THRESHOLD=0 as not-set in the unsupported-vars warning (falsy string check replaces raw truthiness) - upload_tracker: guard set(data[flat_key]) with isinstance(list) check in reset_person so a corrupted non-iterable legacy field doesn't crash mid-reset - upload_tracker: guard dims[0]/dims[1] in find_by_crop_dimension with a length check so a truncated crop_dims entry doesn't raise IndexError - cache: wrap os.remove() in clear() with try/except OSError to handle TOCTOU race with concurrent put() calls - diversity: default conf_array to 0.5 (was 1.0) for faces with missing confidence so they receive a moderate diversity boost instead of being treated as high-confidence - diversity: sort assets in the fast path (len <= limit) so return order is consistent with the sorted-by-fileCreatedAt path --- winnow/cache.py | 7 +++++-- winnow/cli.py | 9 ++++++++- winnow/diversity.py | 10 ++++++---- winnow/executor.py | 4 +++- winnow/immich_api.py | 14 +++++++++++--- winnow/upload_tracker.py | 4 +++- 6 files changed, 36 insertions(+), 12 deletions(-) diff --git a/winnow/cache.py b/winnow/cache.py index 6ce3126..183c3a2 100644 --- a/winnow/cache.py +++ b/winnow/cache.py @@ -103,8 +103,11 @@ class EmbeddingCache: count = 0 for f in os.listdir(self.cache_dir): if f.endswith(".npy"): - os.remove(os.path.join(self.cache_dir, f)) - count += 1 + try: + os.remove(os.path.join(self.cache_dir, f)) + count += 1 + except OSError: + pass logger.info("Cleared %s cached embeddings.", count) diff --git a/winnow/cli.py b/winnow/cli.py index 2ce8d60..e6a6549 100644 --- a/winnow/cli.py +++ b/winnow/cli.py @@ -131,6 +131,12 @@ def _handle_duplicate_people(people: list[dict]) -> list[dict]: if merged_any: rprint(" [dim]Re-fetching people after merge...[/dim]") fresh = get_people() + if not fresh: + logger.warning( + "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] # 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 @@ -173,7 +179,8 @@ def main() -> None: [dim]Immich -> Frigate Training Data Curator[/dim] """) - set_unsupported = [v for v in _UNSUPPORTED_VARS if os.environ.get(v)] + _FALSY = {"", "false", "0", "no", "off"} + set_unsupported = [v for v in _UNSUPPORTED_VARS if os.environ.get(v, "").strip().lower() not in _FALSY] if set_unsupported: console.print( f"[bold yellow]⚠ Advanced tuning vars set: " diff --git a/winnow/diversity.py b/winnow/diversity.py index 4e9ada2..36ffa47 100644 --- a/winnow/diversity.py +++ b/winnow/diversity.py @@ -57,9 +57,9 @@ def select_diverse_assets( Returns: List of selected assets """ - # Fast path: fewer assets than limit + # Fast path: fewer assets than limit — sort for consistent ordering with other paths if limit != "auto" and len(assets) <= limit: - return assets + return sorted(assets, key=lambda x: x.get("fileCreatedAt", "")) # Sort by creation time assets = sorted(assets, key=lambda x: x.get("fileCreatedAt", "")) @@ -483,8 +483,10 @@ def _cluster_aware_selection( norms = np.linalg.norm(emb_matrix, axis=1, keepdims=True) emb_normed = emb_matrix / np.maximum(norms, 1e-8) - # Build confidence weight array for hard example boosting - conf_array = np.ones(n) + # Build confidence weight array for hard example boosting. + # Default to 0.5 for faces with no confidence score so they receive a + # moderate diversity boost rather than being treated as high-confidence. + conf_array = np.full(n, 0.5) if confidence_scores: for i, c in enumerate(confidence_scores): if c is not None: diff --git a/winnow/executor.py b/winnow/executor.py index 0290da0..ef235b9 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -533,7 +533,9 @@ def upload_to_frigate(jobs: list[dict]) -> None: else: if pre_fscore is not None: person_has_fscores = True - actually_uploaded.append((fname, asset_id)) + # Always record for reconcile so the Frigate filename→asset_id + # mapping is created even when the tracker write fails. + actually_uploaded.append((fname, asset_id)) break else: diff --git a/winnow/immich_api.py b/winnow/immich_api.py index 3bde08b..935b62b 100644 --- a/winnow/immich_api.py +++ b/winnow/immich_api.py @@ -57,8 +57,12 @@ def get_people() -> list[dict]: logger.error("Immich API key is invalid or expired (401 Unauthorized). Update API_KEY.") return [] resp.raise_for_status() - return resp.json().get("people", []) - except (requests.RequestException, ValueError) as e: + data = resp.json() + if not isinstance(data, dict): + logger.error("Unexpected response shape from Immich /people: %r", type(data)) + return [] + return data.get("people", []) + except (requests.RequestException, ValueError, AttributeError) as e: logger.error("Failed to fetch people from Immich: %s", e) return [] @@ -118,7 +122,11 @@ def fetch_all_assets(person: dict) -> tuple[list[dict], int]: logger.error("Error fetching assets for %s (page %s): %s", name, page, resp.status_code) break - page_assets = resp.json().get("assets", []) + body = resp.json() + if not isinstance(body, dict): + logger.error("Unexpected response shape fetching assets for %s (page %s): %r", name, page, type(body)) + break + page_assets = body.get("assets", []) # Immich ≥2.x returns {"assets": {"items": [...]}}; # earlier versions returned {"assets": [...]} directly. if isinstance(page_assets, dict): diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index b5b90fc..859b78e 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -360,6 +360,8 @@ def find_by_crop_dimension(size: int) -> list[dict]: asset_to_frigate.setdefault(aid, fn) # first-seen wins; plain inversion silently drops duplicates frigate_scores = entry.get("frigate_scores", {}) for asset_id, dims in entry.get("crop_dims", {}).items(): + if not isinstance(dims, (list, tuple)) or len(dims) < 2: + continue w, h = dims[0], dims[1] if w == size or h == size: results.append({ @@ -440,7 +442,7 @@ def reset_person(person_name: str) -> None: data["by_person"] = by_person flat_key = _flat_key(filename) person_ids = set(_get_ids(tracker_entry)) - if person_ids and flat_key in data: + if person_ids and flat_key in data and isinstance(data[flat_key], list): data[flat_key] = sorted(set(data[flat_key]) - person_ids) _save(filename, data) changed = True From 4af9da255011ccd359d403ffd7f06a09c1300c43 Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 21:17:31 +0000 Subject: [PATCH 03/14] 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: From 96099ed6e2730af0be2d7668a30000ec5250c324 Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 21:43:24 +0000 Subject: [PATCH 04/14] fix: address 8 code review findings (round 4) - diversity: remove erroneous break outside if-faces in _scale_bbox_to_thumbnail (broke people-loop early for first unannotated person, defeating scale fix) - diversity: increment quality_filtered for face-too-small crop skips so the summary log counts them alongside assess_quality failures - diversity: fix hard-example log count to use original confidence_scores[i] instead of synthetic conf_array default (0.5), eliminating false 100% hard-example reports for persons with no Immich confidence data - cache: add EOFError to except tuple in EmbeddingCache.get() so truncated .npy files return None instead of crashing the embedding pipeline - embeddings: wrap each os.dup2 restore in its own try/except OSError in _suppress_output finally block so stderr is always restored even if the stdout restore raises - executor: gate recognize_face on effective_count > 0 (post-stale-cleanup) instead of pre_run_count > 0 so recognize_face is not called against an untrained Frigate model after the user manually deletes all training files - executor: document actually_uploaded trade-off in comment (appending unconditionally on tracker failure risks a Frigate duplicate but prevents permanent filename unmapping which breaks quality-replacement scoring) - jobs: check strategy == "skip" before the has_embedding and custom_limit early-returns in _resolve_strategy so STRATEGY=skip is always honoured --- winnow/cache.py | 2 +- winnow/diversity.py | 11 ++++++++--- winnow/embeddings.py | 10 ++++++++-- winnow/executor.py | 13 +++++++++---- winnow/jobs.py | 2 ++ 5 files changed, 28 insertions(+), 10 deletions(-) diff --git a/winnow/cache.py b/winnow/cache.py index db7c4ab..810fe30 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 (OSError, ValueError): + except (OSError, ValueError, EOFError): return None return None diff --git a/winnow/diversity.py b/winnow/diversity.py index 81fe471..5f85f7a 100644 --- a/winnow/diversity.py +++ b/winnow/diversity.py @@ -199,7 +199,6 @@ def _scale_bbox_to_thumbnail( 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 @@ -304,6 +303,7 @@ def _select_by_embedding( "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 @@ -581,8 +581,13 @@ def _cluster_aware_selection( min_dists = np.minimum(min_dists, dists_to_new) min_dists[best_idx] = -np.inf - selected_conf = [conf_array[i] for i in selected if conf_array[i] < 1.0] - hard_count = sum(1 for c in selected_conf if c < 0.85) + hard_count = sum( + 1 for i in selected + if confidence_scores + and i < len(confidence_scores) + and confidence_scores[i] is not None + and confidence_scores[i] < 0.85 + ) logger.info("Selection complete: %s images (%s hard examples with confidence < 0.85).", len(selected), hard_count) # Slice to target: the while loop enforces this for non-auto mode, but diff --git a/winnow/embeddings.py b/winnow/embeddings.py index d55d23b..112c913 100644 --- a/winnow/embeddings.py +++ b/winnow/embeddings.py @@ -38,10 +38,16 @@ def _suppress_output(): yield finally: if saved_out is not None: - os.dup2(saved_out, 1) + try: + os.dup2(saved_out, 1) + except OSError: + pass os.close(saved_out) if saved_err is not None: - os.dup2(saved_err, 2) + try: + os.dup2(saved_err, 2) + except OSError: + pass os.close(saved_err) if devnull_fd is not None: os.close(devnull_fd) diff --git a/winnow/executor.py b/winnow/executor.py index a4b0e61..97e384b 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -393,7 +393,7 @@ def upload_to_frigate(jobs: list[dict]) -> None: # Pre-upload Frigate score — clean measurement (image not yet in training set). # Called for all below-cap uploads (seeds frigate_scores for future at-cap # replacement) and for at-cap uploads when scores already exist. Skipped on - # the first run (pre_run_count == 0) since Frigate has no model yet. + # skipped when effective_count == 0 since Frigate has no model yet. # recognize_face returns (face_name, score); we only use the score when the # best match is for the correct person. Mismatches (or "unknown") are treated # as None so a wrong-person score never drives a ceiling skip or replacement. @@ -409,7 +409,7 @@ def upload_to_frigate(jobs: list[dict]) -> None: # rebuild-complete signal, poll it between recognize calls during replacement # sequences rather than accepting stale/None scores. pre_fscore: float | None = None - if Config.ENABLE_FRIGATE_SCORES and pre_run_count > 0: + if Config.ENABLE_FRIGATE_SCORES and effective_count > 0: if not at_cap or person_has_fscores: _result = recognize_face(fpath) if _result is not None and (_result[0] or "").casefold() == name.casefold(): @@ -417,8 +417,8 @@ def upload_to_frigate(jobs: list[dict]) -> None: # Below-cap novelty gate: skip candidates already covered by the Frigate model, # including conditions learned from manually-added images winnow can't track. - # pre_fscore is None on the first run (pre_run_count == 0 skips recognize_face - # above), so this block never fires on the first run without an extra guard. + # pre_fscore is None when effective_count == 0 (no Frigate model yet), + # so this block never fires on the first run without an extra guard. if not at_cap and pre_fscore is not None: _ceiling = Config.FRIGATE_SCORE_CEILING if _ceiling is None: @@ -535,6 +535,11 @@ def upload_to_frigate(jobs: list[dict]) -> 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, this asset_id is not in + # the upload set, so it may be re-selected next run (Frigate + # duplicate). The alternative — not appending — leaves the file + # permanently unmapped, breaking quality-replacement scoring. + # Frigate duplicate is the lesser consequence. actually_uploaded.append((fname, asset_id)) break diff --git a/winnow/jobs.py b/winnow/jobs.py index 7344572..cf0e85e 100644 --- a/winnow/jobs.py +++ b/winnow/jobs.py @@ -65,6 +65,8 @@ def _get_strategy_choice(has_embedding: bool) -> tuple[int | str, str]: def _resolve_strategy(strategy: str, has_embedding: bool) -> tuple[int | str, str]: """Resolve env var strategy to (limit, selection_mode) without prompts.""" + if strategy == "skip": + return 0, "skip" if not has_embedding: limit = _getenv_int("LIMIT", 30) if limit <= 0: From 2cb126a5890cb0821ca4bbd7fdeeb86b8d32c908 Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 22:03:59 +0000 Subject: [PATCH 05/14] fix: address 5 code review findings (round 5) - embeddings: move os.close into try/finally so saved_out/saved_err are always closed even when os.dup2 restore raises, preventing fd leak - cache: replace narrow except tuple with except MemoryError: raise / except Exception: return None so struct.error and other np.load failures return None without masking OOM - executor: fix first-run advisory message to check effective_count == 0 (post-stale-cleanup) instead of pre_run_count; remove now-unused pre_run_count variable entirely - jobs: remove dead "skip" entry from strategy_map (unreachable since the early-return at the top of _resolve_strategy fires first) - upload_tracker: log a warning when reset_person encounters a non-list flat_key value instead of silently skipping the cleanup --- winnow/cache.py | 4 +++- winnow/embeddings.py | 6 ++++-- winnow/executor.py | 3 +-- winnow/jobs.py | 1 - winnow/upload_tracker.py | 7 ++++++- 5 files changed, 14 insertions(+), 7 deletions(-) diff --git a/winnow/cache.py b/winnow/cache.py index 810fe30..b28f14a 100644 --- a/winnow/cache.py +++ b/winnow/cache.py @@ -75,7 +75,9 @@ class EmbeddingCache: if os.path.exists(path): try: return np.load(path) - except (OSError, ValueError, EOFError): + except MemoryError: + raise + except Exception: return None return None diff --git a/winnow/embeddings.py b/winnow/embeddings.py index 112c913..53f254e 100644 --- a/winnow/embeddings.py +++ b/winnow/embeddings.py @@ -42,13 +42,15 @@ def _suppress_output(): os.dup2(saved_out, 1) except OSError: pass - os.close(saved_out) + finally: + os.close(saved_out) if saved_err is not None: try: os.dup2(saved_err, 2) except OSError: pass - os.close(saved_err) + finally: + os.close(saved_err) if devnull_fd is not None: os.close(devnull_fd) diff --git a/winnow/executor.py b/winnow/executor.py index 97e384b..f2d138e 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -327,7 +327,6 @@ 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) @@ -360,7 +359,7 @@ def upload_to_frigate(jobs: list[dict]) -> None: ) effective_count = get_tracked_frigate_file_count(name) quality_replacement = job.get("config", {}).get("quality_replacement", False) - if Config.ENABLE_FRIGATE_SCORES and pre_run_count == 0: + if Config.ENABLE_FRIGATE_SCORES and effective_count == 0: progress.console.print( f" [dim]{name}: first run — Frigate diversity scoring will apply from the next run[/dim]" ) diff --git a/winnow/jobs.py b/winnow/jobs.py index cf0e85e..2145879 100644 --- a/winnow/jobs.py +++ b/winnow/jobs.py @@ -83,7 +83,6 @@ def _resolve_strategy(strategy: str, has_embedding: bool) -> tuple[int | str, st strategy_map = { "adaptive": ("auto", "smart"), "auto": ("auto", "smart"), # legacy alias for adaptive - "skip": (0, "skip"), "standard": (30, "smart"), "broad": (100, "smart"), } diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index 859b78e..4e29abf 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -442,7 +442,12 @@ def reset_person(person_name: str) -> None: data["by_person"] = by_person flat_key = _flat_key(filename) person_ids = set(_get_ids(tracker_entry)) - if person_ids and flat_key in data and isinstance(data[flat_key], list): + if flat_key in data and not isinstance(data[flat_key], list): + logger.warning( + "reset_person: %s has unexpected type for %s (%s) — skipping flat-list cleanup", + filename, flat_key, type(data[flat_key]).__name__, + ) + elif person_ids and flat_key in data: data[flat_key] = sorted(set(data[flat_key]) - person_ids) _save(filename, data) changed = True From 692d77ee9f28f2037c1c368446ec5595f551dfc1 Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 22:22:51 +0000 Subject: [PATCH 06/14] fix: address 4 code review findings (round 6) - executor: snapshot has_frigate_model = effective_count > 0 before the upload loop; use it in the recognize_face gate instead of the live effective_count, which is incremented mid-loop and would otherwise trigger recognize_face calls against an empty Frigate model on first run - jobs: restore if already_uploaded > 0 guard before limit = capacity so first-run auto-strategy jobs keep limit="auto" and the FPS adaptive early-stop can fire instead of always filling MAX_AUTO_IMAGES slots - cli: retry get_people() once after a post-merge empty response before falling back to the pre-merge list; improve warning to name expired API key as a possible cause alongside transient network errors - diversity: hoist hard_weight = np.where(...) above the FPS while loop since conf_array is constant; eliminates one O(n) numpy pass per selected image --- winnow/cli.py | 9 +++++++-- winnow/diversity.py | 7 ++++--- winnow/executor.py | 9 +++++++-- winnow/jobs.py | 10 ++++++---- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/winnow/cli.py b/winnow/cli.py index 379be13..55391a0 100644 --- a/winnow/cli.py +++ b/winnow/cli.py @@ -131,10 +131,15 @@ def _handle_duplicate_people(people: list[dict]) -> list[dict]: if merged_any: rprint(" [dim]Re-fetching people after merge...[/dim]") fresh = get_people() + if not fresh: + # Retry once: get_people() returns [] for both transient failures and + # auth errors (401); a second empty result strongly suggests a real failure. + fresh = get_people() if not fresh: logger.warning( - "Re-fetch after merge returned no people" - " — possible transient error; proceeding with pre-merge list" + "Re-fetch after merge returned no people (tried twice)" + " — possible transient error or expired API key;" + " proceeding with pre-merge list. Check IMMICH_API_KEY if this recurs." ) 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 diff --git a/winnow/diversity.py b/winnow/diversity.py index 5f85f7a..3c6ad8d 100644 --- a/winnow/diversity.py +++ b/winnow/diversity.py @@ -556,10 +556,11 @@ def _cluster_aware_selection( for idx in selected: min_dists[idx] = -np.inf + # Hard example weighting: boost distance for low-confidence candidates. + # conf_array is constant after this point, so compute once outside the loop. + hard_weight = np.where(conf_array < 0.85, 1.0 + (0.85 - conf_array) * 2.0, 1.0) + while len(selected) < target: - # Hard example weighting: boost distance for low-confidence candidates - # Confidence < 0.85 gets up to 1.5× distance boost - hard_weight = np.where(conf_array < 0.85, 1.0 + (0.85 - conf_array) * 2.0, 1.0) weighted_dists = min_dists * hard_weight best_idx = int(np.argmax(weighted_dists)) diff --git a/winnow/executor.py b/winnow/executor.py index f2d138e..07b72b6 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -363,6 +363,11 @@ def upload_to_frigate(jobs: list[dict]) -> None: progress.console.print( f" [dim]{name}: first run — Frigate diversity scoring will apply from the next run[/dim]" ) + # Snapshot whether Frigate has a model before the upload loop starts. + # effective_count is incremented inside the loop on each successful upload, + # so using the live value would incorrectly trigger recognize_face calls + # mid-batch on the first run (after the first upload sets it to 1). + has_frigate_model = effective_count > 0 actually_uploaded: list[tuple[str, str | None]] = [] failed_deletes: set[str] = set() min_quality_score_for_slot: float | None = None @@ -392,7 +397,7 @@ def upload_to_frigate(jobs: list[dict]) -> None: # Pre-upload Frigate score — clean measurement (image not yet in training set). # Called for all below-cap uploads (seeds frigate_scores for future at-cap # replacement) and for at-cap uploads when scores already exist. Skipped on - # skipped when effective_count == 0 since Frigate has no model yet. + # skipped when has_frigate_model is False (effective_count was 0 before the loop). # recognize_face returns (face_name, score); we only use the score when the # best match is for the correct person. Mismatches (or "unknown") are treated # as None so a wrong-person score never drives a ceiling skip or replacement. @@ -408,7 +413,7 @@ def upload_to_frigate(jobs: list[dict]) -> None: # rebuild-complete signal, poll it between recognize calls during replacement # sequences rather than accepting stale/None scores. pre_fscore: float | None = None - if Config.ENABLE_FRIGATE_SCORES and effective_count > 0: + if Config.ENABLE_FRIGATE_SCORES and has_frigate_model: if not at_cap or person_has_fscores: _result = recognize_face(fpath) if _result is not None and (_result[0] or "").casefold() == name.casefold(): diff --git a/winnow/jobs.py b/winnow/jobs.py index 2145879..396fc7a 100644 --- a/winnow/jobs.py +++ b/winnow/jobs.py @@ -301,10 +301,12 @@ def auto_configure(people: list[dict]) -> list[dict]: # decides per-image whether to swap; any candidate could be an improvement). if not quality_replacement_only: if limit == "auto": - # Switch from open-ended auto to a fixed budget at remaining capacity - # so the diversity selector itself stops at the right count instead of - # selecting more than MAX_AUTO_IMAGES and overflowing the cap. - limit = capacity + if already_uploaded > 0: + # Switch from open-ended auto to a fixed budget at remaining capacity + # so the diversity selector stops at the right count instead of + # selecting more than MAX_AUTO_IMAGES and overflowing the cap. + # First runs keep limit="auto" so FPS adaptive early-stop can fire. + limit = capacity else: limit = min(limit, capacity) From 7282c76b6836002b011a97c1e74a92ab18b8a978 Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 23:09:14 +0000 Subject: [PATCH 07/14] fix: address 5 code review findings (round 7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - diversity: revert conf_array default from 0.5 back to 1.0 (np.ones); the 0.5 default caused None-confidence images to receive a 1.7× FPS boost and beat high-confidence detections — counter-productive for Frigate training data quality - diversity: fix hard_count to include None-confidence images (count images where score is None or < 0.85, not only confirmed < 0.85); the previous check systematically undercounted boosted images when the Immich faces API omits the score field - executor: fix garbled comment fragment "Skipped on / skipped when" left by a partial edit in round 4; merge into a single coherent sentence - executor: expand actually_uploaded trade-off comment to document all three consequences of a tracker write failure (Frigate duplicate, quality-replacement exclusion, cap-slot consumption), not only the duplicate risk mentioned previously - upload_tracker: add person_ids guard to reset_person isinstance check so the non-list warning only fires when cleanup would actually have run, not on no-op calls where person_ids is empty --- winnow/diversity.py | 10 +++++----- winnow/executor.py | 17 ++++++++++------- winnow/upload_tracker.py | 2 +- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/winnow/diversity.py b/winnow/diversity.py index 3c6ad8d..6961f56 100644 --- a/winnow/diversity.py +++ b/winnow/diversity.py @@ -517,9 +517,10 @@ def _cluster_aware_selection( emb_normed = emb_matrix / np.maximum(norms, 1e-8) # Build confidence weight array for hard example boosting. - # Default to 0.5 for faces with no confidence score so they receive a - # moderate diversity boost rather than being treated as high-confidence. - conf_array = np.full(n, 0.5) + # Default to 1.0 for faces with no confidence score: treat as high-confidence + # (no boost) rather than hard-example territory. A missing score field should + # not cause these images to beat genuinely high-confidence detections in FPS. + conf_array = np.ones(n) if confidence_scores: for i, c in enumerate(confidence_scores): if c is not None: @@ -586,8 +587,7 @@ def _cluster_aware_selection( 1 for i in selected if confidence_scores and i < len(confidence_scores) - and confidence_scores[i] is not None - and confidence_scores[i] < 0.85 + and (confidence_scores[i] is None or confidence_scores[i] < 0.85) ) logger.info("Selection complete: %s images (%s hard examples with confidence < 0.85).", len(selected), hard_count) diff --git a/winnow/executor.py b/winnow/executor.py index 07b72b6..52b7401 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -396,8 +396,8 @@ def upload_to_frigate(jobs: list[dict]) -> None: # Pre-upload Frigate score — clean measurement (image not yet in training set). # Called for all below-cap uploads (seeds frigate_scores for future at-cap - # replacement) and for at-cap uploads when scores already exist. Skipped on - # skipped when has_frigate_model is False (effective_count was 0 before the loop). + # replacement) and for at-cap uploads when scores already exist. + # Skipped when has_frigate_model is False (effective_count was 0 before the loop). # recognize_face returns (face_name, score); we only use the score when the # best match is for the correct person. Mismatches (or "unknown") are treated # as None so a wrong-person score never drives a ceiling skip or replacement. @@ -539,11 +539,14 @@ def upload_to_frigate(jobs: list[dict]) -> 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, this asset_id is not in - # the upload set, so it may be re-selected next run (Frigate - # duplicate). The alternative — not appending — leaves the file - # permanently unmapped, breaking quality-replacement scoring. - # Frigate duplicate is the lesser consequence. + # Trade-off: if mark_uploaded failed, asset_id is absent from + # asset_ids and scores. Consequences: (1) re-selected next run + # → Frigate duplicate; (2) excluded from quality-replacement + # candidates (_pick_mapped_file requires a scores entry); + # (3) counted toward MAX_AUTO_IMAGES cap (via frigate_files). + # The alternative — not appending — leaves the file permanently + # unmapped (reconcile never creates the frigate_files entry), + # making (2) and (3) permanent. Frigate duplicate is lesser. actually_uploaded.append((fname, asset_id)) break diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index 4e29abf..6997b6a 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -442,7 +442,7 @@ def reset_person(person_name: str) -> None: data["by_person"] = by_person flat_key = _flat_key(filename) person_ids = set(_get_ids(tracker_entry)) - if flat_key in data and not isinstance(data[flat_key], list): + if person_ids and flat_key in data and not isinstance(data[flat_key], list): logger.warning( "reset_person: %s has unexpected type for %s (%s) — skipping flat-list cleanup", filename, flat_key, type(data[flat_key]).__name__, From 461ceb7af46c1b2e334c8a56709629a809a57b24 Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 23:24:11 +0000 Subject: [PATCH 08/14] fix: address 5 code review findings (round 8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - diversity: fix hard_count regression from round 7 — revert to 'is not None and < 0.85' so only images that actually receive a FPS boost (confirmed low confidence) are counted as hard examples; None-confidence images use conf_array=1.0 (no boost) and should not appear in the hard-example log count - diversity: fix _scale_bbox_to_thumbnail to use explicit zero-guard for imageWidth/imageHeight (meta_w or 0; scale = img_w/meta_w if meta_w else 1.0) — mirrors image_processing.py pattern; prevents `or img_w` from silently treating imageWidth=0 as missing and returning scale=1.0 without surfacing the zero-metadata case - embeddings: wrap all three os.close calls in _suppress_output finally block with try/except OSError: pass so a failed close in one branch cannot abort the outer finally and leak devnull_fd or the saved_err/saved_out fds - upload_tracker: clear corrupt flat-list key (data[flat_key] = []) after the isinstance warning instead of leaving the corrupt value in place — prevents stale IDs persisting across reset_person calls and future load_uploaded_ids() from seeing a non-list value - executor: move 'if pre_fscore is not None: person_has_fscores = True' out of the try/except else branch so it fires even when mark_uploaded raises; Frigate scores exist once measured regardless of tracker write success, and replacement strategy should reflect that --- winnow/diversity.py | 10 ++++++---- winnow/embeddings.py | 15 ++++++++++++--- winnow/executor.py | 5 ++--- winnow/upload_tracker.py | 3 ++- 4 files changed, 22 insertions(+), 11 deletions(-) diff --git a/winnow/diversity.py b/winnow/diversity.py index 6961f56..7c308fb 100644 --- a/winnow/diversity.py +++ b/winnow/diversity.py @@ -195,9 +195,10 @@ def _scale_bbox_to_thumbnail( 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 + meta_w = faces[0].get("imageWidth") or 0 + meta_h = faces[0].get("imageHeight") or 0 + scale_x = img_w / meta_w if meta_w else 1.0 + scale_y = img_h / meta_h if meta_h else 1.0 return (x1 * scale_x, y1 * scale_y, x2 * scale_x, y2 * scale_y) return bbox @@ -587,7 +588,8 @@ def _cluster_aware_selection( 1 for i in selected if confidence_scores and i < len(confidence_scores) - and (confidence_scores[i] is None or confidence_scores[i] < 0.85) + and confidence_scores[i] is not None + and confidence_scores[i] < 0.85 ) logger.info("Selection complete: %s images (%s hard examples with confidence < 0.85).", len(selected), hard_count) diff --git a/winnow/embeddings.py b/winnow/embeddings.py index 53f254e..1e84ed4 100644 --- a/winnow/embeddings.py +++ b/winnow/embeddings.py @@ -43,16 +43,25 @@ def _suppress_output(): except OSError: pass finally: - os.close(saved_out) + try: + os.close(saved_out) + except OSError: + pass if saved_err is not None: try: os.dup2(saved_err, 2) except OSError: pass finally: - os.close(saved_err) + try: + os.close(saved_err) + except OSError: + pass if devnull_fd is not None: - os.close(devnull_fd) + try: + os.close(devnull_fd) + except OSError: + pass # Lazy-loaded singleton diff --git a/winnow/executor.py b/winnow/executor.py index 52b7401..65114ad 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -534,9 +534,8 @@ def upload_to_frigate(jobs: list[dict]) -> None: " but asset may be re-selected next run: %s", fname, tracker_exc, ) - else: - if pre_fscore is not None: - person_has_fscores = True + 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 diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index 6997b6a..39e57a3 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -444,9 +444,10 @@ def reset_person(person_name: str) -> None: person_ids = set(_get_ids(tracker_entry)) if person_ids and flat_key in data and not isinstance(data[flat_key], list): logger.warning( - "reset_person: %s has unexpected type for %s (%s) — skipping flat-list cleanup", + "reset_person: %s has unexpected type for %s (%s) — clearing corrupt flat-list", filename, flat_key, type(data[flat_key]).__name__, ) + data[flat_key] = [] elif person_ids and flat_key in data: data[flat_key] = sorted(set(data[flat_key]) - person_ids) _save(filename, data) From 25880ded91864ffc5c5f9e8cdcc894287ec610b8 Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 23:40:05 +0000 Subject: [PATCH 09/14] 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 From 34f7985357ca57f6569b7146926dee9d8446541c Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 23:41:38 +0000 Subject: [PATCH 10/14] docs: document BaseException limitation in _suppress_output finally block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A KeyboardInterrupt raised inside the saved_out cleanup block would propagate past the saved_err and devnull_fd blocks, leaking those fds. In CPython this race is not realistically triggerable — KI is delivered between bytecodes and os.dup2 is a single atomic C syscall — so we accept the theoretical risk rather than silencing BaseException in a finally block. --- winnow/embeddings.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/winnow/embeddings.py b/winnow/embeddings.py index 1e84ed4..9d59d8a 100644 --- a/winnow/embeddings.py +++ b/winnow/embeddings.py @@ -37,6 +37,11 @@ def _suppress_output(): os.dup2(devnull_fd, 2) yield finally: + # Each block is a separate sequential statement. A BaseException (e.g. + # KeyboardInterrupt) raised inside block N would propagate past blocks N+1 + # and N+2, leaving saved_err or devnull_fd unclosed. In CPython, KI is + # delivered between bytecodes, not mid-syscall; os.dup2 is a single C call + # and completes atomically, so this race is not realistically triggerable. if saved_out is not None: try: os.dup2(saved_out, 1) From 0236ed2d6b2d75c90e5b2bf1fc891f667bfdaf1d Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 23:51:21 +0000 Subject: [PATCH 11/14] fix: address 3 code review findings (round 10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - immich_api: use 'or []' instead of .get("people", []) in get_people so {"people": null} responses (some Immich versions with zero people enrolled) return [] rather than None; .get() default only fires when the key is absent, not when its value is null - embeddings: log OSError from os.dup2 restore at DEBUG rather than silently swallowing it; if a C extension (CUDA/onnxruntime) invalidates the saved fd, the restore fails silently and stdout stays wired to /dev/null — logging makes the event observable without changing the swallow-and-continue semantics - cache: remove MemoryError re-raise from EmbeddingCache.get(); a cache read OOM aborted the entire diversity-selection batch for the person rather than falling back to a fresh embedding computation, which is the more appropriate OOM gate; broadening back to except Exception restores the pre-round-5 fallback behavior --- winnow/cache.py | 2 -- winnow/embeddings.py | 8 ++++---- winnow/immich_api.py | 2 +- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/winnow/cache.py b/winnow/cache.py index b28f14a..183c3a2 100644 --- a/winnow/cache.py +++ b/winnow/cache.py @@ -75,8 +75,6 @@ class EmbeddingCache: if os.path.exists(path): try: return np.load(path) - except MemoryError: - raise except Exception: return None return None diff --git a/winnow/embeddings.py b/winnow/embeddings.py index 9d59d8a..a715162 100644 --- a/winnow/embeddings.py +++ b/winnow/embeddings.py @@ -45,8 +45,8 @@ def _suppress_output(): if saved_out is not None: try: os.dup2(saved_out, 1) - except OSError: - pass + except OSError as e: + logger.debug("_suppress_output: failed to restore stdout fd: %s", e) finally: try: os.close(saved_out) @@ -55,8 +55,8 @@ def _suppress_output(): if saved_err is not None: try: os.dup2(saved_err, 2) - except OSError: - pass + except OSError as e: + logger.debug("_suppress_output: failed to restore stderr fd: %s", e) finally: try: os.close(saved_err) diff --git a/winnow/immich_api.py b/winnow/immich_api.py index fbe0b3a..6ecc31f 100644 --- a/winnow/immich_api.py +++ b/winnow/immich_api.py @@ -61,7 +61,7 @@ def get_people() -> list[dict]: if not isinstance(data, dict): logger.error("Unexpected response shape from Immich /people: %r", type(data)) return [] - return data.get("people", []) + return data.get("people") or [] except (requests.RequestException, ValueError, AttributeError) as e: logger.error("Failed to fetch people from Immich: %s", e) return [] From 2182c87c4072e911c51ad2b597ff2575fc833ce8 Mon Sep 17 00:00:00 2001 From: Holden Date: Wed, 17 Jun 2026 00:02:49 +0000 Subject: [PATCH 12/14] fix: address 2 code review findings (round 11) - upload_tracker: revert data[flat_key] = [] from round 8; clearing the entire shared legacy flat list on a corrupt value wipes all persons' IDs, not just the one being reset; since a corrupt non-list value is already unreadable by load_uploaded_ids, leaving it in place is safer than a mass-wipe; update warning message to note the field is unaffected but unreadable so the corruption is still observable - cli: use p.get("id") instead of p["id"] in both people-list fallback returns (_handle_duplicate_people lines 144 and 157) for consistency with the success path at line 149; bare subscript crashes on malformed unnamed persons that bypass _smaller_duplicate_ids --- winnow/cli.py | 4 ++-- winnow/upload_tracker.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/winnow/cli.py b/winnow/cli.py index 55391a0..f568f91 100644 --- a/winnow/cli.py +++ b/winnow/cli.py @@ -109,7 +109,7 @@ def _handle_duplicate_people(people: list[dict]) -> list[dict]: ) # Return deduplicated list — keep only the largest per name so that # downstream job creation never runs two jobs for the same Frigate folder. - return [p for p in people if p["id"] not in skip_ids] + return [p for p in people if p.get("id") not in skip_ids] # Auto-merge: survivor = largest asset count, rest merge into it inside Immich merged_any = False @@ -141,7 +141,7 @@ def _handle_duplicate_people(people: list[dict]) -> list[dict]: " — possible transient error or expired API key;" " proceeding with pre-merge list. Check IMMICH_API_KEY if this recurs." ) - return [p for p in people if p["id"] not in skip_ids] + return [p for p in people if p.get("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/upload_tracker.py b/winnow/upload_tracker.py index 39e57a3..04ead4c 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -444,10 +444,10 @@ def reset_person(person_name: str) -> None: person_ids = set(_get_ids(tracker_entry)) if person_ids and flat_key in data and not isinstance(data[flat_key], list): logger.warning( - "reset_person: %s has unexpected type for %s (%s) — clearing corrupt flat-list", + "reset_person: %s has unexpected type for %s (%s) — skipping flat-list cleanup;" + " all persons' legacy IDs in this field are unaffected but unreadable", filename, flat_key, type(data[flat_key]).__name__, ) - data[flat_key] = [] elif person_ids and flat_key in data: data[flat_key] = sorted(set(data[flat_key]) - person_ids) _save(filename, data) From 0914608bc86f0264151c5123613df6643df74b22 Mon Sep 17 00:00:00 2001 From: Holden Date: Wed, 17 Jun 2026 00:10:01 +0000 Subject: [PATCH 13/14] fix: address 2 missed p[\"id\"] bare subscripts in cli.py (round 12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 11's replace_all missed two occurrences: - _smaller_duplicate_ids inner comprehension (line 84): p["id"] → p.get("id") so a named person with a missing "id" field does not crash skip_ids computation before any return path is reached - all-merges-failed fallback return (line 157): same fix; the outer indentation prevented replace_all from matching this occurrence The intentional p["id"] in merge_ids (line 119) is kept: that ID is passed directly to merge_people() where None would be a caller bug, not a silent data corruption. --- winnow/cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/winnow/cli.py b/winnow/cli.py index f568f91..7ac5447 100644 --- a/winnow/cli.py +++ b/winnow/cli.py @@ -81,7 +81,7 @@ def _handle_duplicate_people(people: list[dict]) -> list[dict]: def _smaller_duplicate_ids(groups: dict) -> set[str]: """IDs of all but the largest person in each duplicate group.""" return { - p["id"] + p.get("id") for ps in groups.values() for p in sorted(ps, key=lambda x: x.get("assetCount", 0), reverse=True)[1:] } @@ -154,7 +154,7 @@ def _handle_duplicate_people(people: list[dict]) -> list[dict]: " [yellow]All merges failed — applying local deduplication" " to avoid overwriting output.[/yellow]" ) - return [p for p in people if p["id"] not in skip_ids] + return [p for p in people if p.get("id") not in skip_ids] _UNSUPPORTED_VARS = [ From 5dcfde7c366530b4c8fe95ecdfdbc1fa0d5cc060 Mon Sep 17 00:00:00 2001 From: Holden Date: Wed, 17 Jun 2026 00:17:32 +0000 Subject: [PATCH 14/14] release: v0.6.4 --- CHANGELOG.md | 40 ++++++++++++++++++++++++++++++++++++++++ pyproject.toml | 2 +- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 276c536..58c90b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,46 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.6.4] - 2026-06-17 + +### Fixed + +- **Face bbox scaled to thumbnail space before quality filtering**: `assess_quality` now receives coordinates in thumbnail-pixel space rather than detection-image space. Previously, a face detected on a full-resolution image (e.g. 4000 px wide) was compared against `MIN_FACE_WIDTH` using its original pixel dimensions, causing faces that appear small on the thumbnail to pass the quality filter — and faces that appear large to be incorrectly rejected. + +- **`conf_array` default restored to 1.0 for faces with missing confidence**: the default was incorrectly set to 0.5, causing images with no `score` field in the Immich faces API response to receive a 1.7× FPS diversity boost and be selected ahead of genuinely high-confidence detections. The default is now 1.0 (no boost), treating missing confidence as neutral. + +- **`hard_weight` computed once outside FPS loop**: `conf_array` is constant after initialisation; moving the `np.where` call outside the `while` loop eliminates one O(n) numpy pass per selected image. + +- **`has_frigate_model` snapshot prevents mid-batch `recognize_face` calls on first run**: `effective_count` is incremented inside the upload loop, so using it as the `recognize_face` gate would incorrectly trigger scoring after the first upload on a first run. A boolean snapshot is now taken before the loop. + +- **`person_has_fscores` only set when tracker write succeeds**: the flag was moved outside the `try/except else` block, causing at-cap replacement to switch into Frigate-score mode even when the score was never written to the tracker — `get_most_redundant_mapped_file` then returned `None` and all replacement candidates were silently skipped. The flag is now set only in the `else` branch. + +- **`STRATEGY=skip` honoured before embedding and limit checks**: the strategy was silently converted to `auto` when InsightFace was available, because two early-returns in `_resolve_strategy` ran before the `strategy_map` lookup. + +- **`limit="auto"` preserved on first run**: switching to `limit = capacity` unconditionally caused the FPS adaptive early-stop to never fire on a person's first upload run. `limit="auto"` is now kept when `already_uploaded == 0`. + +- **`EmbeddingCache.get` falls back gracefully on all load errors**: a `MemoryError` during `np.load` of a cached embedding was re-raised, crashing the entire diversity-selection batch for that person. Cache-read failures of any kind now return `None` so the embedding is recomputed fresh. + +- **`get_people` returns `[]` when Immich sends `{"people": null}`**: `.get("people", [])` only uses the default when the key is absent, not when its value is `null`. Changed to `data.get("people") or []` so null-valued responses are handled the same as missing keys. + +- **`get_people` and `fetch_all_assets` guard against non-dict responses**: a proxy or CDN returning a JSON array (or other non-dict body) previously caused an `AttributeError` from `.get()`. Both functions now check `isinstance(data, dict)` and return an empty result with an error log. + +- **`filter_recent_assets` counts and logs assets with missing or unparseable timestamps** instead of silently dropping them. + +- **`_suppress_output` fd cleanup restructured**: the context manager now initialises `devnull_fd`, `saved_out`, and `saved_err` to `None` before the `try` block, so the `finally` can close only the descriptors that were successfully opened. Each `os.close` is wrapped in its own `try/except OSError` so a failed close cannot prevent subsequent descriptors from being released. `OSError` from `os.dup2` restore is logged at DEBUG rather than silently swallowed. + +- **`blur_score_from_image` copies the image before thumbnail resize**: `Image.thumbnail` modifies the image in-place. When the caller's image was already in RGB mode (no convert copy), the resize would have mutated the caller's object. A copy is now made when `score_img is img`. + +- **`imageWidth`/`imageHeight` zero-value treated as missing** in `image_processing.py`: the old `or img_w` fallback silently set `scale = 1.0` for a zero-valued dimension (correct) but also for `None` (also correct) with no distinction. The explicit `scale = img_w / meta_w if meta_w else 1.0` form matches the pattern used in the new `_scale_bbox_to_thumbnail` helper and makes the fallback intent clear. + +- **`_mark` and `update_frigate_count` copy before mutate**: both functions now create a shallow copy of the top-level tracker dict before assigning into `by_person`, so a failed `_save` cannot leave the in-memory cache ahead of the on-disk file. + +- **`reset_person` flat-list guard only warns when cleanup would have run**: the `isinstance(data[flat_key], list)` check previously emitted a warning even when `person_ids` was empty (a no-op call). The warning is now gated behind `person_ids and`, matching the guard on the cleanup branch. + +- **`_handle_duplicate_people` uses `p.get("id")` consistently**: all four return-path filter comprehensions and the `_smaller_duplicate_ids` set comprehension now use `.get("id")` instead of bare `p["id"]`, preventing a `KeyError` if the Immich API returns a person record without an `id` field. + +- **`K-Medoids` non-medoid membership test is O(1)**: `non_medoids` now filters against `set(medoids)` instead of the list, eliminating an O(k) scan per candidate on each outer iteration. + ## [0.6.3] - 2026-06-16 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index a4437a0..3a82977 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.6.3" +version = "0.6.4" description = "Selects diverse, high-quality photos from Immich as training data for Frigate face recognition." license = "AGPL-3.0-or-later" requires-python = ">=3.13"