From 3a052db1ce6053adaeba6a13228881ba453a0de1 Mon Sep 17 00:00:00 2001 From: Holden Salomon Date: Sun, 14 Jun 2026 20:04:24 -0400 Subject: [PATCH] =?UTF-8?q?chore:=20quality=20cleanup=20=E2=80=94=20extrac?= =?UTF-8?q?t=20magic=20numbers,=20improve=20docs=20and=20naming=20(#29)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 | 20 ++++++++++++++++++++ pyproject.toml | 2 +- uv.lock | 2 +- winnow/diversity.py | 17 ++++++++++++----- winnow/frigate_api.py | 4 ++-- winnow/immich_api.py | 5 +++++ winnow/reconcile.py | 8 +++++++- winnow/upload_tracker.py | 37 ++++++++++++++++--------------------- 8 files changed, 64 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 310c4f4..7c0769b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/pyproject.toml b/pyproject.toml index 02f785b..d68ba5b 100644 --- a/pyproject.toml +++ b/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" diff --git a/uv.lock b/uv.lock index 5da1d61..6553ac3 100644 --- a/uv.lock +++ b/uv.lock @@ -862,7 +862,7 @@ wheels = [ [[package]] name = "winnow" -version = "0.5.2" +version = "0.5.5" source = { editable = "." } dependencies = [ { name = "croniter" }, diff --git a/winnow/diversity.py b/winnow/diversity.py index 8a6ee0e..30451a8 100644 --- a/winnow/diversity.py +++ b/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] = {} diff --git a/winnow/frigate_api.py b/winnow/frigate_api.py index 124148b..32c4fa6 100644 --- a/winnow/frigate_api.py +++ b/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, ) diff --git a/winnow/immich_api.py b/winnow/immich_api.py index 8ac9bf2..3357268 100644 --- a/winnow/immich_api.py +++ b/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)) diff --git a/winnow/reconcile.py b/winnow/reconcile.py index 9898be5..77aaf39 100644 --- a/winnow/reconcile.py +++ b/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: diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index 8827ee0..9f39b26 100644 --- a/winnow/upload_tracker.py +++ b/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()))