fix: resolve all linting errors, add tests, bump base image

Linting (ruff): - cli.py: sort relative imports (I001) - jobs.py: remove unused get_people import (F401), wrap long line (E501) - executor.py: remove unused success variable (F841), wrap 4 long lines (E501) Tests (24 passing): - tests/test_config.py: config singleton defaults + env var overrides - tests/test_upload_tracker.py: mark/filter/reset/summary logic - tests/test_immich_api.py: filter_recent_assets date boundary cases - tests/test_jobs.py: _resolve_strategy with LIMIT env var and fallbacks - pyproject.toml: add [tool.pytest.ini_options] testpaths=["tests"] so pytest doesn't scan .venv in CI Security: - Dockerfile: bump CUDA base from 12.6.3 to 12.9.2 to pick up patched Ubuntu packages (fixes Dependabot low-severity alert) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-06-12 00:42:38 +00:00
parent c861d56a53
commit eedbd3180a
9 changed files with 303 additions and 12 deletions
@@ -2,7 +2,7 @@
 #   amd64:  NVIDIA CUDA 12.6 (GPU acceleration when available, CPU fallback)
 #   arm64:  Plain Ubuntu (CPU-only, no CUDA on ARM)

-FROM --platform=$BUILDPLATFORM nvidia/cuda:12.6.3-cudnn-runtime-ubuntu22.04 AS base-amd64
+FROM --platform=$BUILDPLATFORM nvidia/cuda:12.9.2-cudnn-runtime-ubuntu22.04 AS base-amd64
 FROM --platform=$BUILDPLATFORM ubuntu:22.04 AS base-arm64

 # ── Build stage ───────────────────────────────────────────────────────────
@@ -7,11 +7,11 @@ from rich import print as rprint
 from rich.prompt import Confirm

 from .config import Config, ConfigManager
+from .executor import execute_jobs, upload_to_frigate
 from .immich_api import get_people
+from .jobs import _show_preview, auto_configure, interactive_configure
 from .logging import console, setup_logging
 from .upload_tracker import get_person_summary, reset_person
-from .executor import execute_jobs, upload_to_frigate
-from .jobs import auto_configure, interactive_configure, _show_preview

 logger = logging.getLogger(__name__)

@@ -242,8 +242,6 @@ def upload_to_frigate(jobs: list[dict]) -> None:

            for fname in person_files:
                fpath = os.path.join(person_dir, fname)
-                success = False
-
                for attempt in range(1, max_retries + 1):
                    try:
                        with open(fpath, "rb") as f:
@@ -255,7 +253,6 @@ def upload_to_frigate(jobs: list[dict]) -> None:
                        if resp.status_code == 200:
                            uploaded += 1
                            person_uploaded += 1
-                            success = True

                            # Mark this asset as uploaded so it's skipped on future runs
                            asset_id = asset_map.get(fname)
@@ -265,7 +262,10 @@ def upload_to_frigate(jobs: list[dict]) -> None:
                            break
                        else:
                            if attempt < max_retries:
-                                logger.warning(f"Upload attempt {attempt}/{max_retries} for {fname}: HTTP {resp.status_code}, retrying...")
+                                logger.warning(
+                                    f"Upload attempt {attempt}/{max_retries} for {fname}:"
+                                    f" HTTP {resp.status_code}, retrying..."
+                                )
                                continue
                            failed += 1
                            person_failed += 1
@@ -284,17 +284,27 @@ def upload_to_frigate(jobs: list[dict]) -> None:
                                    mark_rejected(asset_id, person_name=name)
                    except (requests.exceptions.ConnectionError, requests.exceptions.Timeout) as exc:
                        if attempt < max_retries:
-                            logger.warning(f"Upload attempt {attempt}/{max_retries} for {fname}: {type(exc).__name__}, retrying...")
+                            logger.warning(
+                                f"Upload attempt {attempt}/{max_retries} for {fname}:"
+                                f" {type(exc).__name__}, retrying..."
+                            )
                            continue
                        failed += 1
                        person_failed += 1
-                        label = "Connection refused" if isinstance(exc, requests.exceptions.ConnectionError) else "Request timed out (30s)"
+                        label = (
+                            "Connection refused"
+                            if isinstance(exc, requests.exceptions.ConnectionError)
+                            else "Request timed out (30s)"
+                        )
                        progress.console.print(
                            f"    [red]✗ {fname}: {label} (after {max_retries} attempts)[/red]"
                        )
                    except Exception as e:
                        if attempt < max_retries:
