fix(recommendation): don't shrink a candidate arm ordered by unseeded random()
test-go / test (push) Successful in 1m10s
test-go / integration (push) Successful in 3m35s
release / Build signed APK (releases and dev) (push) Successful in 5m6s
release / Build + push container image (push) Successful in 16s
release / Verify release artifacts (tag releases only) (push) Skipped
test-go / test (push) Successful in 1m10s
test-go / integration (push) Successful in 3m35s
release / Build signed APK (releases and dev) (push) Successful in 5m6s
release / Build + push container image (push) Successful in 16s
release / Verify release artifacts (tag releases only) (push) Skipped
Fixes the integration failure from f367eeaa:
"same-day rebuild produced different track lists".
Cutting RandomFill 30→10 for Songs-like broke
TestBuildSystemPlaylists_DailyNonceDeterminism, and the reason is worth
stating because the number is not the bug.
`likes_overlap` and `random_fill` end in a bare `ORDER BY random()` with no
daily seed (recommendation.sql:118, :161). Such an arm returns a STABLE set
only while its LIMIT exceeds the rows eligible for it — then it returns all
of them, and the random order stops mattering because scoreAndSortCandidates
sorts by track id before drawing jitter. Below that threshold the arm
returns a random SUBSET, and two builds on the same day draw different ones.
So the test was green by accident. It seeds ~20 tracks against a default
RandomFill of 30; the limit exceeded the library, so the arm returned
everything. Determinism held for a reason unrelated to the code being right.
Which means it does NOT hold in production. Any real library is larger than
30, so same-day rebuilds have been drawing different mixes since that arm
was written — invisible, because a mix changing after a refresh looks like a
feature. Filed as #3889; the fix is a seeded ordering per (user, day), which
needs a .sql change and sqlc regeneration and so cannot land from here.
The correction: grow an arm freely, never shrink one whose ordering is
unseeded random. LikesOverlap and RandomFill go back to the defaults;
TasteOverlap stays halved because it sorts by `tpa.weight DESC, t.id` and is
genuinely deterministic. Guarded by a test that names the reasoning, so the
next person to trim these has to read why first — and it should be DELETED
once #3889 lands rather than worked around.
The cost is honest: the seed-independent share of the Songs-like pool falls
from 29% to 20% instead of the intended cut. That matters less than it
sounds. The pool only biases the draw; the songs_like WEIGHTS are what
actually demote sim_score-0 candidates, and they are untouched here — a
perfect match still scores 5.00 against an unrelated favourite's 2.00.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH
This commit is contained in:
@@ -139,22 +139,46 @@ func DefaultCandidateSourceLimits() CandidateSourceLimits {
|
|||||||
// would produce a short mix or none at all, and "no playlist" is a worse
|
// would produce a short mix or none at all, and "no playlist" is a worse
|
||||||
// answer than "a few tracks further from the seed than we would like".
|
// answer than "a few tracks further from the seed than we would like".
|
||||||
//
|
//
|
||||||
// likes_overlap is cut hardest of the tier-2 arms for a specific reason: its
|
// DO NOT SHRINK AN ARM ORDERED BY UNSEEDED random(). This is the constraint
|
||||||
// SQL assigns a FLAT 0.6 sim_score (recommendation.sql:108) rather than
|
// that shapes the numbers below, and it is not obvious from reading them.
|
||||||
// measuring anything. It is a collaborative signal wearing similarity's
|
//
|
||||||
// clothes, and raising SimilarityWeight amplifies it — if real ListenBrainz
|
// `likes_overlap` and `random_fill` both end in a bare `ORDER BY random()`
|
||||||
// scores commonly land below 0.6 it would outrank genuine matches. Halved
|
// (recommendation.sql:118, :161) with no daily seed. Such an arm returns a
|
||||||
// here pending the fill-rate measurement in #3879; the honest fix is to stop
|
// STABLE set only while its LIMIT exceeds the rows eligible for it — at that
|
||||||
// it claiming a similarity score it did not compute.
|
// point it returns all of them and the random order is irrelevant, because
|
||||||
|
// the caller sorts by id before scoring. Drop the limit below the eligible
|
||||||
|
// count and the arm starts returning a random SUBSET, which differs between
|
||||||
|
// two builds on the same day.
|
||||||
|
//
|
||||||
|
// That is a real defect (#3889) rather than a quirk of this function, and it
|
||||||
|
// bit here: cutting RandomFill to 10 broke
|
||||||
|
// TestBuildSystemPlaylists_DailyNonceDeterminism, whose library is smaller
|
||||||
|
// than the default limit and whose determinism was therefore accidental.
|
||||||
|
// Growing an arm is always safe; only shrinking one is.
|
||||||
|
//
|
||||||
|
// So the seed-independent arms are trimmed only where the ordering is
|
||||||
|
// deterministic: `taste_overlap` sorts by `tpa.weight DESC, t.id` and can be
|
||||||
|
// cut, `random_fill` cannot. The reduction is consequently modest — and it
|
||||||
|
// matters less than it looks, because the WEIGHTS are what demote sim_score-0
|
||||||
|
// candidates now. The pool change biases the draw; the songs_like profile is
|
||||||
|
// what actually keeps unrelated tracks out of the result.
|
||||||
|
//
|
||||||
|
// One arm is left alone that arguably should not be: `likes_overlap` assigns
|
||||||
|
// a FLAT 0.6 sim_score (recommendation.sql:108) rather than measuring
|
||||||
|
// anything — a collaborative signal wearing similarity's clothes, which a
|
||||||
|
// raised SimilarityWeight amplifies. If real ListenBrainz scores commonly
|
||||||
|
// land below 0.6 it will outrank genuine matches. It cannot be trimmed here
|
||||||
|
// without the determinism fix landing first; the honest repair is to stop it
|
||||||
|
// claiming a similarity score it never computed (#3879).
|
||||||
func SongsLikeCandidateSourceLimits() CandidateSourceLimits {
|
func SongsLikeCandidateSourceLimits() CandidateSourceLimits {
|
||||||
return CandidateSourceLimits{
|
return CandidateSourceLimits{
|
||||||
LBSimilar: 60, // tier 1 — doubled; the only arm that measures the seed
|
LBSimilar: 60, // tier 1 — doubled; the only arm that measures the seed
|
||||||
SimilarArtist: 40, // tier 2
|
SimilarArtist: 40, // tier 2 — raised; growing is always safe
|
||||||
TagOverlap: 20, // tier 2
|
TagOverlap: 20, // tier 2
|
||||||
UserCoplay: 20, // tier 2
|
UserCoplay: 20, // tier 2
|
||||||
LikesOverlap: 10, // tier 2, halved — flat 0.6, see above
|
LikesOverlap: 20, // tier 2 — NOT trimmed: unseeded random(), see above
|
||||||
TasteOverlap: 10, // tier 3 floor — halved, not removed
|
TasteOverlap: 10, // tier 3 floor — halved; deterministic ordering, safe
|
||||||
RandomFill: 10, // tier 3 floor — cut hard, never to zero
|
RandomFill: 30, // tier 3 floor — NOT trimmed: unseeded random(), see above
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -59,3 +59,41 @@ func TestSongsLikeLimits_KeepThePoolRoughlyTheSameSize(t *testing.T) {
|
|||||||
"this was meant to re-weight the pool, not starve it", s, d)
|
"this was meant to re-weight the pool, not starve it", s, d)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// The constraint that is invisible in the numbers, and that this file exists
|
||||||
|
// to keep visible.
|
||||||
|
//
|
||||||
|
// `likes_overlap` and `random_fill` end in a bare `ORDER BY random()` with no
|
||||||
|
// daily seed (recommendation.sql:118, :161). Such an arm returns a stable set
|
||||||
|
// only while its LIMIT exceeds the eligible rows; below that it returns a
|
||||||
|
// random SUBSET that differs between two builds on the same day, and the
|
||||||
|
// daily-determinism promise quietly stops holding.
|
||||||
|
//
|
||||||
|
// This is not hypothetical — it is how this change first failed CI. Cutting
|
||||||
|
// RandomFill to 10 broke TestBuildSystemPlaylists_DailyNonceDeterminism,
|
||||||
|
// whose library is smaller than the default limit and whose determinism was
|
||||||
|
// therefore an accident of the limit exceeding the library.
|
||||||
|
//
|
||||||
|
// Growing these arms is always safe. Only shrinking is, and the fix that
|
||||||
|
// would make shrinking safe is a seeded ordering (#3889), not a smaller
|
||||||
|
// number here.
|
||||||
|
func TestSongsLikeLimits_DoNotShrinkTheUnseededRandomArms(t *testing.T) {
|
||||||
|
d := DefaultCandidateSourceLimits()
|
||||||
|
s := SongsLikeCandidateSourceLimits()
|
||||||
|
|
||||||
|
for _, tc := range []struct {
|
||||||
|
arm string
|
||||||
|
songsLike, dflt int
|
||||||
|
}{
|
||||||
|
{"RandomFill", s.RandomFill, d.RandomFill},
|
||||||
|
{"LikesOverlap", s.LikesOverlap, d.LikesOverlap},
|
||||||
|
} {
|
||||||
|
if tc.songsLike < tc.dflt {
|
||||||
|
t.Errorf("%s cut from %d to %d. That arm is ordered by unseeded random(), "+
|
||||||
|
"so a smaller limit makes pool membership vary between same-day "+
|
||||||
|
"rebuilds — it breaks daily determinism rather than merely narrowing "+
|
||||||
|
"the mix. Fix the ordering (#3889) before trimming this.",
|
||||||
|
tc.arm, tc.dflt, tc.songsLike)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user