From 57e039e34d0ba577017364c74a59a2230c88a13c Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 2 Jun 2026 12:58:59 -0700 Subject: [PATCH] Canonical: make source selection a job setting (default active-preferred) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Feedback from the live dry-run: the job was pinning whichever source best fit the files regardless of which source it was, which was surprising — users expect it to respect their active metadata source. Made it a per-job setting instead of a baked-in policy. source_selection (default 'active_preferred'): - active_preferred — use the active/primary metadata source's release when the album has an ID for it AND it clears the score floor; otherwise fall back to the best-fit among the other sources. Respects the configured source but self-heals when that link is clearly broken (below floor / no ID). - active_only — only ever the active source; never considers others. - best_fit — previous behavior: whichever source matches the files best. resolve_canonical_for_album gains mode + primary_source; the orchestration threads the primary source through; the job reads source_selection from its settings. Note: active_preferred respects the active source as long as it clears the floor, so it will NOT override a deluxe-vs-standard mismatch on the primary (#767-Bug2) — that's what best_fit is for; the choice is now the user's. Tests: per-mode coverage in test_canonical_resolver.py (active_preferred uses primary when it fits, falls back when primary is below floor, keeps primary even when another fits better; active_only pins primary / never falls back; best_fit unchanged), orchestration default-mode test, and the setting default. 39 canonical tests pass. --- core/metadata/canonical_resolver.py | 78 +++++++++++++++---- core/repair_jobs/canonical_version_resolve.py | 8 +- tests/test_canonical_orchestration.py | 17 +++- tests/test_canonical_resolver.py | 77 +++++++++++++++++- tests/test_canonical_version_job.py | 6 +- 5 files changed, 162 insertions(+), 24 deletions(-) diff --git a/core/metadata/canonical_resolver.py b/core/metadata/canonical_resolver.py index 15a1b279..86ee6e51 100644 --- a/core/metadata/canonical_resolver.py +++ b/core/metadata/canonical_resolver.py @@ -17,7 +17,16 @@ from __future__ import annotations from typing import Any, Callable, Dict, List, Optional -from core.metadata.canonical_version import pick_canonical_release +from core.metadata.canonical_version import ( + pick_canonical_release, + score_release_against_files, +) + +# Source-selection modes (a per-job setting). See resolve_canonical_for_album. +MODE_ACTIVE_PREFERRED = "active_preferred" # default: use the active source if it fits, else best-fit +MODE_ACTIVE_ONLY = "active_only" # only ever the active source +MODE_BEST_FIT = "best_fit" # whichever source fits the files best +VALID_MODES = (MODE_ACTIVE_PREFERRED, MODE_ACTIVE_ONLY, MODE_BEST_FIT) def resolve_canonical_for_album( @@ -27,38 +36,68 @@ def resolve_canonical_for_album( fetch_tracklist: Callable[[str, str], Optional[List[Dict[str, Any]]]], source_priority: List[str], min_score: float = 0.5, + mode: str = MODE_ACTIVE_PREFERRED, + primary_source: Optional[str] = None, ) -> Optional[Dict[str, Any]]: - """Pick the canonical release for one album. + """Pick the canonical release for one album, honoring the source-selection mode. ``album_source_ids``: ``{source: album_id}`` the album is linked to. ``file_tracks``: on-disk track metadata (``{duration_ms, title}``). ``fetch_tracklist(source, album_id)``: returns that release's tracklist (or None/[] on miss); injected so callers supply ``get_album_tracks_for_source`` while tests supply a fake. - ``source_priority``: order to build candidates in — ties break toward the - earlier (higher-priority) source, keeping the choice deterministic. + ``source_priority``: source order; ties break toward the earlier source. + ``primary_source``: the user's active metadata source (defaults to the first + of ``source_priority``). - Returns ``{'source', 'album_id', 'score'}`` for the best fit, or ``None`` - when there are no files, no resolvable candidates, or nothing clears - ``min_score`` (caller leaves the album unresolved → tools fall back).""" + Modes: + - ``active_preferred`` (default): use the active source's release when the + album has an ID for it AND it clears ``min_score``; otherwise fall back + to the best-fit among the remaining sources. So it normally respects the + user's configured source but self-heals when that link is clearly wrong. + - ``active_only``: only ever the active source (pinned if it clears the + floor; never considers other sources). + - ``best_fit``: whichever source's release best matches the files. + + Returns ``{'source', 'album_id', 'score'}`` or ``None`` when there are no + files, no resolvable candidates, or nothing clears ``min_score``.""" if not file_tracks: return None + primary = primary_source or (source_priority[0] if source_priority else None) - candidates: List[Dict[str, Any]] = [] - for source in source_priority: + def _candidate(source: Optional[str]) -> Optional[Dict[str, Any]]: + if not source: + return None album_id = album_source_ids.get(source) if not album_id: - continue + return None try: tracks = fetch_tracklist(source, str(album_id)) except Exception: tracks = None - if tracks: - candidates.append({ - 'source': source, - 'album_id': str(album_id), - 'tracks': tracks, - }) + if not tracks: + return None + return {'source': source, 'album_id': str(album_id), 'tracks': tracks} + + # Active-source modes: try the primary first. + if mode in (MODE_ACTIVE_ONLY, MODE_ACTIVE_PREFERRED): + cand = _candidate(primary) + if cand: + score = score_release_against_files(file_tracks, cand['tracks']) + if score >= min_score: + return {'source': cand['source'], 'album_id': cand['album_id'], 'score': round(score, 4)} + if mode == MODE_ACTIVE_ONLY: + return None # never fall back to other sources + + # best_fit (and active_preferred's fallback): score the candidate sources + # (skipping the primary we already tried in active_preferred) and pick best. + candidates: List[Dict[str, Any]] = [] + for source in source_priority: + if mode == MODE_ACTIVE_PREFERRED and source == primary: + continue + cand = _candidate(source) + if cand: + candidates.append(cand) if not candidates: return None @@ -111,6 +150,7 @@ def resolve_and_store_canonical_for_album( source_priority: Optional[List[str]] = None, min_score: float = 0.5, store: bool = True, + mode: str = MODE_ACTIVE_PREFERRED, ) -> Optional[Dict[str, Any]]: """Gather an album's source IDs + its tracks' (duration, title) from the DB, resolve the best-fit canonical release, and (when ``store``) persist it. @@ -138,10 +178,12 @@ def resolve_and_store_canonical_for_album( if fetch_tracklist is None: fetch_tracklist = default_fetch_tracklist + primary_source = None if source_priority is None: try: from core.metadata_service import get_primary_source, get_source_priority - source_priority = get_source_priority(get_primary_source()) + primary_source = get_primary_source() + source_priority = get_source_priority(primary_source) except Exception: source_priority = list(source_ids.keys()) @@ -151,6 +193,8 @@ def resolve_and_store_canonical_for_album( fetch_tracklist=fetch_tracklist, source_priority=source_priority, min_score=min_score, + mode=mode, + primary_source=primary_source, ) if result and store: db.set_album_canonical(album_id, result['source'], result['album_id'], result['score']) diff --git a/core/repair_jobs/canonical_version_resolve.py b/core/repair_jobs/canonical_version_resolve.py index 1323eb11..79c4aaef 100644 --- a/core/repair_jobs/canonical_version_resolve.py +++ b/core/repair_jobs/canonical_version_resolve.py @@ -47,6 +47,11 @@ class CanonicalVersionResolveJob(RepairJob): default_settings = { 'dry_run': True, 'min_score': 0.5, + # Which source's release to pin: 'active_preferred' (default — use your + # active metadata source when it fits, else best-fit fallback), + # 'active_only' (only ever the active source), or 'best_fit' (whichever + # source matches the files best, regardless of which it is). + 'source_selection': 'active_preferred', } auto_fix = True @@ -81,6 +86,7 @@ class CanonicalVersionResolveJob(RepairJob): settings = self._get_settings(context) dry_run = settings.get('dry_run', True) min_score = settings.get('min_score', 0.5) + mode = settings.get('source_selection', 'active_preferred') active_server = None if context.config_manager: @@ -115,7 +121,7 @@ class CanonicalVersionResolveJob(RepairJob): try: resolved = resolve_and_store_canonical_for_album( - context.db, album_id, min_score=min_score, store=not dry_run, + context.db, album_id, min_score=min_score, store=not dry_run, mode=mode, ) except Exception as e: logger.warning("Canonical resolve failed for album %s ('%s'): %s", diff --git a/tests/test_canonical_orchestration.py b/tests/test_canonical_orchestration.py index 2f3ae8a0..44ae1b76 100644 --- a/tests/test_canonical_orchestration.py +++ b/tests/test_canonical_orchestration.py @@ -47,14 +47,29 @@ def test_resolve_and_store_picks_best_fit_and_persists(tmp_path): db, album_id, fetch_tracklist=lambda s, a: table.get((s, a)), source_priority=["spotify", "deezer"], + mode="best_fit", ) - # Deezer's standard matches the 11 files better than Spotify's deluxe. + # best_fit: Deezer's standard matches the 11 files better than Spotify's deluxe. assert out["source"] == "deezer" and out["album_id"] == "dz_std" # ...and it was persisted. stored = db.get_album_canonical(album_id) assert stored["source"] == "deezer" and stored["album_id"] == "dz_std" +def test_default_mode_prefers_active_source(tmp_path): + # Same setup, but default (active_preferred) mode: primary = spotify, whose + # deluxe still clears the floor -> pinned, even though deezer fits better. + db = MusicDatabase(str(tmp_path / "m.db")) + album_id = _seed(db, spotify="sp_deluxe", deezer="dz_std") + table = {("spotify", "sp_deluxe"): DLX, ("deezer", "dz_std"): STD} + out = resolve_and_store_canonical_for_album( + db, album_id, + fetch_tracklist=lambda s, a: table.get((s, a)), + source_priority=["spotify", "deezer"], # default mode + ) + assert out["source"] == "spotify" # active source preferred + + def test_resolve_returns_none_when_album_has_no_source_ids(tmp_path): db = MusicDatabase(str(tmp_path / "m.db")) album_id = _seed(db, spotify=None, deezer=None) diff --git a/tests/test_canonical_resolver.py b/tests/test_canonical_resolver.py index 2868a373..a6e7839e 100644 --- a/tests/test_canonical_resolver.py +++ b/tests/test_canonical_resolver.py @@ -17,10 +17,10 @@ def _fetcher(table): return fetch -def test_picks_source_whose_release_fits_the_files(): +def test_best_fit_mode_picks_best_regardless_of_priority(): files = list(STD) # user owns the 11-track standard table = { - ("spotify", "sp_deluxe"): DLX, # spotify linked to deluxe (17) + ("spotify", "sp_deluxe"): DLX, # spotify (primary) linked to deluxe (17) ("musicbrainz", "mb_std"): STD, # musicbrainz has standard (11) } out = resolve_canonical_for_album( @@ -28,12 +28,81 @@ def test_picks_source_whose_release_fits_the_files(): file_tracks=files, fetch_tracklist=_fetcher(table), source_priority=PRIORITY, + mode="best_fit", ) - # Best FIT wins over priority — standard matches the files, deluxe doesn't. - assert out == {"source": "musicbrainz", "album_id": "mb_std", "score": out["score"]} + # best_fit: standard matches the files, deluxe doesn't — fit beats priority. + assert out["source"] == "musicbrainz" and out["album_id"] == "mb_std" assert out["score"] > 0.9 +# ── source-selection modes ──────────────────────────────────────────────── + +def test_active_preferred_uses_primary_when_it_fits(): + files = list(STD) + table = {("spotify", "sp1"): STD, ("musicbrainz", "mb1"): STD} # both fit + out = resolve_canonical_for_album( + album_source_ids={"spotify": "sp1", "musicbrainz": "mb1"}, + file_tracks=files, fetch_tracklist=_fetcher(table), + source_priority=PRIORITY, # primary = spotify + ) # default mode = active_preferred + assert out["source"] == "spotify" + + +def test_active_preferred_falls_back_when_primary_clearly_misfits(): + files = list(STD) # 11 tracks + table = { + ("spotify", "sp_bad"): [{"duration_ms": 60_000, "title": "X"}] * 3, # 3-track, fall back to the fitting source. + assert out["source"] == "musicbrainz" + + +def test_active_preferred_keeps_primary_even_if_another_fits_better(): + files = list(STD) + # primary spotify is a deluxe (decent fit, above floor); musicbrainz is exact. + table = {("spotify", "sp_dlx"): DLX, ("musicbrainz", "mb_std"): STD} + out = resolve_canonical_for_album( + album_source_ids={"spotify": "sp_dlx", "musicbrainz": "mb_std"}, + file_tracks=files, fetch_tracklist=_fetcher(table), + source_priority=PRIORITY, mode="active_preferred", + ) + # active_preferred respects the active source as long as it clears the floor, + # even though musicbrainz would fit better (use best_fit for that). + assert out["source"] == "spotify" + + +def test_active_only_pins_primary_and_never_falls_back(): + files = list(STD) + # primary spotify is below floor; a perfect musicbrainz exists but is ignored. + table = { + ("spotify", "sp_bad"): [{"duration_ms": 60_000, "title": "X"}] * 3, + ("musicbrainz", "mb_std"): STD, + } + out = resolve_canonical_for_album( + album_source_ids={"spotify": "sp_bad", "musicbrainz": "mb_std"}, + file_tracks=files, fetch_tracklist=_fetcher(table), + source_priority=PRIORITY, mode="active_only", + ) + assert out is None # primary didn't fit, and active_only won't consider others + + +def test_active_only_pins_primary_when_it_fits(): + files = list(STD) + table = {("spotify", "sp1"): STD, ("musicbrainz", "mb1"): STD} + out = resolve_canonical_for_album( + album_source_ids={"spotify": "sp1", "musicbrainz": "mb1"}, + file_tracks=files, fetch_tracklist=_fetcher(table), + source_priority=PRIORITY, mode="active_only", + ) + assert out["source"] == "spotify" + + def test_priority_breaks_tie_between_equal_fits(): files = list(STD) table = {("spotify", "a"): STD, ("itunes", "b"): STD} # identical fit diff --git a/tests/test_canonical_version_job.py b/tests/test_canonical_version_job.py index 32c20e3f..cf04cd29 100644 --- a/tests/test_canonical_version_job.py +++ b/tests/test_canonical_version_job.py @@ -33,7 +33,7 @@ def _seed_two_albums(db): def _fake_resolver(monkeypatch): - def fake(db, album_id, *, min_score=0.5, store=True): + def fake(db, album_id, *, min_score=0.5, store=True, mode="active_preferred"): res = {"source": "spotify", "album_id": f"sp_{album_id}", "score": 0.9} if store: db.set_album_canonical(album_id, res["source"], res["album_id"], res["score"]) @@ -52,6 +52,10 @@ def test_job_is_opt_in_and_dry_run_by_default(): assert CanonicalVersionResolveJob.default_settings["dry_run"] is True +def test_source_selection_defaults_to_active_preferred(): + assert CanonicalVersionResolveJob.default_settings["source_selection"] == "active_preferred" + + def test_live_resolves_and_stores(tmp_path, monkeypatch): db = MusicDatabase(str(tmp_path / "m.db")) _seed_two_albums(db)