-                            logger.warning(f"Upload attempt {attempt}/{max_retries} for {fname}: {type(e).__name__}, retrying...")
+                            logger.warning(
+                                f"Upload attempt {attempt}/{max_retries} for {fname}:"
+                                f" {type(e).__name__}, retrying..."
+                            )
                            continue
                        failed += 1
                        person_failed += 1
@@ -11,7 +11,7 @@ from rich.table import Table
 from .config import Config
 from .diversity import select_diverse_assets
 from .embeddings import is_embedding_available, load_embedding_model
-from .immich_api import fetch_all_assets, filter_recent_assets, get_people
+from .immich_api import fetch_all_assets, filter_recent_assets
 from .logging import console
 from .upload_tracker import filter_already_uploaded

@@ -231,7 +231,10 @@ def auto_configure(people: list[dict]) -> list[dict]:
    if min_face_count > 0:
        valid_people = [p for p in valid_people if p.get("assetCount", 0) >= min_face_count]
        if valid_people:
-            rprint(f"  Filtered to {len(valid_people)} people with ≥{min_face_count} assets (MIN_FACE_COUNT={min_face_count})")
+            rprint(
+                f"  Filtered to {len(valid_people)} people with"
+                f" ≥{min_face_count} assets (MIN_FACE_COUNT={min_face_count})"
+            )

    jobs = []
    for person in valid_people:
@@ -91,6 +91,9 @@ ultralytics = "ultralytics"
 [tool.deptry.per_rule_ignores]
 DEP002 = ["onnxruntime-gpu", "nvidia-cudnn-cu12"]

+[tool.pytest.ini_options]
+testpaths = ["tests"]
+
 [build-system]
 requires = ["hatchling"]
 build-backend = "hatchling.build"
