Canonical: make source selection a job setting (default active-preferred)
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.
This commit is contained in:
parent
e40b328a94
commit
57e039e34d
5 changed files with 162 additions and 24 deletions
|
|
@ -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'])
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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, <floor
|
||||
("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_preferred",
|
||||
)
|
||||
# primary spotify scores below floor -> 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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Reference in a new issue