Merge pull request 'fix: look tags up by artist name rather than by MusicBrainz id' (#11) from fix/tag-lookup-by-name into main

Reviewed-on: #11

README.md

@@ -159,11 +159,20 @@ listening history. MusicBrainz genres arrive free with the Lidarr index and are
 no use for this: they are sparse and formal, and will not tell you a record is
 screamo or synthwave. People typing tags will.
 
-Tags are fetched once per artist — one `artist.getTopTags` call each, by
+Tags are fetched once per artist — one `artist.getTopTags` call each — and
-MusicBrainz id where Lidarr has one — and refreshed every ninety days. An
+refreshed every ninety days. `--tag-limit` spreads the first sweep over several
-artist Last.fm has never heard of is recorded as fetched with no tags, so it is
+passes.
-not asked about again on every pass. `--tag-limit` spreads the first sweep over
+
-several passes.
+Artists are looked up **by name**, not by MusicBrainz id, despite Lidarr having
+an id for every one of them. Last.fm's mbid index is stale and partial: it
+answers "the artist you supplied could not be found" for Devo, Escape the Fate,
+Blasterjaxx and a few hundred others whose pages plainly exist and carry exactly
+the tags wanted. Its name index is the one its own site runs on. The id is kept
+only as a fallback, for a name Lidarr spells differently.
+
+An artist neither key resolves is recorded as fetched with no tags, so the next
+pass does not spend a request on it again. A genuine failure — a rate limit, a
+bad key — is *not* recorded, so that one is retried.
 
 The built-in moods are chosen for this library rather than as a general
 taxonomy:

music_curator.py

@@ -266,7 +266,16 @@ WHITESPACE = re.compile(r"\s+")
 
 
 class LastfmError(Exception):
-    """A Last.fm request that failed in a way retrying will not fix."""
+    """A Last.fm request that failed in a way retrying will not fix.
+
+    `code` is the service's own error number where the failure came from the
+    API rather than the transport. Callers need it to tell "this thing does not
+    exist", which is final, from "something went wrong", which is not.
+    """
+
+    def __init__(self, message, code=None):
+        super().__init__(message)
+        self.code = code
 
 
 class LidarrError(Exception):
@@ -447,7 +456,7 @@ class Lastfm:
                 return payload
             detail = f"error {code}: {payload.get('message', '')}".strip()
             if code not in RETRYABLE_ERRORS:
-                raise LastfmError(f"{method}: {detail}")
+                raise LastfmError(f"{method}: {detail}", code=code)
             self._retry_or_raise(method, attempt, detail, None)
 
         raise LastfmError(f"{method}: gave up after {self.attempts} attempts")
@@ -1323,6 +1332,34 @@ def parse_tags(payload):
     return pairs
 
 
+def fetch_tags(client, name, mbid):
+    """Return an artist's tags, asking by name first.
+
+    By name, not by MusicBrainz id, despite Lidarr having an id for everything.
+    Last.fm's mbid index is stale and partial -- it answers "the artist you
+    supplied could not be found" for Devo, Escape the Fate and a few hundred
+    others whose pages plainly exist -- while the name index is the one its own
+    site runs on. The id is kept only as a fallback for a name Lidarr spells
+    differently.
+
+    Returns an empty list when the artist is genuinely unknown, which the caller
+    records so it is not asked again.
+    """
+    attempts = [{"artist": name}] if name else []
+    if mbid:
+        attempts.append({"mbid": mbid})
+
+    for query in attempts:
+        try:
+            return parse_tags(client.call("artist.getTopTags", {**query, "autocorrect": 1}))
+        except LastfmError as error:
+            # Error 6 here means "no such artist", which the next key may still
+            # answer. Anything else is a real failure and belongs to the caller.
+            if error.code != 6:
+                raise
+    return []
+
+
 def sync_tags(client, store, now, limit=0):
     """Fetch crowd tags for library artists that have none, or stale ones.
 
@@ -1348,20 +1385,23 @@ def sync_tags(client, store, now, limit=0):
     else:
         logger.info("fetching tags for %d artists", len(stale))
 
-    tagged = 0
+    resolved = 0
+    unknown = 0
     for artist in stale:
-        query = {"mbid": artist["mbid"]} if artist["mbid"] else {"artist": artist["name"]}
         try:
-            payload = client.call("artist.getTopTags", {**query, "autocorrect": 1})
+            pairs = fetch_tags(client, artist["name"], artist["mbid"])
         except LastfmError as error:
-            # One artist Last.fm cannot answer for is not worth losing the pass.
+            # Something went wrong rather than the artist not existing. Left
+            # unrecorded on purpose, so the next pass tries again.
             logger.warning("no tags for %s: %s", artist["name"], error)
             continue
-        store.replace_tags(artist["norm_name"], parse_tags(payload), now)
+        store.replace_tags(artist["norm_name"], pairs, now)
-        tagged += 1
+        resolved += 1
+        if not pairs:
+            unknown += 1
 
-    logger.info("tagged %d artists", tagged)
+    logger.info("tagged %d artists (%d with nothing to say about them)", resolved, unknown)
-    return tagged
+    return resolved
 
 
 def build_vibe_playlists(store, vibes, mirror_root, library_root, limit, now):

tests/conftest.py

@@ -86,7 +86,13 @@ class FakeLastfm:
             return json.dumps(self._loved(query))
         if method == "artist.gettoptags":
             key = query.get("mbid") or query.get("artist", "")
-            return json.dumps({"toptags": {"tag": self.tags.get(key, [])}})
+            if key not in self.tags:
+                # What the real service says for a key it cannot resolve, which
+                # for mbids is a great many artists whose pages plainly exist.
+                return json.dumps(
+                    {"error": 6, "message": "The artist you supplied could not be found"}
+                )
+            return json.dumps({"toptags": {"tag": self.tags[key]}})
         raise AssertionError(f"unexpected method {method}")
 
     def _recent(self, query):

tests/test_music_curator.py

@@ -1050,8 +1050,8 @@ def test_a_vibe_selects_by_tag(tmp_path):
     store, source, mirror = tagged_store(
         tmp_path,
         {
-            "artist-mbid-1": [{"name": "screamo", "count": 100}],
+            "Played Band": [{"name": "screamo", "count": 100}],
-            "artist-mbid-2": [{"name": "classic rock", "count": 100}],
+            "Silent Band": [{"name": "classic rock", "count": 100}],
         },
     )
     vibes = [{"name": "screamo", "tags": ["screamo"]}]
@@ -1066,7 +1066,7 @@ def test_a_vibe_selects_by_tag(tmp_path):
 def test_a_weakly_tagged_artist_is_below_the_threshold(tmp_path):
     """A single low-weight tag is not a genre, it is somebody's stray opinion."""
     store, source, mirror = tagged_store(
-        tmp_path, {"artist-mbid-1": [{"name": "screamo", "count": 3}]}
+        tmp_path, {"Played Band": [{"name": "screamo", "count": 3}]}
     )
     vibes = [{"name": "screamo", "tags": ["screamo"]}]
 
@@ -1079,7 +1079,7 @@ def test_a_vibe_can_be_restricted_by_release_year(tmp_path):
     """What separates eighties synth records from everything else a synthpop
     tag drags in."""
     store, source, mirror = tagged_store(
-        tmp_path, {"artist-mbid-1": [{"name": "synthpop", "count": 100}]}
+        tmp_path, {"Played Band": [{"name": "synthpop", "count": 100}]}
     )
     inside = [{"name": "eighties", "tags": ["synthpop"], "years": [1975, 1992]}]
     outside = [{"name": "nineties", "tags": ["synthpop"], "years": [1993, 1999]}]
@@ -1127,3 +1127,74 @@ def test_a_bad_vibes_file_is_refused_up_front(tmp_path, content):
 
     with pytest.raises(ValueError):
         music_curator.load_vibes(str(path))
+
+
+def test_tags_are_looked_up_by_name_not_by_mbid(tmp_path):
+    """Last.fm's mbid index is stale: it cannot find Devo or Escape the Fate by
+    one, though their pages plainly exist. Its name index can."""
+    api, source, mirror = playlist_library(tmp_path)
+    store = store_at(tmp_path)
+    music_curator.index_library(
+        music_curator.Lidarr("http://lidarr", "key", transport=api), store
+    )
+    lastfm = FakeLastfm(tags={"Played Band": [{"name": "screamo", "count": 90}]})
+
+    music_curator.sync_tags(client_for(lastfm), store, NOW)
+
+    asked = [call for call in lastfm.calls if call.get("method") == "artist.getTopTags"]
+    # The name is always tried first; the mbid only appears as a fallback for
+    # the artist that the name could not resolve.
+    assert "artist" in asked[0]
+    assert [call for call in asked if call.get("artist") == "Played Band"]
+    assert not [call for call in asked if call.get("mbid") == "artist-mbid-1"]
+    assert store.scalar("SELECT COUNT(*) FROM artist_tag WHERE tag = 'screamo'") == 1
+
+
+def test_the_mbid_is_tried_when_the_name_is_not_found(tmp_path):
+    """Kept only for a name Lidarr spells differently to Last.fm."""
+    api, source, mirror = playlist_library(tmp_path)
+    store = store_at(tmp_path)
+    music_curator.index_library(
+        music_curator.Lidarr("http://lidarr", "key", transport=api), store
+    )
+    lastfm = FakeLastfm(tags={"artist-mbid-1": [{"name": "dnb", "count": 80}]})
+
+    music_curator.sync_tags(client_for(lastfm), store, NOW)
+
+    assert store.scalar("SELECT COUNT(*) FROM artist_tag WHERE tag = 'dnb'") == 1
+    asked = [call for call in lastfm.calls if call.get("method") == "artist.getTopTags"]
+    assert any("mbid" in call for call in asked)
+
+
+def test_an_artist_neither_key_resolves_is_recorded_and_not_retried(tmp_path):
+    """Otherwise every pass spends a request on it again, for ever."""
+    api, source, mirror = playlist_library(tmp_path)
+    store = store_at(tmp_path)
+    music_curator.index_library(
+        music_curator.Lidarr("http://lidarr", "key", transport=api), store
+    )
+    lastfm = FakeLastfm(tags={})
+
+    assert music_curator.sync_tags(client_for(lastfm), store, NOW) == 2
+    spent = len(lastfm.calls)
+
+    assert music_curator.sync_tags(client_for(lastfm), store, NOW) == 0
+    assert len(lastfm.calls) == spent
+
+
+def test_a_real_failure_is_not_recorded_so_the_next_pass_retries(tmp_path):
+    """A rate limit is not the same as an artist not existing."""
+    api, source, mirror = playlist_library(tmp_path)
+    store = store_at(tmp_path)
+    music_curator.index_library(
+        music_curator.Lidarr("http://lidarr", "key", transport=api), store
+    )
+    lastfm = FakeLastfm(
+        tags={"Played Band": [], "Silent Band": []},
+        outcomes=[{"error": 10, "message": "Invalid API key"}],
+    )
+
+    music_curator.sync_tags(client_for(lastfm), store, NOW)
+
+    # One artist failed hard and must still be pending.
+    assert store.scalar("SELECT COUNT(*) FROM artist_tag_fetched") == 1