chore: quality cleanup — extract magic numbers, improve docs and naming (#29)

- diversity.py: extract 3000/20/32 to _POOL_CAP/_POOL_SCALE/_EMBEDDING_BATCH_SIZE
- reconcile.py: extract (1,2,4,8) poll delays to _RECONCILE_POLL_DELAYS with comment
- immich_api.py: document dual response shape; debug-log skipped non-dict items
- frigate_api.py: rename `encoded` → `encoded_name` for clarity
- upload_tracker.py: atomic-write note on record_frigate_files_batch docstring;
  get_person_summary() uses setdefault to eliminate four repeated default dicts;
  _VALID_SCORE_COLS comment explains SQL-injection guard intent

Bump version to 0.5.5

CHANGELOG.md

@@ -7,6 +7,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.5.5] - 2026-06-15
+
+### Changed
+
+- **Module-level constants in `diversity.py`**: magic numbers `3000` (pool cap), `20` (pool scale), and `32` (embedding batch size) extracted to named constants `_POOL_CAP`, `_POOL_SCALE`, and `_EMBEDDING_BATCH_SIZE`.
+
+- **Reconciliation poll delays extracted**: `(1, 2, 4, 8)` back-off delays in `reconcile.py` extracted to `_RECONCILE_POLL_DELAYS` with an explanatory comment.
+
+- **`_VALID_SCORE_COLS` comment**: explains that the frozenset is a SQL-injection guard for dynamic column interpolation, not a runtime filter.
+
+- **`record_frigate_files_batch` docstring**: clarifies that all mappings are written atomically — no partial failure is possible.
+
+- **`get_person_summary()` refactored**: eliminated four repeated default-dict blocks using a local `_entry()` helper with `setdefault`.
+
+- **`encoded` → `encoded_name` in `frigate_api.py`**: renamed the URL-encoded person name variable for clarity.
+
+- **Dual response shape comment in `immich_api.py`**: documents that Immich ≥2.x returns `{"assets": {"items": [...]}}` while earlier versions returned `{"assets": [...]}` directly.
+
+- **Non-dict item debug log in `fetch_all_assets`**: skipped non-dict items in a page response now emit a `logger.debug` line with the count and page number.
+
 ## [0.5.4] - 2026-06-14
 
 ### Fixed

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "winnow"
-version = "0.5.4"
+version = "0.5.5"
 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"

uv.lock

