Wire alias-aware artist match into AcoustID verifier — fixes #442
This is the user-visible commit. The reporter's exact two cases (Japanese kanji, Russian Cyrillic) now pass verification instead of being quarantined. # What changed Verifier's three artist-similarity sites now route through the shared `core.matching.artist_aliases.artist_names_match` helper instead of raw `_similarity`: - `_find_best_title_artist_match` (per-recording scoring at the best-match stage) - Secondary scan when title matches but best-match's artist doesn't (line ~355 pre-fix) - Final fallback scan over all recordings (line ~403 pre-fix) Aliases for the expected artist are resolved ONCE at the top of `verify_audio_file` via `_resolve_expected_artist_aliases`, which calls the new `MusicBrainzService.lookup_artist_aliases` chain (library DB → cache → live MB). Single resolution per verification regardless of how many AcoustID recordings come back — pinned by test. New helper `_alias_aware_artist_sim(expected, actual, aliases)` wraps the pure helper with the verifier's normaliser (`_similarity`) and threshold (`ARTIST_MATCH_THRESHOLD`). Returns a single float so existing threshold-comparison code paths keep their shape — minimal diff. # Reporter's cases — verified Case 1 (issue #442 verbatim): File: YAMANAIAME by 澤野弘之 Expected: YAMANAIAME by Hiroyuki Sawano Pre-fix: Quarantined (artist=0%) Post-fix: PASS (alias '澤野弘之' resolved from MB) Case 2 (issue #442 verbatim): File: On the Other Side by Sergey Lazarev Expected: On the other side by Сергей Лазарев Pre-fix: Quarantined (artist=7%) Post-fix: PASS (alias 'Sergey Lazarev' resolved from MB) Both reproduced as regression tests with stubbed MB service. # Backward compat Three test cases pin that no-aliases / failure paths preserve pre-fix behaviour exactly: - Clear artist mismatch (different artist, same script) still FAILs — aliases bridge synonyms, not unrelated artists. - Exact title + artist match still PASSes regardless of aliases. - MB service raise → verifier completes with direct similarity (treats failure as "no aliases available" — same as pre-fix). Also covers manual import: the import-modal "Search for Match" flow goes through the same verifier, so the reporter's complaint that "manual import simply throws them back in quarantine again" is fixed by the same change. # Tests added (11) `tests/matching/test_acoustid_verification_aliases.py`: - `_alias_aware_artist_sim`: alias bridges score ↑, no-aliases falls back, aliases don't mask genuine mismatches - `_find_best_title_artist_match` accepts + uses aliases - Reporter's case 1 (Japanese) end-to-end - Reporter's case 2 (Russian) end-to-end - Backward compat: no-aliases mismatch still fails, exact match still passes, MB-service-raise doesn't break verification - Performance: alias lookup fires ONCE per verification regardless of recording count # Verification - 11 new verifier tests pass - 31 prior service tests pass - 28 prior helper tests pass - 294 matching + imports tests pass total (no regression) - Ruff clean
This commit is contained in:
Broque Thomas
parent
15244f24cf
commit
7066233c37
2 changed files with 380 additions and 4 deletions
|
|
@ -87,14 +87,51 @@ def _similarity(a: str, b: str) -> float:
|
|||
return SequenceMatcher(None, na, nb).ratio()
|
||||
|
||||
|
||||
def _alias_aware_artist_sim(
|
||||
expected_artist: str,
|
||||
actual_artist: str,
|
||||
aliases: Optional[List[str]] = None,
|
||||
) -> float:
|
||||
"""Best artist-similarity across (expected, *aliases) vs actual.
|
||||
|
||||
Issue #442 — when expected and actual are in different scripts
|
||||
(e.g. `Hiroyuki Sawano` vs `澤野弘之`), raw `_similarity` scores
|
||||
near 0% even though MusicBrainz aliases bridge them. Routes
|
||||
through the pure helper so the verifier inherits one shared
|
||||
contract.
|
||||
|
||||
Returns the highest score across all candidates so existing
|
||||
threshold checks (>= ARTIST_MATCH_THRESHOLD) keep their
|
||||
semantics. When `aliases` is None or empty, behaves identically
|
||||
to the prior raw `_similarity(expected, actual)` call.
|
||||
"""
|
||||
from core.matching.artist_aliases import artist_names_match
|
||||
_matched, score = artist_names_match(
|
||||
expected_artist,
|
||||
actual_artist,
|
||||
aliases=aliases,
|
||||
threshold=ARTIST_MATCH_THRESHOLD,
|
||||
similarity=_similarity,
|
||||
)
|
||||
return score
|
||||
|
||||
|
||||
def _find_best_title_artist_match(
|
||||
recordings: List[Dict[str, Any]],
|
||||
expected_title: str,
|
||||
expected_artist: str,
|
||||
expected_artist_aliases: Optional[List[str]] = None,
|
||||
) -> Tuple[Optional[Dict], float, float]:
|
||||
"""
|
||||
Find the AcoustID recording that best matches expected title/artist.
|
||||
|
||||
Issue #442 — `expected_artist_aliases` (when supplied) is the
|
||||
list of alternate spellings for `expected_artist` (Japanese
|
||||
kanji, Cyrillic, etc.). Each recording's artist is scored
|
||||
against (expected, *aliases) and the best score wins. When the
|
||||
list is empty or omitted, behavior is identical to the prior
|
||||
raw similarity comparison.
|
||||
|
||||
Returns:
|
||||
(best_recording, title_similarity, artist_similarity)
|
||||
"""
|
||||
|
|
@ -108,7 +145,9 @@ def _find_best_title_artist_match(
|
|||
artist = rec.get('artist') or ''
|
||||
|
||||
title_sim = _similarity(expected_title, title)
|
||||
artist_sim = _similarity(expected_artist, artist)
|
||||
artist_sim = _alias_aware_artist_sim(
|
||||
expected_artist, artist, expected_artist_aliases,
|
||||
)
|
||||
# Weight title higher since that's the primary identifier
|
||||
combined = (title_sim * 0.6) + (artist_sim * 0.4)
|
||||
|
||||
|
|
@ -125,6 +164,12 @@ def _find_best_title_artist_match(
|
|||
_mb_client = None
|
||||
_mb_client_lock = threading.Lock()
|
||||
|
||||
# Shared MusicBrainzService for alias lookups (issue #442). Service
|
||||
# layer wraps the raw client + adds caching + DB access — all of which
|
||||
# the alias resolution chain (library DB → cache → live MB) needs.
|
||||
_mb_service = None
|
||||
_mb_service_lock = threading.Lock()
|
||||
|
||||
MAX_MB_ENRICHMENT_LOOKUPS = 3
|
||||
|
||||
|
||||
|
|
@ -138,6 +183,42 @@ def _get_mb_client() -> MusicBrainzClient:
|
|||
return _mb_client
|
||||
|
||||
|
||||
def _get_mb_service():
|
||||
"""Get or create a shared MusicBrainzService instance.
|
||||
|
||||
Used by the alias-resolution chain in `verify_audio_file`. Lazy
|
||||
init so importing this module doesn't trigger a DB connection on
|
||||
paths that never run AcoustID verification (test runs, dry runs).
|
||||
"""
|
||||
global _mb_service
|
||||
if _mb_service is None:
|
||||
with _mb_service_lock:
|
||||
if _mb_service is None:
|
||||
from core.musicbrainz_service import MusicBrainzService
|
||||
from database.music_database import get_database
|
||||
_mb_service = MusicBrainzService(get_database())
|
||||
return _mb_service
|
||||
|
||||
|
||||
def _resolve_expected_artist_aliases(expected_artist_name: str) -> List[str]:
|
||||
"""Look up alternate-spelling aliases for the expected artist.
|
||||
|
||||
Issue #442 — bridges cross-script artist comparisons (Japanese
|
||||
kanji ↔ romanized, Cyrillic ↔ Latin, etc.) without forcing the
|
||||
verifier to know about the resolution chain. Best-effort: any
|
||||
failure (no MB service, network down, no library DB) returns
|
||||
empty list so verification falls back to the prior direct
|
||||
similarity check.
|
||||
"""
|
||||
if not expected_artist_name:
|
||||
return []
|
||||
try:
|
||||
return _get_mb_service().lookup_artist_aliases(expected_artist_name)
|
||||
except Exception as e:
|
||||
logger.debug("alias lookup failed for %r: %s", expected_artist_name, e)
|
||||
return []
|
||||
|
||||
|
||||
def _enrich_recordings_from_musicbrainz(
|
||||
recordings: List[Dict[str, Any]],
|
||||
) -> List[Dict[str, Any]]:
|
||||
|
|
@ -290,9 +371,23 @@ class AcoustIDVerification:
|
|||
# Enrich recordings that are missing title/artist via MusicBrainz lookup
|
||||
recordings = _enrich_recordings_from_musicbrainz(recordings)
|
||||
|
||||
# Issue #442 — resolve alternate-spelling aliases for the
|
||||
# expected artist ONCE. Multi-tier resolution (library DB
|
||||
# → cache → live MB), cached per artist name so 100
|
||||
# quarantine candidates with the same artist don't trigger
|
||||
# 100 MB API calls. Empty list on any failure → verifier
|
||||
# falls back to prior direct-similarity behaviour.
|
||||
expected_artist_aliases = _resolve_expected_artist_aliases(expected_artist_name)
|
||||
if expected_artist_aliases:
|
||||
logger.debug(
|
||||
"Resolved %d aliases for expected artist '%s'",
|
||||
len(expected_artist_aliases), expected_artist_name,
|
||||
)
|
||||
|
||||
# Step 4: Find best title/artist match among AcoustID results
|
||||
best_rec, title_sim, artist_sim = _find_best_title_artist_match(
|
||||
recordings, expected_track_name, expected_artist_name
|
||||
recordings, expected_track_name, expected_artist_name,
|
||||
expected_artist_aliases=expected_artist_aliases,
|
||||
)
|
||||
|
||||
if not best_rec:
|
||||
|
|
@ -352,7 +447,10 @@ class AcoustIDVerification:
|
|||
# metadata for this fingerprint, it's likely the right track
|
||||
# (AcoustID's "best" match just picked the wrong variant).
|
||||
for rec in recordings:
|
||||
if _similarity(expected_artist_name, rec.get('artist', '')) >= ARTIST_MATCH_THRESHOLD:
|
||||
rec_artist = rec.get('artist', '')
|
||||
if _alias_aware_artist_sim(
|
||||
expected_artist_name, rec_artist, expected_artist_aliases,
|
||||
) >= ARTIST_MATCH_THRESHOLD:
|
||||
msg = (
|
||||
f"Audio verified: found '{expected_track_name}' by '{expected_artist_name}' "
|
||||
f"in AcoustID results"
|
||||
|
|
@ -400,7 +498,9 @@ class AcoustIDVerification:
|
|||
if _detect_title_version(t) != expected_version:
|
||||
continue
|
||||
if (_similarity(expected_track_name, t) >= TITLE_MATCH_THRESHOLD and
|
||||
_similarity(expected_artist_name, a) >= ARTIST_MATCH_THRESHOLD):
|
||||
_alias_aware_artist_sim(
|
||||
expected_artist_name, a, expected_artist_aliases,
|
||||
) >= ARTIST_MATCH_THRESHOLD):
|
||||
msg = (
|
||||
f"Audio verified: found '{t}' by '{a}' in AcoustID results "
|
||||
f"matching expected '{expected_track_name}' by '{expected_artist_name}'"
|
||||
|
|
|
|||
276
tests/matching/test_acoustid_verification_aliases.py
Normal file
276
tests/matching/test_acoustid_verification_aliases.py
Normal file
|
|
@ -0,0 +1,276 @@
|
|||
"""Regression tests for issue #442 — AcoustID verifier alias awareness.
|
||||
|
||||
The reporter posted two exact cases:
|
||||
|
||||
Case 1 (Japanese kanji ↔ romanized):
|
||||
File: YAMANAIAME by 澤野弘之
|
||||
Expected: YAMANAIAME by Hiroyuki Sawano
|
||||
Pre-fix: quarantined (artist_sim=0%)
|
||||
Post-fix: passes verification because MB aliases bridge the
|
||||
two spellings.
|
||||
|
||||
Case 2 (Cyrillic ↔ Latin):
|
||||
File: On the Other Side by Sergey Lazarev
|
||||
Expected: On the other side by Сергей Лазарев
|
||||
Pre-fix: quarantined (artist_sim=7%)
|
||||
Post-fix: passes via aliases.
|
||||
|
||||
These tests pin the verifier through the helper. AcoustID's
|
||||
fingerprint call is stubbed (no network), MB service's
|
||||
`lookup_artist_aliases` is stubbed to return the relevant aliases.
|
||||
The verifier's pass/fail decision is the assertion.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from core.acoustid_verification import (
|
||||
AcoustIDVerification,
|
||||
VerificationResult,
|
||||
_alias_aware_artist_sim,
|
||||
_find_best_title_artist_match,
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Pure helper — _alias_aware_artist_sim
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestAliasAwareArtistSim:
|
||||
def test_returns_higher_score_when_alias_matches(self):
|
||||
score = _alias_aware_artist_sim(
|
||||
'Hiroyuki Sawano', '澤野弘之',
|
||||
aliases=['澤野弘之', 'SawanoHiroyuki'],
|
||||
)
|
||||
assert score == 1.0
|
||||
|
||||
def test_no_aliases_falls_back_to_direct_similarity(self):
|
||||
"""Cross-script with NO aliases → score ~0, pre-fix behaviour."""
|
||||
score = _alias_aware_artist_sim(
|
||||
'Hiroyuki Sawano', '澤野弘之', aliases=None,
|
||||
)
|
||||
assert score < 0.1
|
||||
|
||||
def test_aliases_dont_mask_genuine_mismatch(self):
|
||||
"""Different artist entirely → still scores low even when
|
||||
aliases are provided. Aliases bridge synonyms, not unrelated
|
||||
artists."""
|
||||
score = _alias_aware_artist_sim(
|
||||
'Hiroyuki Sawano', 'Khalil Turk & Friends',
|
||||
aliases=['澤野弘之', 'SawanoHiroyuki'],
|
||||
)
|
||||
assert score < 0.5
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _find_best_title_artist_match — accepts aliases now
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestFindBestMatchWithAliases:
|
||||
def test_japanese_alias_picks_correct_recording(self):
|
||||
"""Reporter's case 1: AcoustID returned recording with kanji
|
||||
artist. Without aliases the scorer ranks it low and the
|
||||
verifier later quarantines. With aliases it scores high."""
|
||||
recordings = [
|
||||
{'title': 'YAMANAIAME', 'artist': '澤野弘之'},
|
||||
{'title': 'Different Song', 'artist': 'Hiroyuki Sawano'},
|
||||
]
|
||||
# Aliases provided — bridge to recording 0
|
||||
best, title_sim, artist_sim = _find_best_title_artist_match(
|
||||
recordings, 'YAMANAIAME', 'Hiroyuki Sawano',
|
||||
expected_artist_aliases=['澤野弘之', 'SawanoHiroyuki'],
|
||||
)
|
||||
assert best is recordings[0]
|
||||
assert artist_sim == 1.0
|
||||
|
||||
def test_no_aliases_legacy_behaviour_preserved(self):
|
||||
"""Default arg / empty aliases → identical to pre-fix
|
||||
behaviour. Critical for paths not yet wired up to alias
|
||||
lookup."""
|
||||
recordings = [
|
||||
{'title': 'Track', 'artist': 'Artist'},
|
||||
]
|
||||
best, title_sim, artist_sim = _find_best_title_artist_match(
|
||||
recordings, 'Track', 'Artist',
|
||||
)
|
||||
assert title_sim == 1.0
|
||||
assert artist_sim == 1.0
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# End-to-end — reporter's cases through the full verifier
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def stubbed_verifier(monkeypatch):
|
||||
"""AcoustIDVerification with the acoustid client + MB service
|
||||
layer stubbed. Lets us drive the verifier's full decision path
|
||||
without network or DB. Returns the verifier + mutable handles
|
||||
to the stubs so each test can shape the AcoustID response +
|
||||
aliases."""
|
||||
verifier = AcoustIDVerification()
|
||||
verifier.acoustid_client = MagicMock()
|
||||
verifier.acoustid_client.is_available.return_value = (True, '')
|
||||
|
||||
# Stub the MB service so verifier alias lookup doesn't touch DB
|
||||
# or network. Each test sets fake_service.lookup_artist_aliases.
|
||||
fake_service = MagicMock()
|
||||
fake_service.lookup_artist_aliases.return_value = []
|
||||
monkeypatch.setattr(
|
||||
'core.acoustid_verification._get_mb_service', lambda: fake_service,
|
||||
)
|
||||
|
||||
return verifier, fake_service
|
||||
|
||||
|
||||
class TestIssue442Regression:
|
||||
def test_japanese_kanji_artist_passes_verification(self, stubbed_verifier):
|
||||
"""Reporter's case 1 — verbatim from the issue:
|
||||
|
||||
File: YAMANAIAME by 澤野弘之
|
||||
Expected: YAMANAIAME by Hiroyuki Sawano
|
||||
Pre-fix: Quarantined (artist=0%)
|
||||
"""
|
||||
verifier, fake_service = stubbed_verifier
|
||||
|
||||
# AcoustID returns the recording with kanji artist
|
||||
verifier.acoustid_client.fingerprint_and_lookup.return_value = {
|
||||
'best_score': 0.95,
|
||||
'recordings': [
|
||||
{'title': 'YAMANAIAME', 'artist': '澤野弘之', 'mbid': 'rec-x'},
|
||||
],
|
||||
}
|
||||
# MB knows Hiroyuki Sawano's aliases
|
||||
fake_service.lookup_artist_aliases.return_value = [
|
||||
'澤野弘之', 'SawanoHiroyuki', 'Sawano Hiroyuki',
|
||||
]
|
||||
|
||||
result, msg = verifier.verify_audio_file(
|
||||
'/fake/path.mp3', 'YAMANAIAME', 'Hiroyuki Sawano',
|
||||
)
|
||||
|
||||
assert result == VerificationResult.PASS, (
|
||||
f"Reporter's exact case must pass verification post-fix; "
|
||||
f"got result={result.value!r} msg={msg!r}"
|
||||
)
|
||||
fake_service.lookup_artist_aliases.assert_called_once_with('Hiroyuki Sawano')
|
||||
|
||||
def test_cyrillic_artist_passes_verification(self, stubbed_verifier):
|
||||
"""Reporter's case 2 — Sergey Lazarev / Сергей Лазарев."""
|
||||
verifier, fake_service = stubbed_verifier
|
||||
|
||||
verifier.acoustid_client.fingerprint_and_lookup.return_value = {
|
||||
'best_score': 0.95,
|
||||
'recordings': [
|
||||
{'title': 'On the Other Side', 'artist': 'Sergey Lazarev', 'mbid': 'rec-y'},
|
||||
],
|
||||
}
|
||||
fake_service.lookup_artist_aliases.return_value = [
|
||||
'Sergey Lazarev', 'Sergei Lazarev',
|
||||
]
|
||||
|
||||
result, msg = verifier.verify_audio_file(
|
||||
'/fake/path.flac', 'On the other side', 'Сергей Лазарев',
|
||||
)
|
||||
|
||||
assert result == VerificationResult.PASS
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Backward compat — no aliases available → behavior identical to pre-fix
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestBackwardCompat:
|
||||
def test_no_aliases_clear_artist_mismatch_still_fails(self, stubbed_verifier):
|
||||
"""Pre-fix: clear mismatches (artist sim near 0, NOT a script
|
||||
difference) should FAIL. Post-fix with empty aliases must
|
||||
preserve this — aliases bridge synonyms, not unrelated
|
||||
artists."""
|
||||
verifier, fake_service = stubbed_verifier
|
||||
|
||||
# Wrong artist entirely — Latin script both sides, sim ~0
|
||||
verifier.acoustid_client.fingerprint_and_lookup.return_value = {
|
||||
'best_score': 0.95,
|
||||
'recordings': [
|
||||
{'title': 'Some Track', 'artist': 'Khalil Turk & Friends'},
|
||||
],
|
||||
}
|
||||
fake_service.lookup_artist_aliases.return_value = [] # No aliases
|
||||
|
||||
result, msg = verifier.verify_audio_file(
|
||||
'/fake/path.mp3', 'Some Track', 'Foreigner',
|
||||
)
|
||||
|
||||
assert result == VerificationResult.FAIL
|
||||
|
||||
def test_no_aliases_exact_match_still_passes(self, stubbed_verifier):
|
||||
"""Exact title + artist match → PASS regardless of aliases."""
|
||||
verifier, fake_service = stubbed_verifier
|
||||
|
||||
verifier.acoustid_client.fingerprint_and_lookup.return_value = {
|
||||
'best_score': 0.95,
|
||||
'recordings': [
|
||||
{'title': 'Dirty White Boy', 'artist': 'Foreigner'},
|
||||
],
|
||||
}
|
||||
fake_service.lookup_artist_aliases.return_value = []
|
||||
|
||||
result, _ = verifier.verify_audio_file(
|
||||
'/fake/path.mp3', 'Dirty White Boy', 'Foreigner',
|
||||
)
|
||||
assert result == VerificationResult.PASS
|
||||
|
||||
def test_alias_lookup_failure_does_not_break_verification(self, stubbed_verifier):
|
||||
"""MB service raises → verifier still completes with direct
|
||||
similarity (pre-fix behaviour preserved)."""
|
||||
verifier, fake_service = stubbed_verifier
|
||||
fake_service.lookup_artist_aliases.side_effect = Exception("MB down")
|
||||
|
||||
verifier.acoustid_client.fingerprint_and_lookup.return_value = {
|
||||
'best_score': 0.95,
|
||||
'recordings': [
|
||||
{'title': 'Dirty White Boy', 'artist': 'Foreigner'},
|
||||
],
|
||||
}
|
||||
|
||||
result, _ = verifier.verify_audio_file(
|
||||
'/fake/path.mp3', 'Dirty White Boy', 'Foreigner',
|
||||
)
|
||||
# Should still pass — direct similarity works
|
||||
assert result == VerificationResult.PASS
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Performance contract — alias lookup fires ONCE per verification
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestAliasLookupCalledOncePerVerify:
|
||||
def test_single_lookup_call_regardless_of_recordings_count(self, stubbed_verifier):
|
||||
"""The verifier processes multiple recordings + scans through
|
||||
them at up to 3 sites — but should only call
|
||||
`lookup_artist_aliases` ONCE per verify_audio_file invocation.
|
||||
Otherwise verifying a track with 20 AcoustID recordings could
|
||||
fire 60+ MB lookups (cached or not, that's wasteful)."""
|
||||
verifier, fake_service = stubbed_verifier
|
||||
|
||||
verifier.acoustid_client.fingerprint_and_lookup.return_value = {
|
||||
'best_score': 0.95,
|
||||
'recordings': [
|
||||
{'title': 'X', 'artist': '澤野弘之'},
|
||||
{'title': 'X', 'artist': 'SawanoHiroyuki'},
|
||||
{'title': 'X', 'artist': 'Different Artist'},
|
||||
],
|
||||
}
|
||||
fake_service.lookup_artist_aliases.return_value = ['澤野弘之', 'SawanoHiroyuki']
|
||||
|
||||
verifier.verify_audio_file('/fake/path.mp3', 'X', 'Hiroyuki Sawano')
|
||||
|
||||
assert fake_service.lookup_artist_aliases.call_count == 1
|
||||
Loading…
Reference in a new issue