Merge pull request 'feat: rebuild the moods from real tags, add exclusions, fix playlist ownership' (#12) from feat/moods-from-real-tags into main

Reviewed-on: #12

README.md

@@ -174,17 +174,62 @@ 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:
+The built-in moods are built from the **tag distribution of this library**,
+measured, rather than from a general taxonomy:
 
 | Mood | Selected on |
-| ------------------ | ------------------------------------------------- |
-| `80s-synths` | synthpop, new wave, synthwave — released 1975-1992 |
-| `high-energy-rock` | hard rock, punk, pop punk, alternative |
-| `screamo` | screamo, post-hardcore, metalcore, emo |
-| `drum-and-bass` | drum and bass, liquid funk, neurofunk, jungle |
-| `dance` | house, big room, hardstyle, trance, dubstep |
-| `classic-rock` | classic rock, prog, psychedelic, blues rock |
+| --------------- | -------------------------------------------------------- |
+| `drum-and-bass` | drum and bass and its six spellings, liquid funk, neurofunk, jungle, techstep, hospital records |
+| `bass` | dubstep, brostep, grime, trip-hop, big beat |
+| `dance` | house and its variants, trance, techno, electro, rave |
+| `pop-punk` | pop punk, punk, emo, emocore, easycore, power pop |
+| `screamo` | screamo, post-hardcore, metalcore, melodic hardcore, trancecore |
+| `heavy-metal` | heavy metal, thrash, speed, power, death, prog, NWOBHM |
+| `hair-metal` | hair metal, glam metal, glam rock, arena rock, AOR — 1975-1994 |
+| `nu-metal` | nu metal, alternative metal, rapcore, industrial |
+| `classic-rock` | classic rock, prog, psychedelic, blues rock, 70s, 60s |
+| `80s-synths` | 80s, new wave, synth pop, electropop, post-punk — 1975-1992, rock excluded |
+| `indie` | indie, indie rock, indie pop, britpop, singer-songwriter |
+
+Measuring first mattered. `synthwave`, `edm`, `big room` and `hardstyle` are
+plausible tags that carry **nothing at all** here, while `techstep`, `easycore`
+and `hospital records` carry real weight. Guessing produces the first list.
+
+Three kinds of tag are never used, and there is a test enforcing it:
+
+- **Nationality** — `american` alone spans 241 artists. A passport is not a
+  sound.
+- **`rock` and `electronic`** — 340 and 275 artists, most of the library. A
+  mood that matches everything is not a mood.
+- **Artist names** — Last.fm's most popular tag for an artist is frequently
+  their own name. `green day`, `paramore` and `queen` are single-artist
+  playlists waiting to happen.
+
+### Exclusions
+
+A mood may also list `exclude`. An excluded tag drops the artist outright rather
+than docking their score, and it exists because `80s-synths` cannot be written
+any other way.
+
+`80s` is the eleventh most-played tag here, and it sits on Def Leppard and Bon
+Jovi exactly as heavily as on Eurythmics. Weighting cannot separate them,
+because the tag it would weight is the one they share. What does separate them
+is that the stadium rock also carries `hard rock` and `hair metal`, and the
+synth acts do not.
+
+Checked against live Last.fm pages, since the tag census only sees artists
+already in the library:
+
+| Artist | Tags |
+| --- | --- |
+| Eurythmics | `80s`, `new wave`, `pop`, `female vocalists`, `synth pop` |
+| Frankie Goes to Hollywood | `80s`, `new wave`, `pop`, `british`, `dance` |
+| Depeche Mode | `electronic`, `synthpop`, `new wave`, `80s`, `synth pop` |
+| Duran Duran | `new wave`, `80s`, `pop`, `synth pop`, `rock` |
+
+Four of Eurythmics' five tags are ones no mood may use. Three of the four
+artists spell it **`synth pop`** with a space; only one spells it `synthpop`.
+Guessing one spelling would have missed most of the canon.
 
 An artist qualifies when their tag weights inside a mood sum to at least 30 out
 of Last.fm's 0-100 scale. One low-weight tag is not a genre, it is somebody's
@@ -216,6 +261,13 @@ in step by hand. Override it if that guess is wrong.
 Entries are written **relative to the playlist file**, so one playlist works
 from the NAS, from a Mac over SMB, and from Linux, without rewriting.
 
+Each playlist is given the **owner and group of the mirror** it is written
+into. The image runs as root by default so that a bind mount of any ownership
+stays writable, and the cost of that is output owned by root — which the account
+serving the share cannot read, group bit or no group bit, because the group is
+also root. Copying the mirror's own ownership avoids having to be told what it
+should be, and does nothing when the two already agree.
+
 A track is only listed once its mirror file has been confirmed to exist. Lidarr
 holding the FLAC says nothing about whether the MP3 has been encoded yet. If a
 large number are missing, the run says so — that is what a wrong `--library-root`

music_curator.py

@@ -1242,49 +1242,108 @@ PLAYLISTS = (
 # `years` filters on the album's release date, which is what separates eighties
 # synth records from everything a synthpop tag would otherwise drag in.
 DEFAULT_VIBES = (
+    # Built from the tag distribution of this library rather than from a general
+    # taxonomy, which is why some obvious-looking tags are absent and some
+    # unobvious ones are here. `synthwave`, `edm`, `big room` and `hardstyle`
+    # carry nothing at all; `techstep`, `easycore` and `hospital records` carry
+    # real weight.
+    #
+    # Three kinds of tag are deliberately never used. Nationality -- american,
+    # british, swedish -- describes a passport, not a sound, and `american`
+    # alone spans 241 artists. `rock` and `electronic` span 340 and 275, which
+    # is most of the library and therefore no mood at all. And Last.fm's most
+    # popular tag for an artist is frequently their own name, so `green day`,
+    # `paramore` and `queen` are single-artist playlists waiting to happen.
     {
-        "name": "80s-synths",
+        "name": "drum-and-bass",
         "tags": [
-            "synthpop", "synth pop", "synth-pop", "new wave", "synthwave",
-            "new romantic", "electropop", "80s", "1980s",
+            "drum and bass", "dnb", "drum n bass", "drum'n'bass", "drum & bass",
+            "drum 'n' bass", "liquid funk", "neurofunk", "jungle", "techstep",
+            "darkstep", "drumstep", "breakbeat", "hospital records",
         ],
-        "years": [1975, 1992],
     },
     {
-        "name": "high-energy-rock",
+        "name": "bass",
+        "tags": ["dubstep", "brostep", "grime", "trip-hop", "big beat"],
+    },
+    {
+        "name": "dance",
         "tags": [
-            "hard rock", "punk rock", "pop punk", "punk", "alternative rock",
-            "rock", "garage rock", "skate punk",
+            "house", "electro house", "progressive house", "tech house", "trance",
+            "techno", "electro", "rave", "dance", "minimal",
+        ],
+    },
+    {
+        "name": "pop-punk",
+        "tags": [
+            "pop punk", "pop-punk", "punk rock", "punk", "skate punk", "emo",
+            "emocore", "easycore", "powerpop", "power pop", "post-grunge",
         ],
     },
     {
         "name": "screamo",
         "tags": [
-            "screamo", "post-hardcore", "metalcore", "emo", "hardcore",
-            "melodic hardcore", "emocore",
+            "screamo", "post-hardcore", "metalcore", "melodic metalcore",
+            "melodic hardcore", "hardcore", "trancecore", "deathcore",
         ],
     },
     {
-        "name": "drum-and-bass",
+        "name": "heavy-metal",
         "tags": [
-            "drum and bass", "drum n bass", "dnb", "liquid funk", "neurofunk",
-            "jungle", "breakbeat",
+            "heavy metal", "metal", "thrash metal", "thrash", "speed metal",
+            "power metal", "death metal", "progressive metal", "nwobhm",
+            "classic metal",
         ],
     },
     {
-        "name": "dance",
+        "name": "hair-metal",
         "tags": [
-            "electro house", "house", "big room", "electronic dance music",
-            "edm", "hardstyle", "trance", "dubstep", "electro",
+            "hair metal", "glam metal", "glam rock", "arena rock", "aor",
+            "rock and roll", "rock n roll",
+        ],
+        "years": [1975, 1994],
+    },
+    {
+        "name": "nu-metal",
+        "tags": [
+            "nu metal", "nu-metal", "alternative metal", "rapcore",
+            "industrial metal", "industrial rock", "industrial",
         ],
     },
     {
         "name": "classic-rock",
         "tags": [
-            "classic rock", "progressive rock", "psychedelic rock",
-            "blues rock", "70s", "60s",
+            "classic rock", "progressive rock", "psychedelic rock", "psychedelic",
+            "blues rock", "blues", "southern rock", "art rock", "space rock",
+            "british invasion", "folk rock", "70s", "60s",
         ],
     },
+    {
+        "name": "80s-synths",
+        # `80s` is included, which on its own would drag in Def Leppard and Bon
+        # Jovi -- they carry it as heavily as Eurythmics does. The exclusion is
+        # what separates them: the stadium rock is also tagged hard rock and
+        # hair metal, and the synth acts are not. Weighting cannot do this,
+        # because the tag it would weight is the one they share.
+        #
+        # Both spellings of synth pop are listed. Of Depeche Mode, Duran Duran,
+        # Eurythmics and Frankie Goes to Hollywood, three carry "synth pop" with
+        # a space and only one carries "synthpop" without.
+        "tags": [
+            "80s", "new wave", "synth pop", "synthpop", "synth-pop", "synthwave",
+            "electropop", "new romantic", "post-punk", "post-punk revival",
+        ],
+        "exclude": [
+            "hard rock", "hair metal", "glam metal", "glam rock", "heavy metal",
+            "metal", "arena rock", "aor", "nwobhm", "thrash metal",
+            "classic rock", "southern rock", "blues rock",
+        ],
+        "years": [1975, 1992],
+    },
+    {
+        "name": "indie",
+        "tags": ["indie", "indie rock", "indie pop", "britpop", "singer-songwriter"],
+    },
 )
 
 
@@ -1310,6 +1369,13 @@ def load_vibes(path):
             raise ValueError(f"{path}: {name!r} is not a usable vibe name (a-z, 0-9, -)")
         if not vibe.get("tags"):
             raise ValueError(f"{path}: vibe {name!r} lists no tags")
+        overlap = {str(tag).casefold() for tag in vibe["tags"]} & {
+            str(tag).casefold() for tag in vibe.get("exclude", [])
+        }
+        if overlap:
+            raise ValueError(
+                f"{path}: vibe {name!r} both selects on and excludes {sorted(overlap)}"
+            )
     return tuple(loaded)
 
 
@@ -1416,8 +1482,24 @@ def build_vibe_playlists(store, vibes, mirror_root, library_root, limit, now):
     for vibe in vibes:
         tags = [str(tag).strip().casefold() for tag in vibe["tags"]]
         placeholders = ",".join("?" * len(tags))
-        years = vibe.get("years")
         parameters = [*tags, vibe.get("min_score", VIBE_MIN_SCORE)]
+
+        # An exclusion drops the artist outright rather than docking their
+        # score. It is the only way to write "the eighties, but not the stadium
+        # rock": those artists carry `80s` as heavily as the synth acts do, so
+        # no amount of weighting separates them -- but they also carry `hard
+        # rock`, and the synth acts do not.
+        excluded = [str(tag).strip().casefold() for tag in vibe.get("exclude", [])]
+        exclude_clause = ""
+        if excluded:
+            exclude_clause = (
+                " AND NOT EXISTS (SELECT 1 FROM artist_tag x"
+                "                  WHERE x.norm_artist = a.norm_name"
+                f"                   AND x.tag IN ({','.join('?' * len(excluded))}))"
+            )
+            parameters += excluded
+
+        years = vibe.get("years")
         year_clause = ""
         if years:
             year_clause = (
@@ -1440,7 +1522,7 @@ def build_vibe_playlists(store, vibes, mirror_root, library_root, limit, now):
               JOIN lidarr_artist a ON a.id = t.artist_id
               JOIN vibe v ON v.norm_artist = a.norm_name
               LEFT JOIN lidarr_album al ON al.id = t.album_id
-             WHERE {PLAYABLE}{year_clause}
+             WHERE {PLAYABLE}{exclude_clause}{year_clause}
              ORDER BY ((t.id * {SHUFFLE_MULTIPLIER}) + ?) % {SHUFFLE_MODULUS}
              LIMIT ?
         """
@@ -1452,7 +1534,7 @@ def build_vibe_playlists(store, vibes, mirror_root, library_root, limit, now):
                 continue
             entries.append({**dict(row), "mirror": mirror})
 
-        write_playlist(directory / f"{vibe['name']}.m3u", entries)
+        write_playlist(directory / f"{vibe['name']}.m3u", entries, Path(mirror_root))
         total += len(entries)
         logger.info("playlist %-20s %4d tracks -- by tag", vibe["name"], len(entries))
 
@@ -1490,7 +1572,40 @@ def mirror_path_for(source, library_root, mirror_root):
     return (Path(mirror_root) / relative).with_suffix(MIRROR_SUFFIX)
 
 
-def write_playlist(path, entries):
+def set_ownership(path, uid, gid):
+    """Give a path an owner and group. Returns whether anything changed."""
+    try:
+        current = path.stat()
+        if (current.st_uid, current.st_gid) == (uid, gid):
+            return False
+        os.chown(path, uid, gid)
+    except OSError:
+        # Not permitted unless running as root, which is the case where the
+        # ownership is already whatever the caller runs as.
+        return False
+    return True
+
+
+def match_ownership(path, reference):
+    """Give a path the owner and group of the tree it is joining.
+
+    The image runs as root by default, so that a bind mount of any ownership
+    stays writable. The cost is that everything it writes comes out root-owned,
+    and a root-owned playlist inside a mirror owned by the apps account is
+    unreadable to the thing that serves it -- the group bit does not help when
+    the group is root.
+
+    Copying the mirror's own ownership avoids having to be told what it should
+    be, and is a no-op when the two already agree.
+    """
+    try:
+        wanted = reference.stat()
+    except OSError:
+        return False
+    return set_ownership(path, wanted.st_uid, wanted.st_gid)
+
+
+def write_playlist(path, entries, reference=None):
     """Write one extended M3U, atomically.
 
     Paths are relative to the playlist file, so the same playlist works from the
@@ -1502,7 +1617,11 @@ def write_playlist(path, entries):
         lines.append(f"#EXTINF:{seconds},{entry['artist']} - {entry['title']}")
         lines.append(os.path.relpath(entry["mirror"], path.parent))
 
+    fresh = not path.parent.exists()
     path.parent.mkdir(parents=True, exist_ok=True)
+    if fresh and reference is not None:
+        match_ownership(path.parent, reference)
+
     handle, temporary = tempfile.mkstemp(dir=path.parent, suffix=".m3u.part")
     os.close(handle)
     temporary = Path(temporary)
@@ -1513,6 +1632,10 @@ def write_playlist(path, entries):
         mode = temporary.stat().st_mode
         if not mode & GROUP_READ:
             temporary.chmod(mode | GROUP_READ)
+        # Before the rename, so the playlist is never briefly visible owned by
+        # the wrong account.
+        if reference is not None:
+            match_ownership(temporary, reference)
         os.replace(temporary, path)
     finally:
         temporary.unlink(missing_ok=True)
@@ -1543,7 +1666,7 @@ def build_playlists(store, mirror_root, library_root, limit, now):
                 missing += 1
                 continue
             entries.append({**dict(row), "mirror": mirror})
-        write_playlist(directory / f"{name}.m3u", entries)
+        write_playlist(directory / f"{name}.m3u", entries, Path(mirror_root))
         total += len(entries)
         logger.info("playlist %-20s %4d tracks -- %s", name, len(entries), description)
 

tests/test_music_curator.py

@@ -1,4 +1,5 @@
 import json
+import os
 import stat
 import urllib.error
 from pathlib import Path
@@ -1198,3 +1199,146 @@ def test_a_real_failure_is_not_recorded_so_the_next_pass_retries(tmp_path):
 
     # One artist failed hard and must still be pending.
     assert store.scalar("SELECT COUNT(*) FROM artist_tag_fetched") == 1
+
+
+# Tags that carry real weight in a real library, against tags that describe a
+# passport, span most of the collection, or are somebody's artist name.
+USELESS_TAGS = {
+    "american", "british", "australian", "canadian", "swedish", "dutch", "german",
+    "scottish", "english", "uk", "usa", "canada",
+    "rock", "electronic", "pop", "alternative", "metal ", "all", "heavy",
+    "female vocalists", "male vocalists", "female vocalist",
+    "my top songs", "cover", "covers", "not emo",
+    "green day", "paramore", "queen", "bon jovi", "shinedown", "aerosmith",
+    "journey", "fleetwood mac",
+}
+
+
+def test_no_mood_selects_on_a_useless_tag():
+    """Nationality is not a sound; `rock` and `electronic` span most of the
+    library; and Last.fm's top tag for an artist is often their own name."""
+    for vibe in music_curator.DEFAULT_VIBES:
+        overlap = {tag.casefold() for tag in vibe["tags"]} & USELESS_TAGS
+        assert not overlap, f"{vibe['name']} selects on {overlap}"
+
+
+def test_mood_names_are_unique():
+    names = [vibe["name"] for vibe in music_curator.DEFAULT_VIBES]
+    assert len(names) == len(set(names))
+
+
+ROCK_TAGS = {
+    "classic rock", "hard rock", "blues rock", "southern rock", "arena rock",
+    "glam rock", "hair metal", "glam metal", "heavy metal", "metal", "art rock",
+    "psychedelic rock", "progressive rock", "rock and roll", "rock n roll",
+}
+
+
+def test_a_decade_tag_in_a_non_rock_mood_must_exclude_the_rock():
+    """`80s` sits on Def Leppard and Bon Jovi as heavily as on Eurythmics, so a
+    mood that reaches for a decade without wanting rock has to say so. A mood
+    that does want it -- classic-rock reaching for 70s -- is exempt."""
+    for vibe in music_curator.DEFAULT_VIBES:
+        tags = {tag.casefold() for tag in vibe["tags"]}
+        decades = {"60s", "70s", "80s", "90s"} & tags
+        if not decades or tags & ROCK_TAGS:
+            continue
+        excluded = {tag.casefold() for tag in vibe.get("exclude", [])}
+        missing = {"hard rock", "hair metal"} - excluded
+        assert not missing, f"{vibe['name']} selects on {decades} without excluding {missing}"
+
+
+def test_the_eighties_mood_covers_the_canon():
+    """Depeche Mode, Duran Duran, Eurythmics and Frankie Goes to Hollywood --
+    checked against their live Last.fm tags. Three of the four carry "synth pop"
+    with a space; only one carries it without."""
+    synths = next(v for v in music_curator.DEFAULT_VIBES if v["name"] == "80s-synths")
+    tags = {t.casefold() for t in synths["tags"]}
+    for artist_tags in (
+        {"80s", "new wave", "pop", "female vocalists", "synth pop"},   # Eurythmics
+        {"80s", "new wave", "pop", "british", "dance"},                # Frankie
+        {"electronic", "synthpop", "new wave", "80s", "synth pop"},    # Depeche Mode
+        {"new wave", "80s", "pop", "synth pop", "rock"},               # Duran Duran
+    ):
+        assert tags & artist_tags, artist_tags
+    assert synths["years"] == [1975, 1992]
+
+
+def test_an_excluded_tag_drops_the_artist(tmp_path):
+    """Weighting cannot separate the eighties synth acts from the eighties
+    stadium rock, because the tag they would be weighted on is the one they
+    share."""
+    store, source, mirror = tagged_store(
+        tmp_path,
+        {
+            "Played Band": [{"name": "80s", "count": 100}, {"name": "hard rock", "count": 90}],
+            "Silent Band": [{"name": "80s", "count": 100}, {"name": "synth pop", "count": 90}],
+        },
+    )
+    vibes = [{"name": "eighties", "tags": ["80s", "synth pop"], "exclude": ["hard rock"]}]
+
+    music_curator.build_vibe_playlists(store, vibes, mirror, str(source), 100, NOW)
+
+    written = (mirror / "_playlists" / "eighties.m3u").read_text()
+    assert "Silent Band" in written
+    assert "Played Band" not in written
+
+
+def test_a_vibe_cannot_both_select_and_exclude_a_tag(tmp_path):
+    path = tmp_path / "vibes.json"
+    path.write_text(json.dumps([{"name": "x", "tags": ["80s"], "exclude": ["80s"]}]))
+
+    with pytest.raises(ValueError, match="selects on and excludes"):
+        music_curator.load_vibes(str(path))
+
+
+def test_matching_ownership_is_a_no_op_when_it_already_agrees(tmp_path):
+    target = tmp_path / "file"
+    target.write_text("x")
+
+    assert music_curator.match_ownership(target, tmp_path) is False
+
+
+def test_ownership_failure_is_tolerated(tmp_path):
+    """Not permitted unless running as root -- which is exactly the case where
+    the ownership is already whatever the caller runs as."""
+    target = tmp_path / "file"
+    target.write_text("x")
+
+    # uid 0 from a non-root test process: refused, and must not raise.
+    assert music_curator.set_ownership(target, 0, 0) is False
+
+
+def test_a_playlist_is_chowned_to_match_the_mirror(tmp_path, monkeypatch):
+    """The image runs as root, so its output is root-owned, and a root-owned
+    playlist in an apps-owned mirror is unreadable to whatever serves it."""
+    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
+    )
+    music_curator.match_library(store)
+
+    attempted = []
+    real_stat = music_curator.Path.stat
+
+    def pretend_mirror_is_owned_by_568(self, *args, **kwargs):
+        info = real_stat(self, *args, **kwargs)
+        if self == mirror:
+            return os.stat_result(
+                (info.st_mode, info.st_ino, info.st_dev, info.st_nlink, 568, 568,
+                 info.st_size, int(info.st_atime), int(info.st_mtime), int(info.st_ctime))
+            )
+        return info
+
+    monkeypatch.setattr(music_curator.Path, "stat", pretend_mirror_is_owned_by_568)
+    monkeypatch.setattr(
+        music_curator.os, "chown", lambda p, u, g: attempted.append((str(p), u, g))
+    )
+
+    music_curator.build_playlists(store, mirror, str(source), 100, NOW)
+
+    assert attempted, "no ownership was applied"
+    assert all(tuple(owner) == (568, 568) for _, *owner in attempted)
+    # The temporary file, before the rename, never the finished playlist.
+    assert all(path.endswith(".part") or path.endswith("_playlists") for path, *_ in attempted)