@@ -862,7 +862,7 @@ wheels = [
 
 [[package]]
 name = "winnow"
-version = "0.5.2"
+version = "0.5.5"
 source = { editable = "." }
 dependencies = [
     { name = "croniter" },

winnow/diversity.py

@@ -23,6 +23,15 @@ from .quality import assess_quality
 
 logger = logging.getLogger(__name__)
 
+# Candidate pool: cap at _POOL_CAP assets, but take at least _POOL_SCALE × the
+# requested limit so small limits don't artificially narrow the search space.
+_POOL_CAP = 3000
+_POOL_SCALE = 20
+
+# Embedding batch size: bounds decoded thumbnails in memory.
+# At ~3-8 MB each, 32 images ≈ 100–250 MB peak — safe in a 4 GB container.
+_EMBEDDING_BATCH_SIZE = 32
+
 
 def select_diverse_assets(
     assets: list,
@@ -193,9 +202,8 @@ def _select_by_embedding(
     4. Embedding computation
     5. Cluster-aware selection with hard example weighting
     """
-    # Determine candidate pool (cap at 3000 for performance)
     effective_limit = 30 if limit == "auto" else limit
-    pool_size = min(3000, max(effective_limit * 20, len(assets)))
+    pool_size = min(_POOL_CAP, max(effective_limit * _POOL_SCALE, len(assets)))
 
     # Subsample if needed (evenly distributed in time)
     if len(assets) > pool_size:
@@ -216,13 +224,12 @@ def _select_by_embedding(
     # produce a subtly different embedding than the full-res version. For most
     # libraries this is negligible; it matters if Immich preview quality is low.
     _fetch = fetch_fn or _fetch_thumbnail
-    _BATCH = 32
     embeddings, valid_candidates, confidence_scores = [], [], []
     quality_filtered = 0
     processed = 0
 
-    for batch_start in range(0, len(candidates), _BATCH):
-        batch = candidates[batch_start : batch_start + _BATCH]
+    for batch_start in range(0, len(candidates), _EMBEDDING_BATCH_SIZE):
+        batch = candidates[batch_start : batch_start + _EMBEDDING_BATCH_SIZE]
 
         # Download this batch concurrently
         batch_images: dict[str, Image.Image] = {}

winnow/frigate_api.py

@@ -144,10 +144,10 @@ def delete_frigate_person_files(person_name: str, filenames: list[str]) -> bool:
     if not frigate_url or not filenames:
         return False
     from urllib.parse import quote
-    encoded = quote(person_name, safe="")
+    encoded_name = quote(person_name, safe="")
     try:
         resp = requests.post(
-            f"{frigate_url}/api/faces/{encoded}/delete",
+            f"{frigate_url}/api/faces/{encoded_name}/delete",
             json={"ids": filenames},
             timeout=10,
         )

winnow/immich_api.py

@@ -110,12 +110,17 @@ def fetch_all_assets(person: dict) -> list[dict]:
                 break
 
             page_assets = resp.json().get("assets", [])
+            # Immich ≥2.x returns {"assets": {"items": [...]}};
+            # earlier versions returned {"assets": [...]} directly.
             if isinstance(page_assets, dict):
                 page_assets = page_assets.get("items", [])
 
             if not page_assets:
                 break
 
+            skipped = [a for a in page_assets if not isinstance(a, dict)]
+            if skipped:
+                logger.debug("%s: skipping %s non-dict item(s) in page %s", name, len(skipped), page)
             assets.extend(a for a in page_assets if isinstance(a, dict))
             logger.debug("Fetched page %s, total: %s", page, len(assets))
 

winnow/reconcile.py

@@ -9,6 +9,12 @@ from .upload_tracker import record_frigate_files_batch
 
 logger = logging.getLogger(__name__)
 
+# Exponential back-off delays (seconds) when polling Frigate after uploads.
+# Frigate processes the upload queue asynchronously, so files aren't
+# immediately visible in GET /api/faces — we wait progressively longer
+# rather than hammering the API.
+_RECONCILE_POLL_DELAYS = (1, 2, 4, 8)
+
 
 def reconcile_frigate_mappings(
     person_name: str,
@@ -34,7 +40,7 @@ def reconcile_frigate_mappings(
     target = len(uploaded)
     current_files: set[str] = set()
 
-    for delay in (1, 2, 4, 8):
+    for delay in _RECONCILE_POLL_DELAYS:
         time.sleep(delay)
         fresh = get_frigate_person_files(person_name)
         if fresh is None:

winnow/upload_tracker.py

@@ -246,7 +246,11 @@ def mark_rejected(asset_id: str, person_name: str | None = None) -> None:
 
 
 def record_frigate_files_batch(person_name: str, mappings: dict[str, str]) -> None:
-    """Record multiple Frigate filename → asset_id mappings in a single transaction."""
+    """Record multiple Frigate filename → asset_id mappings in a single transaction.
+
+    All rows are written atomically — either every mapping is persisted or none
+    are (SQLite rolls back on error), so there is no risk of partial failure.
+    """
     if not mappings:
         return
     conn = _get_conn()
@@ -311,6 +315,8 @@ def has_frigate_scores(person_name: str) -> bool:
     return row[0] > 0
 
 
+# Guard against SQL injection from callers passing dynamic column names.
+# Only these two columns exist and are safe to interpolate into queries.
 _VALID_SCORE_COLS = frozenset({"blur_score", "frigate_score"})
 
 
@@ -436,13 +442,14 @@ def get_person_summary() -> dict[str, dict]:
            GROUP BY person_name, status"""
     ).fetchall()
 
+    def _entry(summary: dict, name: str) -> dict:
+        return summary.setdefault(
+            name, {"uploaded": 0, "rejected": 0, "frigate_count": None, "scores": {}, "frigate_files": {}}
+        )
+
     summary: dict[str, dict] = {}
     for r in rows:
-        name = r["person_name"]
-        if name not in summary:
-            summary[name] = {"uploaded": 0, "rejected": 0, "frigate_count": None,
-                             "scores": {}, "frigate_files": {}}
-        summary[name][r["status"]] = r["cnt"]
+        _entry(summary, r["person_name"])[r["status"]] = r["cnt"]
 
     # Scores for uploaded assets
     score_rows = conn.execute(
@@ -451,33 +458,21 @@ def get_person_summary() -> dict[str, dict]:
            WHERE status='uploaded' AND person_name IS NOT NULL AND blur_score IS NOT NULL"""
     ).fetchall()
     for r in score_rows:
-        name = r["person_name"]
-        if name not in summary:
-            summary[name] = {"uploaded": 0, "rejected": 0, "frigate_count": None,
-                             "scores": {}, "frigate_files": {}}
-        summary[name]["scores"][r["asset_id"]] = r["blur_score"]
+        _entry(summary, r["person_name"])["scores"][r["asset_id"]] = r["blur_score"]
 
     # Frigate file mappings
     ff_rows = conn.execute(
         "SELECT person_name, frigate_filename, asset_id FROM frigate_files"
     ).fetchall()
     for r in ff_rows:
-        name = r["person_name"]
-        if name not in summary:
-            summary[name] = {"uploaded": 0, "rejected": 0, "frigate_count": None,
-                             "scores": {}, "frigate_files": {}}
-        summary[name]["frigate_files"][r["frigate_filename"]] = r["asset_id"]
+        _entry(summary, r["person_name"])["frigate_files"][r["frigate_filename"]] = r["asset_id"]
 
     # Frigate counts
     meta_rows = conn.execute(
         "SELECT person_name, frigate_count FROM person_metadata"
     ).fetchall()
     for r in meta_rows:
-        name = r["person_name"]
-        if name not in summary:
-            summary[name] = {"uploaded": 0, "rejected": 0, "frigate_count": None,
-                             "scores": {}, "frigate_files": {}}
-        summary[name]["frigate_count"] = r["frigate_count"]
+        _entry(summary, r["person_name"])["frigate_count"] = r["frigate_count"]
 
     return dict(sorted(summary.items()))