Recommendation relevance, the rollback unit, and a version that names what shipped #131
@@ -108,7 +108,15 @@ func (h *handlers) handleRadio(w http.ResponseWriter, r *http.Request) {
|
||||
// Scoring weights come from the DB-backed tuning lab (#1250) —
|
||||
// read per request so an admin change takes effect live.
|
||||
weights := h.recSettings.Weights(recsettings.ScopeRadio)
|
||||
picks := recommendation.Shuffle(candidates, weights, time.Now().UTC(), h.rng, limit-1)
|
||||
// Diversity caps (#3882). Radio had none while every sibling surface did,
|
||||
// which is how a whole session could come back from one artist. Scaled to
|
||||
// the requested length so a 20-track radio and a 200-track one are capped
|
||||
// alike; Shuffle relaxes them rather than returning a short radio.
|
||||
//
|
||||
// limit-1 because the seed track occupies the first slot and is prepended
|
||||
// below — the caps govern the tracks that FOLLOW it.
|
||||
caps := recommendation.RadioDiversityCaps(limit - 1)
|
||||
picks := recommendation.Shuffle(candidates, weights, time.Now().UTC(), h.rng, limit-1, caps)
|
||||
|
||||
out := make([]TrackRef, 0, len(picks)+1)
|
||||
out = append(out, trackRefFrom(track, album.Title, artist.Name))
|
||||
|
||||
@@ -4,6 +4,8 @@ import (
|
||||
"sort"
|
||||
"time"
|
||||
|
||||
"github.com/jackc/pgx/v5/pgtype"
|
||||
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
|
||||
)
|
||||
|
||||
@@ -13,15 +15,72 @@ type Candidate struct {
|
||||
Inputs ScoringInputs
|
||||
}
|
||||
|
||||
// Shuffle scores each candidate, sorts descending by score, and returns
|
||||
// the top `limit` candidates. limit <= 0 returns nil; nil input returns
|
||||
// nil. Pure — no IO, no global state beyond the rng callback.
|
||||
// DiversityCaps bounds how much of one selection a single artist or album
|
||||
// may occupy. A zero value means "no cap", which is what Shuffle did
|
||||
// unconditionally before #3882.
|
||||
type DiversityCaps struct {
|
||||
MaxPerArtist int
|
||||
MaxPerAlbum int
|
||||
}
|
||||
|
||||
// radioCapArtistPer25 / radioCapAlbumPer25 hold the caps at the same
|
||||
// PROPORTION the system mixes already use — 3 artist / 2 album tracks in a
|
||||
// 25-track mix — so a 20-track radio and a 200-track one feel alike rather
|
||||
// than one of them being effectively uncapped.
|
||||
//
|
||||
// A fixed count cannot do that. Three-per-artist is a reasonable 12% of a
|
||||
// 25-track mix and an absurd 1.5% of a 200-track radio, where it would put
|
||||
// the selection permanently in the relaxation path below and quietly undo
|
||||
// the cap it was meant to enforce.
|
||||
const (
|
||||
radioCapArtistPer25 = 3
|
||||
radioCapAlbumPer25 = 2
|
||||
)
|
||||
|
||||
// RadioDiversityCaps scales the diversity caps to the requested radio
|
||||
// length. Floors of 2 and 1 keep a very short radio from being capped into
|
||||
// a single track per artist, which would be its own kind of wrong.
|
||||
func RadioDiversityCaps(limit int) DiversityCaps {
|
||||
artist := limit * radioCapArtistPer25 / 25
|
||||
if artist < 2 {
|
||||
artist = 2
|
||||
}
|
||||
album := limit * radioCapAlbumPer25 / 25
|
||||
if album < 1 {
|
||||
album = 1
|
||||
}
|
||||
return DiversityCaps{MaxPerArtist: artist, MaxPerAlbum: album}
|
||||
}
|
||||
|
||||
// Shuffle scores each candidate, sorts descending by score, and returns the
|
||||
// top `limit` candidates, preferring artist/album diversity. limit <= 0
|
||||
// returns nil; nil input returns nil. Pure — no IO, no global state beyond
|
||||
// the rng callback.
|
||||
//
|
||||
// THE COUNT IS NEVER REDUCED BY THE CAPS. Selection runs in two passes: the
|
||||
// first takes candidates that fit under the caps, and the second fills any
|
||||
// remaining slots from those the first pass skipped, still in score order.
|
||||
// So the result holds min(limit, len(candidates)) either way — the caps
|
||||
// change WHICH tracks are chosen, never HOW MANY.
|
||||
//
|
||||
// That two-pass shape is the whole design, and a hard cap would have been
|
||||
// the easy mistake. Radio asks for 50 tracks by default and 200 at most; a
|
||||
// pool concentrated on a few artists would return six tracks and call it a
|
||||
// radio. Rule 131's principle — degrade, never vanish — applies past the
|
||||
// system mixes it was written for.
|
||||
//
|
||||
// Before #3882 there was no cap here at all, while discover.go,
|
||||
// you_might_like.go and home.go all had one. That asymmetry is what let a
|
||||
// radio session come back entirely from a single artist: nothing between
|
||||
// the pool and the output bounded any artist's share, so a pool dominated
|
||||
// by one artist produced an output dominated by it too.
|
||||
func Shuffle(
|
||||
candidates []Candidate,
|
||||
weights ScoringWeights,
|
||||
now time.Time,
|
||||
rng func() float64,
|
||||
limit int,
|
||||
caps DiversityCaps,
|
||||
) []Candidate {
|
||||
if len(candidates) == 0 || limit <= 0 {
|
||||
return nil
|
||||
@@ -40,9 +99,36 @@ func Shuffle(
|
||||
if limit > len(scored) {
|
||||
limit = len(scored)
|
||||
}
|
||||
out := make([]Candidate, limit)
|
||||
for i := 0; i < limit; i++ {
|
||||
out[i] = scored[i].c
|
||||
|
||||
out := make([]Candidate, 0, limit)
|
||||
deferred := make([]Candidate, 0, len(scored)-limit)
|
||||
artistCount := map[pgtype.UUID]int{}
|
||||
albumCount := map[pgtype.UUID]int{}
|
||||
|
||||
for _, s := range scored {
|
||||
if len(out) == limit {
|
||||
break
|
||||
}
|
||||
overArtist := caps.MaxPerArtist > 0 && artistCount[s.c.Track.ArtistID] >= caps.MaxPerArtist
|
||||
overAlbum := caps.MaxPerAlbum > 0 && albumCount[s.c.Track.AlbumID] >= caps.MaxPerAlbum
|
||||
if overArtist || overAlbum {
|
||||
// Held back, not discarded — pass two may still need it.
|
||||
deferred = append(deferred, s.c)
|
||||
continue
|
||||
}
|
||||
artistCount[s.c.Track.ArtistID]++
|
||||
albumCount[s.c.Track.AlbumID]++
|
||||
out = append(out, s.c)
|
||||
}
|
||||
|
||||
// Pass two: the caps could not fill the request, so relax them rather
|
||||
// than hand back a short radio. Still score order, so the best of the
|
||||
// held-back candidates go first.
|
||||
for _, c := range deferred {
|
||||
if len(out) == limit {
|
||||
break
|
||||
}
|
||||
out = append(out, c)
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
@@ -0,0 +1,197 @@
|
||||
package recommendation
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/jackc/pgx/v5/pgtype"
|
||||
|
||||
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
|
||||
)
|
||||
|
||||
// candBy builds a candidate with a real artist and album identity, which
|
||||
// `cand` deliberately leaves zero.
|
||||
func candBy(t *testing.T, id, artist, album string, in ScoringInputs) Candidate {
|
||||
t.Helper()
|
||||
tr := dbq.Track{Title: id}
|
||||
if err := tr.ID.Scan("00000000-0000-0000-0000-" + id); err != nil {
|
||||
t.Fatalf("track id %q: %v", id, err)
|
||||
}
|
||||
if err := tr.ArtistID.Scan("00000000-0000-0000-0001-" + artist); err != nil {
|
||||
t.Fatalf("artist id %q: %v", artist, err)
|
||||
}
|
||||
if err := tr.AlbumID.Scan("00000000-0000-0000-0002-" + album); err != nil {
|
||||
t.Fatalf("album id %q: %v", album, err)
|
||||
}
|
||||
return Candidate{Track: tr, Inputs: in}
|
||||
}
|
||||
|
||||
// artistKey is the map key artistsOf produces for a given fixture artist,
|
||||
// derived the same way the fixture builds the UUID. Hand-writing the hex is
|
||||
// how the first version of this test went wrong: the artist id is not all
|
||||
// zeros — it carries 0001 in its fourth group — so the literal did not match
|
||||
// and the assertion measured nothing.
|
||||
func artistKey(t *testing.T, artist string) string {
|
||||
t.Helper()
|
||||
var id pgtype.UUID
|
||||
if err := id.Scan("00000000-0000-0000-0001-" + artist); err != nil {
|
||||
t.Fatalf("artist id %q: %v", artist, err)
|
||||
}
|
||||
return fmt.Sprintf("%x", id.Bytes)
|
||||
}
|
||||
|
||||
// artistsOf counts how many picks each artist contributed.
|
||||
func artistsOf(picks []Candidate) map[string]int {
|
||||
out := map[string]int{}
|
||||
for _, p := range picks {
|
||||
out[fmt.Sprintf("%x", p.Track.ArtistID.Bytes)]++
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// THE BUG. Operator, 2026-09-10: started radio from a song and "literally
|
||||
// all of the songs in the playlist after that were from a single artist".
|
||||
//
|
||||
// One artist's tracks all outscore everything else, and there are more of
|
||||
// them than the radio has room for. Without a cap the output is entirely
|
||||
// that artist — nothing between the pool and the result bounded its share.
|
||||
func TestShuffle_CapStopsOneArtistTakingTheWholeRadio(t *testing.T) {
|
||||
var cs []Candidate
|
||||
// 20 tracks by artist A, all liked so they sort to the top.
|
||||
for i := 0; i < 20; i++ {
|
||||
cs = append(cs, candBy(t, fmt.Sprintf("%012d", i), "00000000000a",
|
||||
fmt.Sprintf("%012d", i), ScoringInputs{IsGeneralLiked: true}))
|
||||
}
|
||||
// 10 tracks by 10 other artists, none liked, so they all rank below.
|
||||
for i := 0; i < 10; i++ {
|
||||
cs = append(cs, candBy(t, fmt.Sprintf("%012d", 100+i),
|
||||
fmt.Sprintf("%012d", 200+i), fmt.Sprintf("%012d", 100+i),
|
||||
ScoringInputs{IsGeneralLiked: false}))
|
||||
}
|
||||
|
||||
const limit = 10
|
||||
caps := DiversityCaps{MaxPerArtist: 3}
|
||||
picks := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), limit, caps)
|
||||
|
||||
if len(picks) != limit {
|
||||
t.Fatalf("len = %d, want %d", len(picks), limit)
|
||||
}
|
||||
byArtist := artistsOf(picks)
|
||||
artistA := artistKey(t, "00000000000a")
|
||||
if got := byArtist[artistA]; got != 3 {
|
||||
t.Errorf("artist A contributed %d of %d picks, cap is 3", got, limit)
|
||||
}
|
||||
if len(byArtist) < 8 {
|
||||
t.Errorf("only %d distinct artists in a %d-track radio; the cap is not "+
|
||||
"spreading the selection", len(byArtist), limit)
|
||||
}
|
||||
|
||||
// The same pool with NO cap is the regression this exists to catch. If
|
||||
// this stops holding, the test above is no longer proving anything.
|
||||
uncapped := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), limit, DiversityCaps{})
|
||||
if artistsOf(uncapped)[artistA] != limit {
|
||||
t.Errorf("uncapped selection was not single-artist, so this fixture no " +
|
||||
"longer reproduces the bug being fixed")
|
||||
}
|
||||
}
|
||||
|
||||
// A CAP IS A BOUND, NOT AN EXCLUSION. The operator asked for the opposite of
|
||||
// removal: "again it should be able to add songs from the same artist."
|
||||
func TestShuffle_CappedArtistIsStillRepresented(t *testing.T) {
|
||||
var cs []Candidate
|
||||
for i := 0; i < 20; i++ {
|
||||
cs = append(cs, candBy(t, fmt.Sprintf("%012d", i), "00000000000a",
|
||||
fmt.Sprintf("%012d", i), ScoringInputs{IsGeneralLiked: true}))
|
||||
}
|
||||
for i := 0; i < 10; i++ {
|
||||
cs = append(cs, candBy(t, fmt.Sprintf("%012d", 100+i),
|
||||
fmt.Sprintf("%012d", 200+i), fmt.Sprintf("%012d", 100+i),
|
||||
ScoringInputs{IsGeneralLiked: false}))
|
||||
}
|
||||
picks := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10,
|
||||
DiversityCaps{MaxPerArtist: 3})
|
||||
if artistsOf(picks)[artistKey(t, "00000000000a")] == 0 {
|
||||
t.Error("the dominant artist was excluded entirely; the cap should bound " +
|
||||
"its share, not remove it")
|
||||
}
|
||||
}
|
||||
|
||||
// RULE 131, applied past the system mixes it was written for: degrade, never
|
||||
// vanish. A hard cap over a pool with few artists would hand back a six-track
|
||||
// "radio" for a fifty-track request. The caps must change WHICH tracks are
|
||||
// picked, never HOW MANY.
|
||||
func TestShuffle_CapsNeverShortenTheResult(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
artists int
|
||||
perArt int
|
||||
limit int
|
||||
}{
|
||||
{"one artist owns the entire pool", 1, 30, 10},
|
||||
{"two artists, tight cap", 2, 15, 20},
|
||||
{"pool smaller than the request", 3, 2, 50},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
var cs []Candidate
|
||||
n := 0
|
||||
for a := 0; a < tc.artists; a++ {
|
||||
for k := 0; k < tc.perArt; k++ {
|
||||
cs = append(cs, candBy(t, fmt.Sprintf("%012d", n),
|
||||
fmt.Sprintf("%012d", 300+a), fmt.Sprintf("%012d", n),
|
||||
ScoringInputs{}))
|
||||
n++
|
||||
}
|
||||
}
|
||||
want := tc.limit
|
||||
if len(cs) < want {
|
||||
want = len(cs)
|
||||
}
|
||||
picks := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), tc.limit,
|
||||
DiversityCaps{MaxPerArtist: 3, MaxPerAlbum: 2})
|
||||
if len(picks) != want {
|
||||
t.Errorf("len = %d, want %d — the caps shortened the result instead "+
|
||||
"of relaxing to fill it", len(picks), want)
|
||||
}
|
||||
// No duplicates: a candidate deferred in pass one must not also be
|
||||
// taken in pass two.
|
||||
seen := map[[16]byte]bool{}
|
||||
for _, p := range picks {
|
||||
if seen[p.Track.ID.Bytes] {
|
||||
t.Fatalf("track %x appears twice; pass two re-added a pick", p.Track.ID.Bytes)
|
||||
}
|
||||
seen[p.Track.ID.Bytes] = true
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// A fixed count cannot serve both a 20-track radio and a 200-track one: three
|
||||
// per artist is 12% of the first and 1.5% of the second, which would leave the
|
||||
// long radio permanently in the relaxation path and effectively uncapped.
|
||||
func TestRadioDiversityCaps_ScaleWithTheRequestedLength(t *testing.T) {
|
||||
short := RadioDiversityCaps(25)
|
||||
long := RadioDiversityCaps(200)
|
||||
|
||||
if short.MaxPerArtist != 3 {
|
||||
t.Errorf("a 25-track radio caps artists at %d, want 3 — the same "+
|
||||
"proportion the system mixes use", short.MaxPerArtist)
|
||||
}
|
||||
if long.MaxPerArtist <= short.MaxPerArtist {
|
||||
t.Errorf("a 200-track radio caps artists at %d, no higher than a 25-track "+
|
||||
"one at %d; the cap is not scaling", long.MaxPerArtist, short.MaxPerArtist)
|
||||
}
|
||||
// Proportion held, not just "bigger".
|
||||
if long.MaxPerArtist != 24 {
|
||||
t.Errorf("200-track artist cap = %d, want 24 (3 per 25)", long.MaxPerArtist)
|
||||
}
|
||||
|
||||
// Floors: a tiny radio must not be capped down to one track per artist.
|
||||
tiny := RadioDiversityCaps(1)
|
||||
if tiny.MaxPerArtist < 2 {
|
||||
t.Errorf("tiny radio artist cap = %d, want at least 2", tiny.MaxPerArtist)
|
||||
}
|
||||
if tiny.MaxPerAlbum < 1 {
|
||||
t.Errorf("tiny radio album cap = %d, want at least 1", tiny.MaxPerAlbum)
|
||||
}
|
||||
}
|
||||
@@ -21,7 +21,7 @@ func TestShuffle_LikedRanksAboveUnliked(t *testing.T) {
|
||||
cand("000000000001", ScoringInputs{IsGeneralLiked: false}),
|
||||
cand("000000000002", ScoringInputs{IsGeneralLiked: true}),
|
||||
}
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10)
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{})
|
||||
if out[0].Track.Title != "000000000002" {
|
||||
t.Errorf("liked track did not rank first: %+v", out)
|
||||
}
|
||||
@@ -33,7 +33,7 @@ func TestShuffle_HighSkipRanksLast(t *testing.T) {
|
||||
cand("000000000002", ScoringInputs{PlayCount: 10, SkipCount: 0}), // ratio 0
|
||||
cand("000000000003", ScoringInputs{PlayCount: 10, SkipCount: 5}), // ratio 0.5
|
||||
}
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10)
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{})
|
||||
if out[0].Track.Title != "000000000002" || out[2].Track.Title != "000000000001" {
|
||||
t.Errorf("skip-ratio ordering broken: %v", titles(out))
|
||||
}
|
||||
@@ -44,7 +44,7 @@ func TestShuffle_LimitTruncates(t *testing.T) {
|
||||
for i := range cs {
|
||||
cs[i] = cand("00000000000"+string(rune('a'+i%26)), ScoringInputs{})
|
||||
}
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10)
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{})
|
||||
if len(out) != 10 {
|
||||
t.Errorf("len = %d, want 10", len(out))
|
||||
}
|
||||
@@ -58,7 +58,7 @@ func TestShuffle_JitterDoesNotFlipStructuralWinner(t *testing.T) {
|
||||
cand("000000000001", ScoringInputs{IsGeneralLiked: false}),
|
||||
cand("000000000002", ScoringInputs{IsGeneralLiked: true}),
|
||||
}
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), r.Float64, 10)
|
||||
out := Shuffle(cs, defaultWeights(), time.Now(), r.Float64, 10, DiversityCaps{})
|
||||
if out[0].Track.Title != "000000000002" {
|
||||
t.Fatalf("iter %d: liked did not rank first; out=%v", i, titles(out))
|
||||
}
|
||||
@@ -66,7 +66,7 @@ func TestShuffle_JitterDoesNotFlipStructuralWinner(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestShuffle_Empty_ReturnsEmpty(t *testing.T) {
|
||||
out := Shuffle(nil, defaultWeights(), time.Now(), fixedRNG(0.5), 10)
|
||||
out := Shuffle(nil, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{})
|
||||
if len(out) != 0 {
|
||||
t.Errorf("len = %d, want 0", len(out))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user