diff --git a/README.md b/README.md index d70690a..c8382f6 100644 --- a/README.md +++ b/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 -MusicBrainz id where Lidarr has one — and refreshed every ninety days. An -artist Last.fm has never heard of is recorded as fetched with no tags, so it is -not asked about again on every pass. `--tag-limit` spreads the first sweep over -several passes. +Tags are fetched once per artist — one `artist.getTopTags` call each — and +refreshed every ninety days. `--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: diff --git a/music_curator.py b/music_curator.py index 4b78036..03687f0 100644 --- a/music_curator.py +++ b/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) - tagged += 1 + store.replace_tags(artist["norm_name"], pairs, now) + resolved += 1 + if not pairs: + unknown += 1 - logger.info("tagged %d artists", tagged) - return tagged + logger.info("tagged %d artists (%d with nothing to say about them)", resolved, unknown) + return resolved def build_vibe_playlists(store, vibes, mirror_root, library_root, limit, now): diff --git a/tests/conftest.py b/tests/conftest.py index f9ab7df..d40f52b 100644 --- a/tests/conftest.py +++ b/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): diff --git a/tests/test_music_curator.py b/tests/test_music_curator.py index c6443fc..d47d353 100644 --- a/tests/test_music_curator.py +++ b/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}], - "artist-mbid-2": [{"name": "classic rock", "count": 100}], + "Played Band": [{"name": "screamo", "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