@@ -0,0 +1,80 @@
+"""Smoke tests for configuration loading."""
+
+import os
+
+import pytest
+
+
+def test_config_loads_defaults(monkeypatch):
+    """Config reads env vars and falls back to documented defaults."""
+    monkeypatch.setenv("IMMICH_URL", "http://test:2283")
+    monkeypatch.setenv("API_KEY", "test-key")
+
+    from if_curator.config import _Config
+
+    _Config.reset()
+    cfg = _Config()
+
+    assert cfg.IMMICH_URL == "http://test:2283"
+    assert cfg.API_KEY == "test-key"
+    assert cfg.OUTPUT_DIR == "./frigate_train"
+    assert cfg.YEARS_FILTER == 10
+    assert cfg.MIN_FACE_WIDTH == 50
+    assert cfg.MIN_FACE_COUNT == 0
+    assert cfg.BLUR_THRESHOLD == 100.0
+    assert cfg.MIN_CONFIDENCE == 0.7
+    assert cfg.MAX_AUTO_IMAGES == 80
+    assert cfg.FACE_MARGIN == 0.15
+    assert cfg.USE_FULL_RESOLUTION is True
+    assert cfg.ENABLE_FACE_ALIGNMENT is True
+    assert cfg.ENABLE_CACHE is False
+
+    _Config.reset()
+
+
+def test_config_env_overrides(monkeypatch):
+    """All quality settings are overridable via environment variables."""
+    monkeypatch.setenv("IMMICH_URL", "http://test:2283")
+    monkeypatch.setenv("API_KEY", "test-key")
+    monkeypatch.setenv("YEARS_FILTER", "5")
+    monkeypatch.setenv("MIN_FACE_WIDTH", "80")
+    monkeypatch.setenv("BLUR_THRESHOLD", "50.0")
+    monkeypatch.setenv("MIN_CONFIDENCE", "0.9")
+    monkeypatch.setenv("MAX_AUTO_IMAGES", "40")
+    monkeypatch.setenv("FACE_MARGIN", "0.2")
+    monkeypatch.setenv("USE_FULL_RESOLUTION", "false")
+    monkeypatch.setenv("ENABLE_FACE_ALIGNMENT", "false")
+    monkeypatch.setenv("ENABLE_CACHE", "true")
+
+    from if_curator.config import _Config
+
+    _Config.reset()
+    cfg = _Config()
+
+    assert cfg.YEARS_FILTER == 5
+    assert cfg.MIN_FACE_WIDTH == 80
+    assert cfg.BLUR_THRESHOLD == 50.0
+    assert cfg.MIN_CONFIDENCE == 0.9
+    assert cfg.MAX_AUTO_IMAGES == 40
+    assert cfg.FACE_MARGIN == 0.2
+    assert cfg.USE_FULL_RESOLUTION is False
+    assert cfg.ENABLE_FACE_ALIGNMENT is False
+    assert cfg.ENABLE_CACHE is True
+
+    _Config.reset()
+
+
+def test_get_headers_returns_api_key(monkeypatch):
+    """get_headers() returns the correct auth header dict."""
+    monkeypatch.setenv("IMMICH_URL", "http://test:2283")
+    monkeypatch.setenv("API_KEY", "my-secret-key")
+
+    from if_curator.config import _Config, get_headers
+
+    _Config.reset()
+    headers = get_headers()
+
+    assert headers["x-api-key"] == "my-secret-key"
+    assert headers["Accept"] == "application/json"
+
+    _Config.reset()
@@ -0,0 +1,53 @@
+"""Tests for Immich API utility functions (no network required)."""
+
+from datetime import datetime, timedelta, timezone
+
+
+def _asset(days_ago: int) -> dict:
+    ts = (datetime.now(timezone.utc) - timedelta(days=days_ago)).isoformat()
+    return {"id": f"id-{days_ago}d", "fileCreatedAt": ts}
+
+
+def test_filter_recent_keeps_new_assets(monkeypatch):
+    from if_curator.immich_api import filter_recent_assets
+    assets = [_asset(1), _asset(30), _asset(365 * 5)]
+    result = filter_recent_assets(assets, years=10)
+    assert len(result) == 3
+
+
+def test_filter_recent_removes_old_assets(monkeypatch):
+    from if_curator.immich_api import filter_recent_assets
+    old = _asset(365 * 15)
+    recent = _asset(10)
+    result = filter_recent_assets([old, recent], years=10)
+    assert len(result) == 1
+    assert result[0]["id"] == "id-10d"
+
+
+def test_filter_recent_boundary(monkeypatch):
+    from if_curator.immich_api import filter_recent_assets
+    just_inside = _asset(365 * 10 - 1)
+    just_outside = _asset(365 * 10 + 1)
+    result = filter_recent_assets([just_inside, just_outside], years=10)
+    assert len(result) == 1
+    assert result[0] == just_inside
+
+
+def test_filter_recent_skips_missing_date():
+    from if_curator.immich_api import filter_recent_assets
+    assets = [{"id": "no-date"}, _asset(5)]
+    result = filter_recent_assets(assets, years=10)
+    assert len(result) == 1
+    assert result[0]["id"] == "id-5d"
+
+
+def test_filter_recent_skips_bad_date():
+    from if_curator.immich_api import filter_recent_assets
+    assets = [{"id": "bad", "fileCreatedAt": "not-a-date"}, _asset(5)]
+    result = filter_recent_assets(assets, years=10)
+    assert len(result) == 1
+
+
+def test_filter_recent_empty_list():
+    from if_curator.immich_api import filter_recent_assets
+    assert filter_recent_assets([], years=10) == []
@@ -0,0 +1,68 @@
+"""Tests for job strategy resolution and env var handling."""
+
+import pytest
+
+
+@pytest.fixture(autouse=True)
+def clean_env(monkeypatch):
+    """Ensure LIMIT and STRATEGY are unset before each test."""
+    monkeypatch.delenv("LIMIT", raising=False)
+    monkeypatch.delenv("STRATEGY", raising=False)
+
+
+def test_resolve_strategy_default_auto():
+    from if_curator.jobs import _resolve_strategy
+    limit, mode = _resolve_strategy("auto", has_embedding=True)
+    assert limit == "auto"
+    assert mode == "smart"
+
+
+def test_resolve_strategy_standard():
+    from if_curator.jobs import _resolve_strategy
+    limit, mode = _resolve_strategy("standard", has_embedding=True)
+    assert limit == 30
+    assert mode == "smart"
+
+
+def test_resolve_strategy_broad():
+    from if_curator.jobs import _resolve_strategy
+    limit, mode = _resolve_strategy("broad", has_embedding=True)
+    assert limit == 100
+    assert mode == "smart"
+
+
+def test_resolve_strategy_custom_limit_env(monkeypatch):
+    monkeypatch.setenv("LIMIT", "50")
+    from if_curator.jobs import _resolve_strategy
+    limit, mode = _resolve_strategy("auto", has_embedding=True)
+    assert limit == 50
+    assert mode == "smart"
+
+
+def test_resolve_strategy_limit_overrides_strategy(monkeypatch):
+    monkeypatch.setenv("LIMIT", "25")
+    from if_curator.jobs import _resolve_strategy
+    limit, mode = _resolve_strategy("broad", has_embedding=True)
+    assert limit == 25
+
+
+def test_resolve_strategy_no_embedding_falls_back_to_time():
+    from if_curator.jobs import _resolve_strategy
+    limit, mode = _resolve_strategy("auto", has_embedding=False)
+    assert mode == "time"
+    assert isinstance(limit, int)
+
+
+def test_resolve_strategy_no_embedding_respects_limit(monkeypatch):
+    monkeypatch.setenv("LIMIT", "60")
+    from if_curator.jobs import _resolve_strategy
+    limit, mode = _resolve_strategy("auto", has_embedding=False)
+    assert limit == 60
+    assert mode == "time"
+
+
+def test_resolve_strategy_unknown_falls_back_to_auto():
+    from if_curator.jobs import _resolve_strategy
+    limit, mode = _resolve_strategy("unknown-strategy", has_embedding=True)
+    assert limit == "auto"
+    assert mode == "smart"
@@ -0,0 +1,74 @@
+"""Tests for upload tracker — mark, filter, reset, and summary logic."""
+
+import json
+import os
+import tempfile
+
+import pytest
+
+
+@pytest.fixture(autouse=True)
+def isolated_cache(monkeypatch, tmp_path):
+    """Point tracker at a temp directory so tests don't touch real cache files."""
+    monkeypatch.setenv("CACHE_DIR", str(tmp_path))
+    from if_curator.config import _Config
+    _Config.reset()
+    yield tmp_path
+    _Config.reset()
+
+
+def test_filter_returns_all_when_empty():
+    from if_curator.upload_tracker import filter_already_uploaded
+    ids = ["a1", "b2", "c3"]
+    assert filter_already_uploaded(ids) == ids
+
+
+def test_mark_uploaded_excludes_from_filter():
+    from if_curator.upload_tracker import filter_already_uploaded, mark_uploaded
+    mark_uploaded("a1", person_name="Alice")
+    result = filter_already_uploaded(["a1", "b2"])
+    assert result == ["b2"]
+
+
+def test_mark_rejected_excludes_from_filter():
+    from if_curator.upload_tracker import filter_already_uploaded, mark_rejected
+    mark_rejected("x9", person_name="Bob")
+    result = filter_already_uploaded(["x9", "y8"])
+    assert result == ["y8"]
+
+
+def test_retry_rejected_includes_rejected():
+    from if_curator.upload_tracker import filter_already_uploaded, mark_rejected
+    mark_rejected("x9", person_name="Bob")
+    result = filter_already_uploaded(["x9", "y8"], retry_rejected=True)
+    assert "x9" in result
+
+
+def test_reset_person_clears_records():
+    from if_curator.upload_tracker import filter_already_uploaded, mark_uploaded, reset_person
+    mark_uploaded("a1", person_name="Alice")
+    mark_uploaded("a2", person_name="Alice")
+    mark_uploaded("b1", person_name="Bob")
+    reset_person("Alice")
+    assert filter_already_uploaded(["a1", "a2"]) == ["a1", "a2"]
+    assert filter_already_uploaded(["b1"]) == []
+
+
+def test_get_person_summary():
+    from if_curator.upload_tracker import get_person_summary, mark_rejected, mark_uploaded
+    mark_uploaded("a1", person_name="Alice")
+    mark_uploaded("a2", person_name="Alice")
+    mark_rejected("a3", person_name="Alice")
+    mark_uploaded("b1", person_name="Bob")
+    summary = get_person_summary()
+    assert summary["Alice"]["uploaded"] == 2
+    assert summary["Alice"]["rejected"] == 1
+    assert summary["Bob"]["uploaded"] == 1
+    assert summary["Bob"]["rejected"] == 0
+
+
+def test_duplicate_marks_are_idempotent():
+    from if_curator.upload_tracker import filter_already_uploaded, mark_uploaded
+    mark_uploaded("dup", person_name="Alice")
+    mark_uploaded("dup", person_name="Alice")
+    assert filter_already_uploaded(["dup", "new"]) == ["new"]