Real-world wishlist case the original c3b88e69 design missed: user with
26 missing tracks from 26 different albums. Each item used to promote
to its own album-bundle sub-batch (``min_tracks_per_album=1``), which
downloaded the ENTIRE album (5-42 files) to claim one track. Confirmed
in app.log:
- "Licensed To Ill" downloaded 3 times across cycles (3-4 files each)
- "The Understanding" 17 files for 1 wishlist track
- "Alright, Still" 42 files for 1 wishlist track
- ~85% wasted bandwidth, slskd hammered with 26 concurrent searches
PR 1 of a 4-PR fix series — see commit body footer for the other PRs.
Default ``min_tracks_per_album`` 1 → 2. Single-track wishlist items
fall to ``residual_tracks`` → classic per-track batch (already works,
already efficient). Album-bundle kept for the case it was designed
for: user has 2+ tracks missing from the same album.
Override via the new ``wishlist.album_bundle_min_tracks`` config key:
- 1 = previous behaviour (bundle every item)
- 2 = new default
- 3+ = stricter, for users who want bundle only on bigger gaps
Helper ``_resolve_album_bundle_threshold`` lives in
``core/wishlist/processing.py``. Defensive shape mirrors the existing
config-driven knobs (``get_poll_interval`` / ``get_transient_miss_threshold``):
non-numeric, non-positive, or config-manager-raise all fall back to
the safe default. Three test cases pin the fallback chain.
Both wishlist entry points wired through the same helper:
- ``process_wishlist_automatically`` (auto cycle, line 812)
- ``start_manual_wishlist_download_batch`` (manual run, line 539)
Tests:
- ``tests/wishlist/test_album_grouping.py`` — old ``test_default_threshold_promotes_solo_albums`` flipped to ``test_default_threshold_demotes_solo_albums`` with explanatory docstring naming the real-world cause. New ``test_default_threshold_promotes_multi_track_albums`` pins the 2+ promotion. New ``test_explicit_threshold_one_restores_solo_promotion`` pins that the kwarg still works for opt-back-in.
- ``tests/wishlist/test_processing.py`` — 3 new tests for ``_resolve_album_bundle_threshold``: default-when-config-missing, honors-config-override, falls-back-on-garbage.
- ``tests/wishlist/test_automation.py`` — ``test_wishlist_albums_cycle_splits_into_per_album_batches`` updated to use 2+ tracks per album (5 tracks across 2 albums instead of 3 across 2 with 1 solo). ``test_wishlist_albums_cycle_residual_for_orphan_tracks`` updated to include 2 tracks from Album One so it still promotes.
- ``tests/wishlist/test_manual_download.py`` — same shape update for the manual path test.
- ``tests/wishlist/test_album_grouping.py:test_multiple_albums_emit_separate_groups`` updated to reflect new default (alb1 with 2 tracks promotes, alb2 with 1 track goes residual).
- ``tests/wishlist/test_album_grouping.py:test_nested_track_data_payloads_normalized`` pinned with explicit ``min_tracks_per_album=1`` so the test stays focused on payload-shape parsing, not the threshold rule.
114 wishlist tests pass; 866 across wishlist + automation + downloads +
album_bundle + album_bundle_dispatch suites still green. Ruff clean.
Sibling PRs queued in TaskCreate:
- PR 2 — investigate post-process staging-match miss (the second-order
bug that causes the same album to redownload every cycle when the
staging step doesn't claim the requested track).
- PR 3 — fix sibling-completion gate that fires on first sibling
instead of last (log evidence: run a4945c88 finalized 1/26 batches).
- PR 4 — UI distinguish Queued from Analyzing for batches waiting
on the executor (23/26 batches sit at "Analyzing..." while really
queued at max_workers=3).
211 lines
7.4 KiB
Python
211 lines
7.4 KiB
Python
"""Wishlist album grouping for the per-album bundle dispatch.
|
|
|
|
When the auto-wishlist cycle is ``'albums'`` the user expects each
|
|
album with missing tracks to fire ONE album-bundle search instead
|
|
of one per-track search per missing track. Track lists in the
|
|
wishlist may span multiple albums in one cycle, so we group them
|
|
upfront + emit one sub-batch per album.
|
|
|
|
Pure function — no IO, no runtime-state dependency — so it can be
|
|
unit-tested without standing up the wishlist runner.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import json
|
|
from dataclasses import dataclass, field
|
|
from typing import Any, Dict, List, Optional
|
|
|
|
|
|
def _extract_track_data(track: Dict[str, Any]) -> Dict[str, Any]:
|
|
"""Mirror of ``classification._extract_track_data``: unwrap nested
|
|
Spotify payloads regardless of which key the wishlist row chose
|
|
to stash them under."""
|
|
for key in ("track_data", "spotify_data", "metadata", "track"):
|
|
data = track.get(key)
|
|
if isinstance(data, str):
|
|
try:
|
|
data = json.loads(data)
|
|
except Exception:
|
|
data = {}
|
|
if isinstance(data, dict) and data:
|
|
nested = (
|
|
data.get("track_data")
|
|
or data.get("spotify_data")
|
|
or data.get("metadata")
|
|
or data.get("track")
|
|
)
|
|
if isinstance(nested, str):
|
|
try:
|
|
nested = json.loads(nested)
|
|
except Exception:
|
|
nested = {}
|
|
if isinstance(nested, dict) and nested:
|
|
return nested
|
|
return data
|
|
return {}
|
|
|
|
|
|
def _album_key(spotify_data: Dict[str, Any]) -> Optional[str]:
|
|
"""Derive a stable grouping key from a track's Spotify metadata.
|
|
|
|
Prefers album id (canonical). Falls back to a name-normalized
|
|
key when the album row has no id (older wishlist rows can be
|
|
missing it). Returns ``None`` when no album information is
|
|
available at all — those tracks can't participate in an
|
|
album-bundle search and stay on the residual per-track flow.
|
|
"""
|
|
album = spotify_data.get('album') or {}
|
|
if not isinstance(album, dict):
|
|
return None
|
|
album_id = album.get('id')
|
|
if isinstance(album_id, str) and album_id.strip():
|
|
return album_id.strip()
|
|
name = album.get('name')
|
|
if isinstance(name, str) and name.strip():
|
|
return f"_name_{name.strip().lower()}"
|
|
return None
|
|
|
|
|
|
def _artist_name_from_track(spotify_data: Dict[str, Any], track: Dict[str, Any]) -> str:
|
|
"""Pick a primary artist name from the track's metadata.
|
|
|
|
Album-bundle search needs an artist string. Prefer the first
|
|
Spotify artist (most accurate), fall back to ``track_info['artist']``
|
|
or ``track['artist_name']`` from the wishlist row, then to empty
|
|
string (caller will skip the bundle).
|
|
"""
|
|
artists = spotify_data.get('artists') or []
|
|
if isinstance(artists, list) and artists:
|
|
first = artists[0]
|
|
if isinstance(first, dict):
|
|
name = first.get('name')
|
|
if isinstance(name, str) and name.strip():
|
|
return name.strip()
|
|
elif isinstance(first, str) and first.strip():
|
|
return first.strip()
|
|
for key in ('artist_name', 'artist'):
|
|
val = track.get(key)
|
|
if isinstance(val, str) and val.strip():
|
|
return val.strip()
|
|
return ''
|
|
|
|
|
|
@dataclass
|
|
class WishlistAlbumGroup:
|
|
"""One album's worth of wishlist tracks ready for a sub-batch."""
|
|
|
|
album_key: str
|
|
album_context: Dict[str, Any]
|
|
artist_context: Dict[str, Any]
|
|
tracks: List[Dict[str, Any]] = field(default_factory=list)
|
|
|
|
|
|
@dataclass
|
|
class WishlistGroupingResult:
|
|
"""Aggregated grouping output.
|
|
|
|
- ``album_groups``: one entry per resolvable album. Each carries
|
|
enough context to be submitted as an album-bundle batch.
|
|
- ``residual_tracks``: tracks that couldn't be grouped (no
|
|
album metadata + no artist). They fall through to the normal
|
|
per-track flow.
|
|
"""
|
|
|
|
album_groups: List[WishlistAlbumGroup] = field(default_factory=list)
|
|
residual_tracks: List[Dict[str, Any]] = field(default_factory=list)
|
|
|
|
|
|
def group_wishlist_tracks_by_album(
|
|
tracks: List[Dict[str, Any]],
|
|
*,
|
|
min_tracks_per_album: int = 2,
|
|
) -> WishlistGroupingResult:
|
|
"""Group wishlist tracks by their owning album.
|
|
|
|
``min_tracks_per_album`` controls the threshold for promoting an
|
|
album to its own sub-batch. Default ``2`` means an album needs at
|
|
least two missing tracks before the album-bundle search engages —
|
|
single-track items fall to ``residual_tracks`` and take the
|
|
classic per-track path. The 1-track case used to default to bundle
|
|
too, but real-world wishlists frequently look like "26 single
|
|
tracks from 26 different albums," and engaging bundle for each
|
|
one downloads ~85% of bandwidth as unwanted files, hammers slskd
|
|
with concurrent searches, and re-downloads the same album every
|
|
cycle when the staging-match step doesn't claim the requested
|
|
track. Bundle shines when several tracks from the same album are
|
|
missing — that's the case worth the bandwidth premium.
|
|
|
|
Override via the ``wishlist.album_bundle_min_tracks`` config key
|
|
or by passing ``min_tracks_per_album=N`` explicitly (kept for
|
|
tests + power users who want different behaviour).
|
|
"""
|
|
result = WishlistGroupingResult()
|
|
if not tracks:
|
|
return result
|
|
|
|
# First pass: bucket by album key.
|
|
buckets: Dict[str, WishlistAlbumGroup] = {}
|
|
unbucketable: List[Dict[str, Any]] = []
|
|
|
|
for track in tracks:
|
|
spotify_data = _extract_track_data(track)
|
|
key = _album_key(spotify_data)
|
|
if key is None:
|
|
unbucketable.append(track)
|
|
continue
|
|
|
|
artist_name = _artist_name_from_track(spotify_data, track)
|
|
if not artist_name:
|
|
unbucketable.append(track)
|
|
continue
|
|
|
|
album = spotify_data.get('album') or {}
|
|
if not isinstance(album, dict):
|
|
album = {}
|
|
album_name = album.get('name', '')
|
|
if not (isinstance(album_name, str) and album_name.strip()):
|
|
unbucketable.append(track)
|
|
continue
|
|
|
|
group = buckets.get(key)
|
|
if group is None:
|
|
album_context = {
|
|
'id': album.get('id') or key,
|
|
'name': album_name.strip(),
|
|
'release_date': album.get('release_date', ''),
|
|
'total_tracks': album.get('total_tracks', 0),
|
|
'album_type': album.get('album_type', 'album'),
|
|
'images': album.get('images', []),
|
|
'artists': album.get('artists', []),
|
|
}
|
|
artist_context = {
|
|
'id': 'wishlist',
|
|
'name': artist_name,
|
|
'genres': [],
|
|
}
|
|
group = WishlistAlbumGroup(
|
|
album_key=key,
|
|
album_context=album_context,
|
|
artist_context=artist_context,
|
|
)
|
|
buckets[key] = group
|
|
group.tracks.append(track)
|
|
|
|
# Second pass: promote groups meeting the threshold; demote
|
|
# smaller groups to residual.
|
|
for group in buckets.values():
|
|
if len(group.tracks) >= min_tracks_per_album:
|
|
result.album_groups.append(group)
|
|
else:
|
|
result.residual_tracks.extend(group.tracks)
|
|
|
|
result.residual_tracks.extend(unbucketable)
|
|
return result
|
|
|
|
|
|
__all__ = [
|
|
'group_wishlist_tracks_by_album',
|
|
'WishlistAlbumGroup',
|
|
'WishlistGroupingResult',
|
|
]
|