fix: code review corrections — dedup O(N²), truncated rejection check, pool warning, path guard
- _dedup_embeddings: rebuild kept_stack only on keep (was every iteration → O(N²)) - _dedup_embeddings: fix quality_score sort key to use explicit None check (falsy-zero) - _select_by_embedding: add post-dedup pool < limit guard with warning - executor: use full resp.text for 'face' keyword check; only truncate display snippet - _safe_person_dir: avoid false "//" prefix when output_dir resolves to filesystem root
This commit is contained in:
@@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
## [0.4.5] - 2026-06-14
|
||||
|
||||
### Fixed
|
||||
|
||||
- **Near-duplicate dedup O(N²) allocation**: `np.vstack(kept_normed)` was rebuilt on every loop iteration even for candidates that would be dropped; the stack is now rebuilt only when a new item is kept, reducing memory pressure significantly for large pools.
|
||||
- **`quality_score` falsy-zero in dedup sort**: the sort key used `c.get("quality_score") or 0.0`, which treated a legitimate `quality_score=0.0` identically to a missing key. Changed to an explicit `None` check so zero is preserved as-is, and object-mode candidates (which have no `quality_score`) continue to sort stably to the back.
|
||||
- **Post-dedup pool not re-checked against limit**: after near-duplicate removal the pool could silently shrink below the requested limit with no warning. A second `len < limit` guard now fires after dedup and emits the same "Only N embeddings" warning that the pre-dedup guard does.
|
||||
- **`mark_rejected` could miss plain-text 400 bodies longer than 100 bytes**: `error_detail = resp.text[:100]` was being searched for the keyword `"face"` to gate `mark_rejected()`, so a response body with `"face"` after byte 100 would never mark the asset rejected and it would be retried on every future run. The `"face"` check now uses the full response body; truncation is kept only for the displayed snippet.
|
||||
- **`_safe_person_dir` raised ValueError for all person names when `output_dir` resolved to `/`**: `base + os.sep` produced `"//"` when base was `"/"`, and valid paths like `/alice` don't start with `"//"`. Fixed by using `base` directly as the prefix when `base == os.sep`.
|
||||
|
||||
## [0.4.4] - 2026-06-14
|
||||
|
||||
### Added
|
||||
|
||||
+1
-1
@@ -1,6 +1,6 @@
|
||||
[project]
|
||||
name = "winnow"
|
||||
version = "0.4.4"
|
||||
version = "0.4.5"
|
||||
description = "Selects diverse, high-quality photos from Immich as training data for Frigate face recognition and object classification."
|
||||
license = "AGPL-3.0-or-later"
|
||||
requires-python = ">=3.13"
|
||||
|
||||
+12
-6
@@ -290,6 +290,11 @@ def _select_by_embedding(
|
||||
embeddings, valid_candidates, confidence_scores
|
||||
)
|
||||
|
||||
# Re-check after dedup: pool may have shrunk below limit
|
||||
if limit != "auto" and len(valid_candidates) < limit:
|
||||
logger.warning(f"Only {len(valid_candidates)} embeddings after near-duplicate removal. Returning all.")
|
||||
return valid_candidates
|
||||
|
||||
# --- Phase 6: Cluster-aware selection ---
|
||||
return _cluster_aware_selection(
|
||||
embeddings,
|
||||
@@ -326,21 +331,22 @@ def _dedup_embeddings(
|
||||
norms = np.linalg.norm(emb_matrix, axis=1, keepdims=True)
|
||||
emb_normed = emb_matrix / np.maximum(norms, 1e-8)
|
||||
|
||||
# Sort by quality descending so the best image in each near-duplicate group wins
|
||||
quality_scores = [c.get("quality_score") or 0.0 for c in candidates]
|
||||
# Sort by quality descending so the best image in each near-duplicate group wins.
|
||||
# Use explicit None check so a legitimate quality_score=0.0 isn't treated as missing.
|
||||
quality_scores = [qs if (qs := c.get("quality_score")) is not None else 0.0 for c in candidates]
|
||||
order = sorted(range(len(candidates)), key=lambda i: quality_scores[i], reverse=True)
|
||||
|
||||
kept_indices = []
|
||||
kept_normed = []
|
||||
kept_stack: np.ndarray | None = None # rebuilt only when a new item is kept (not every iteration)
|
||||
|
||||
for i in order:
|
||||
if kept_normed:
|
||||
kept_stack = np.vstack(kept_normed)
|
||||
if kept_stack is not None:
|
||||
sims = emb_normed[i] @ kept_stack.T
|
||||
if np.any(sims > 1 - _DEDUP_THRESHOLD):
|
||||
continue
|
||||
kept_indices.append(i)
|
||||
kept_normed.append(emb_normed[i])
|
||||
row = emb_normed[i : i + 1]
|
||||
kept_stack = row if kept_stack is None else np.vstack([kept_stack, row])
|
||||
|
||||
dropped = len(embeddings) - len(kept_indices)
|
||||
if dropped:
|
||||
|
||||
+8
-4
@@ -46,7 +46,10 @@ def _safe_person_dir(output_dir: str, person_name: str) -> str:
|
||||
"""
|
||||
candidate = os.path.realpath(os.path.join(output_dir, person_name))
|
||||
base = os.path.realpath(output_dir)
|
||||
if not candidate.startswith(base + os.sep) and candidate != base:
|
||||
# Use the base path as its own prefix when it's the filesystem root ("/"),
|
||||
# otherwise append os.sep — avoids the false "//" double-slash when base == "/".
|
||||
base_prefix = base if base == os.sep else base + os.sep
|
||||
if not candidate.startswith(base_prefix) and candidate != base:
|
||||
raise ValueError(f"Person name {person_name!r} escapes output directory — skipping")
|
||||
return candidate
|
||||
|
||||
@@ -603,15 +606,16 @@ def upload_to_frigate(jobs: list[dict]) -> None:
|
||||
progress.console.print(
|
||||
f" [red]✗ {fname}: HTTP {resp.status_code} (after {max_retries} attempts)[/red]"
|
||||
)
|
||||
full_body = resp.text
|
||||
try:
|
||||
error_detail = resp.json().get("message", resp.text[:100])
|
||||
error_detail = resp.json().get("message", full_body[:100])
|
||||
except Exception:
|
||||
error_detail = resp.text[:100]
|
||||
error_detail = full_body[:100]
|
||||
if resp.status_code == 400:
|
||||
progress.console.print(f" [dim]{error_detail}[/dim]")
|
||||
else:
|
||||
logger.debug(f"{fname} HTTP {resp.status_code}: {error_detail}")
|
||||
if resp.status_code == 400 and "face" in error_detail.lower():
|
||||
if resp.status_code == 400 and "face" in full_body.lower():
|
||||
asset_id = asset_map.get(fname)
|
||||
if asset_id:
|
||||
mark_rejected(asset_id, person_name=name)
|
||||
|
||||
Reference in New Issue
Block a user