autotagging: introduce Source class for matching - #6681
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6681 +/- ##
==========================================
+ Coverage 75.61% 75.67% +0.05%
==========================================
Files 163 164 +1
Lines 21323 21342 +19
Branches 3363 3357 -6
==========================================
+ Hits 16124 16151 +27
+ Misses 4406 4399 -7
+ Partials 793 792 -1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
big change — grug nervous but also see good idea. PR introduce Source value object so importer, matcher, distance, and UI all share one current-metadata shape instead of each layer re-cooking common tags from raw items. also move AttrDict, Match/AlbumMatch/TrackMatch, and many exports around so beets.autotag becomes flatter public surface.
Changes:
- new
beets/autotag/source.pywithSource.from_items/Source.from_item;tag_albumandtag_itemnow takeSourceand return onlyProposal distance()now takeoriginal: AttrDict+unmatched_count(no longer recompute common tags);get_most_common_tags()returns onlylikeliesAttrDictmoved tobeets.util;Match/AlbumMatch/TrackMatchmoved tobeets/autotag/match.py;cur_artist/cur_albumremoved fromImportTask; many import paths flattened tobeets.autotag
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| beets/autotag/source.py | new Source NamedTuple, central current-metadata object |
| beets/autotag/match.py | tag_album/tag_item take Source; Match/AlbumMatch/TrackMatch moved here |
| beets/autotag/distance.py | distance() takes original AttrDict + unmatched_count |
| beets/autotag/hooks.py | AttrDict and Match classes removed; uses beets.util.AttrDict |
| beets/autotag/init.py | flatter re-exports incl. Source, Distance, Match, etc. |
| beets/util/init.py | AttrDict lives here; get_most_common_tags returns one dict |
| beets/importer/tasks.py | task.source cached_property; removes cur_artist/cur_album; new lookup_candidates signatures |
| beets/ui/commands/import_/session.py | choose_candidate/manual flows use task.source |
| beets/ui/commands/import_/display.py | show_change/show_item_change take artist/name strings directly |
| beetsplug/mbpseudo.py | use Source.from_items(...).data + unmatched_count for distance() |
| beetsplug/bench.py | only updates tag_album import (call site still passes items — broken) |
| many beetsplug/* + tests | mechanical import path updates to from beets.autotag import ... |
| .git-blame-ignore-revs | adds refactor commit hashes |
other notable findings: beetsplug/ihate.py still references removed task.cur_artist/cur_album; beetsplug/titlecase.py TYPE_CHECKING imports Info from beets.autotag where it is not exported; several docstrings (tag_album, get_most_common_tags, choose_candidate, Source.from_item, importer/stages.py) still describe pre-refactor return values or parameters.
Comments suppressed due to low confidence (1)
beets/ui/commands/import_/session.py:371
- docstring still describe old parameters
cur_artist,cur_album,itemcount,itemandsingleton. new signature only takecandidates,rec,source,choices. update docstring so future grug not chase ghost args.
def choose_candidate(candidates, rec, source: Source, choices=[]):
"""Given a sorted list of candidates, ask the user for a selection
of which candidate to use. Applies to both full albums and
singletons (tracks). Candidates are either AlbumMatch or TrackMatch
objects depending on `singleton`. for albums, `cur_artist`,
`cur_album`, and `itemcount` must be provided. For singletons,
`item` must be provided.
7322755 to
95e0a81
Compare
|
Am having a bit of a hard time understanding the changes here since you also moved functions around quite a bit. For reviewing purpose, would you mind splitting the refactor into another PR? |
Of course - I will split the first two commits into a separate PR. |
|
See #6687 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
test/ui/commands/test_import.py:94
- grug spy bug here.
show_change()signature now(source: Source, match: AlbumMatch)perbeets/ui/commands/import_/display.py, but this test still pass old three-arg shape(artist_str, album_str, match). Test will fail at call time. Need aSourcewhoseartist/namematch the expected "another artist with ..." / "another album" strings used in the assertions below, e.g. build one viaSource.from_itemsover items carrying those values, or constructSource(...)directly. grug skip fix because rightSourceshape is judgment call that affect expected snapshot text.
show_change(
f"another artist with {long_name}",
"another album",
AlbumMatch(change_dist, info, dict(item_info_pairs)),
)
|
|
||
| def distance( | ||
| items: Sequence[Item], | ||
| original: AttrDict[Any], |
There was a problem hiding this comment.
I would assume this should take a Source object now instead of an AttrDict[Any]?
The function arguments should only be these three in my opinion:
- Candidate (AlbumInfo)
- Source
- Assignment (item_info_pairs)
See here for more information and my understanding of it.
There was a problem hiding this comment.
See part of the docstring:
`original` is an AttrDict with the original album metadata.
It previously needed items in order to calculate likelies:
likelies, _ = get_most_common_tags(items)Now, this calculation is done by Source and the data is assigned to an attribute Source.data. It is then fed into distance, since it doesn't need any other data.
I made it an AttrDict so that keys can be accessed as attribute, to make these calls consistent:
dist.add_string("album", original.album, album_info.album)There was a problem hiding this comment.
How about we make Source.data a cached property and give Source as parameter here than. Seems like a slightly more clear abstraction for my mental model.
There was a problem hiding this comment.
Seems a bit weird that we need to precompute the get_most_common_tags in the from_item methods anyways.
There was a problem hiding this comment.
How about we make
Source.dataa cached property and giveSourceas parameter here than. Seems like a slightly more clear abstraction for my mental model.
Two issues with this:
datais different for eachSourcetype so we compute it either infrom_itemorfrom_itemsdatais needed in two places: for distance and inImportTask.chosen_infoso it does indeed need to be pre-computed
There was a problem hiding this comment.
Both of that shouldn't be an issue anymore once the ImportTask and SentienlImportTask combine. This is one of the main goals here, right?
There was a problem hiding this comment.
Indeed - but those will be combined in later PRs!
| ) | ||
|
|
||
| @classmethod | ||
| def from_items(cls, items: Sequence[Item]) -> Source: |
There was a problem hiding this comment.
Can we move the items creation into here? Maybe with a from_path alternate constructor?
I can see this being a bit difficulties with integrating this into current task factory but might simplify some things further and would align with my understanding of source.
There was a problem hiding this comment.
I think I kind of see the direction here. Are you saying we could get Source within ImportTaskFactory.singleton and ImportTaskFactory.album? And then replace ImportTask.items with ImportTask.source in the construction?
There was a problem hiding this comment.
Yes that is roughtly my idea. If we dont work with items but work with the source it should allow us to abstract most operations regardless of if we look at an album or singleton.
There was a problem hiding this comment.
Yup, this will get done but in later PRs.
c99d420 to
de7492d
Compare
88878c0 to
4a556fc
Compare
b4f70ec to
25e3006
Compare
| - The most common value for each field. | ||
| - Whether each field's value was unanimous (values are booleans). | ||
| """ | ||
| def get_most_common_tags(items: Sequence[Item]) -> dict[str, Any]: |
There was a problem hiding this comment.
I would maybe do something like this to get typing here:
class Likelies(TypedDict, total=False):
"""Most likely values for a list of items"""
artist: str
album: str
albumartist: str
year: int
disctotal: int
mb_albumid: str
label: str
barcode: str
catalognum: str
country: str
media: str
albumdisambig: str
data_source: str
def get_most_common_tags(items: Sequence[Item]) -> Likelies:
"""Extract the most common value for each field given a list of items."""
assert items
likelies: Likelies = {}
for field in tuple(Likelies.__annotations__):
values = [item.get(field) for item in items if item]
value, _ = plurality(values)
if value is not None:
likelies[field] = value
albumartist = likelies.get("albumartist")
if len({i.albumartist for i in items}) == 1 and albumartist:
likelies["artist"] = albumartist
return likeliesI know we are currently not using it but it would be nice to have the counts in there and if it is a consensus value. I see this as an object we are going to pass to the metadata_plugins at some point.
class MetadataPlugin:
def candidates(items, likelies, search_query): ...
# or
def candidates(source, search_query):
likelies = source.likeliesThere was a problem hiding this comment.
I considered this but the biggest issue is that I wanted to feed original as AttrDict into distance to normalize attribute access within that function:
diff --git a/beets/autotag/distance.py b/beets/autotag/distance.py
index d5821f473..05647da21 100644
--- a/beets/autotag/distance.py
+++ b/beets/autotag/distance.py
@@ -9,13 +9,14 @@
from unidecode import unidecode
from beets import config, metadata_plugins
-from beets.util import as_string, cached_classproperty, get_most_common_tags
+from beets.util import as_string, cached_classproperty
from beets.util.color import colorize
if TYPE_CHECKING:
from collections.abc import Iterator, KeysView, Sequence
from beets.library import Item
+ from beets.util import AttrDict
from beets.util.color import ColorName
from .hooks import AlbumInfo, TrackInfo
@@ -427,28 +428,27 @@ def track_distance
def distance(
- items: Sequence[Item],
+ original: AttrDict[Any],
album_info: AlbumInfo,
item_info_pairs: list[tuple[Item, TrackInfo]],
+ unmatched_count: int,
) -> Distance:
- """Determines how "significant" an album metadata change would be.
- Returns a Distance object. `album_info` is an AlbumInfo object
- reflecting the album to be compared. `items` is a sequence of all
- Item objects that will be matched (order is not important).
- `mapping` is a dictionary mapping Items to TrackInfo objects; the
- keys are a subset of `items` and the values are a subset of
- `album_info.tracks`.
- """
- likelies, _ = get_most_common_tags(items)
+ """Determine how "significant" an album metadata change would be.
+ Returns a Distance object.
+ `original` is an AttrDict with the original album metadata.
+ `album_info` is an AlbumInfo object reflecting the album to be compared.
+ `item_info_pairs` is a list with matched (Item, TrackInfo) pairs.
+ `unmatched_count` is the number of unmatched tracks on the release.
+ """
dist = Distance()
# Artist, if not various.
if not album_info.va:
- dist.add_string("artist", likelies["artist"], album_info.artist)
+ dist.add_string("artist", original.artist, album_info.artist)
# Album.
- dist.add_string("album", likelies["album"], album_info.album)
+ dist.add_string("album", original.album, album_info.album)
preferred_config = config["match"]["preferred"]
# Current or preferred media.
@@ -461,29 +461,29 @@ def distance
if options:
dist.add_priority("media", album_info.media, options)
# Current media.
- elif likelies["media"]:
- dist.add_equality("media", album_info.media, likelies["media"])
+ elif original.media:
+ dist.add_equality("media", album_info.media, original.media)
# Mediums.
- if likelies["disctotal"] and album_info.mediums:
- dist.add_number("mediums", likelies["disctotal"], album_info.mediums)
+ if original.disctotal and album_info.mediums:
+ dist.add_number("mediums", original.disctotal, album_info.mediums)
# Prefer earliest release.
if album_info.year and preferred_config["original_year"]:
# Assume 1889 (earliest first gramophone discs) if we don't know the
# original year.
- original = album_info.original_year or 1889
- diff = abs(album_info.year - original)
- diff_max = abs(datetime.date.today().year - original)
+ original_year = album_info.original_year or 1889
+ diff = abs(album_info.year - original_year)
+ diff_max = abs(datetime.date.today().year - original_year)
dist.add_ratio("year", diff, diff_max)
# Year.
- elif likelies["year"] and album_info.year:
- if likelies["year"] in (album_info.year, album_info.original_year):
+ elif original.year and album_info.year:
+ if original.year in (album_info.year, album_info.original_year):
# No penalty for matching release or original year.
dist.add("year", 0.0)
elif album_info.original_year:
# Prefer matchest closest to the release year.
- diff = abs(likelies["year"] - album_info.year)
+ diff = abs(original.year - album_info.year)
diff_max = abs(
datetime.date.today().year - album_info.original_year
)
@@ -498,30 +498,28 @@ def distance
if album_info.country and options:
dist.add_priority("country", album_info.country, options)
# Country.
- elif likelies["country"] and album_info.country:
- dist.add_string("country", likelies["country"], album_info.country)
+ elif original.country and album_info.country:
+ dist.add_string("country", original.country, album_info.country)
# Label.
- if likelies["label"] and album_info.label:
- dist.add_string("label", likelies["label"], album_info.label)
+ if original.label and album_info.label:
+ dist.add_string("label", original.label, album_info.label)
# Catalog number.
- if likelies["catalognum"] and album_info.catalognum:
+ if original.catalognum and album_info.catalognum:
dist.add_string(
- "catalognum", likelies["catalognum"], album_info.catalognum
+ "catalognum", original.catalognum, album_info.catalognum
)
# Disambiguation.
- if likelies["albumdisambig"] and album_info.albumdisambig:
+ if original.albumdisambig and album_info.albumdisambig:
dist.add_string(
- "albumdisambig", likelies["albumdisambig"], album_info.albumdisambig
+ "albumdisambig", original.albumdisambig, album_info.albumdisambig
)
# Album ID.
- if likelies["mb_albumid"]:
- dist.add_equality(
- "album_id", likelies["mb_albumid"], album_info.album_id
- )
+ if original.mb_albumid:
+ dist.add_equality("album_id", original.mb_albumid, album_info.album_id)
# Tracks.
dist.tracks = {}
@@ -534,9 +532,9 @@ def distance
dist.add("missing_tracks", 1.0)
# Unmatched tracks.
- for _ in range(len(items) - len(item_info_pairs)):
+ for _ in range(unmatched_count):
dist.add("unmatched_tracks", 1.0)
- dist.add_data_source(likelies["data_source"], album_info.data_source)
+ dist.add_data_source(original.data_source, album_info.data_source)
return distThere was a problem hiding this comment.
But it seems like we can do this?
diff --git a/beets/autotag/source.py b/beets/autotag/source.py
index 4f1b222e2..06e89b4ee 100644
--- a/beets/autotag/source.py
+++ b/beets/autotag/source.py
@@ -1,8 +1,8 @@
from __future__ import annotations
-from typing import TYPE_CHECKING, Any, Literal, NamedTuple
+from typing import TYPE_CHECKING, Literal, NamedTuple
-from beets.util import AttrDict, get_most_common_tags
+from beets.util import Likelies, get_most_common_tags
if TYPE_CHECKING:
from collections.abc import Sequence
@@ -14,7 +14,7 @@ class Source
type: Literal["album", "track"]
artist: str
name: str
- data: AttrDict[Any]
+ data: Likelies
items: Sequence[Item]
id: str
id_consensus: bool
@@ -35,11 +35,11 @@ def from_items
likelies = get_most_common_tags(items)
return cls(
type="album",
- artist=likelies["artist"],
- name=likelies["album"],
- data=AttrDict(likelies),
+ artist=likelies.artist,
+ name=likelies.album,
+ data=likelies,
items=items,
- id=likelies["mb_albumid"],
+ id=likelies.mb_albumid,
id_consensus=len({i.mb_albumid for i in items}) == 1,
)
@@ -50,7 +50,7 @@ def from_item
type="track",
artist=item.artist,
name=item.title,
- data=AttrDict(item),
+ data=Likelies(item), # type: ignore[arg-type]
items=[item],
id=item.mb_trackid,
id_consensus=True,
diff --git a/beets/util/__init__.py b/beets/util/__init__.py
index 9af5138ea..9bd8127fb 100644
--- a/beets/util/__init__.py
+++ b/beets/util/__init__.py
@@ -809,7 +809,7 @@ def plurality
return c.most_common(1)[0]
-def get_most_common_tags(items: Sequence[Item]) -> dict[str, Any]:
+def get_most_common_tags(items: Sequence[Item]) -> Likelies:
"""Extract the most common value for each field given a list of items."""
assert items # Must be nonempty.
@@ -837,7 +837,7 @@ def get_most_common_tags
if len({i.albumartist for i in items}) == 1 and likelies["albumartist"]:
likelies["artist"] = likelies["albumartist"]
- return likelies
+ return Likelies(likelies)
# stdout and stderr as bytes
@@ -1238,3 +1238,21 @@ def copy
def __hash__(self) -> int: # type: ignore[override]
return id(self)
+
+
+class Likelies(AttrDict[str | None]):
+ """A dictionary of the most common tags in a list of items."""
+
+ artist: str
+ album: str
+ albumartist: str
+ year: str
+ disctotal: str
+ mb_albumid: str
+ label: str
+ barcode: str
+ catalognum: str
+ country: str
+ media: str
+ albumdisambig: str
+ data_source: str | NoneThere was a problem hiding this comment.
Can we make the Likelies a frozen dataclass or is there a specific reason it needs to be a mutable AttrDict?
There was a problem hiding this comment.
A frozen dataclass does sound like a good fit for values that are calculated once and only read afterwards. I'm just a little hesitant to introduce another metadata container alongside Item, AttrDict, Source, AlbumInfo, and TrackInfo, particularly if we then need conversions between them.
On the other hand, I am more than happy to add Likelies as suggested above. That would give us typed attribute access while retaining the existing mapping behavior and avoiding a separate representation. It would also let us pass the same typed mapping directly into beets.autotag.distance.distance. If we later pass this data to metadata plugins, that concrete API might give us a better basis for deciding whether a different container would be worthwhile. What do you think?
There was a problem hiding this comment.
I was mostly asking because I'm not a big fan of AttrDict in general. Since it's backed by a dict, it still allows arbitrary attributes to be added dynamically, which isn't really what we want here.
That said, I'm happy with introducing Likelies.
To me, it also makes sense to have different containers for different concepts. I see the hierarchy roughly like this:
Source: wraps the raw files on disk.Item: the canonical abstraction for track metadata (whether it comes from the filesystem or the library database).AlbumInfo/TrackInfo: metadata returned from remote providers.
Likelies feels like a different kind of object. It doesn't represent metadata itself, but the aggregated "likely" values derived from a group of Items (e.g., the majority value for album artist, year, etc.). Having a dedicated type for that seems reasonable to me.
#6843) ## Bug `SD_PATTERNS` in `beets/autotag/distance.py` has this entry: ```python (r"[\[\(]?(featuring|feat|ft)[\. :].+", 0.1), ``` The `ft` alternative has no word boundary before it, so it matches `ft` embedded inside any ordinary word followed by a space/period/colon — `draft`, `left`, `gift`, `craft`, `soft`, `swift`, etc. — and treats everything after the match as a low-weight "featuring artist" suffix (weight `0.1`), effectively deleting it from the comparison. ```python from beets.autotag.distance import string_dist string_dist("Draft Beer", "Draft Whiskey") # 0.05 (near-identical!) string_dist("Left Field", "Left Symphony") # 0.067 string_dist("Gift Ideas", "Gift Cards") # 0.044 ``` Confirmed through the actual matching pipeline (`track_distance`) too: "Draft Beer" vs. "Draft Whiskey" scores almost as close as an exact match, while a genuinely unrelated title correctly scores much higher. `string_dist`/`add_string` backs nearly every fuzzy field comparison used during `beet import` (title, artist, album, label, catalognum, ...), so this silently degrades match quality for any title/artist containing one of these words. ## Fix Add a word boundary before the alternation: ```python (r"[\[\(]?\b(featuring|feat|ft)[\. :].+", 0.1), ``` Verified this doesn't regress the intended "real" cases (`"Song feat. Artist"`, `"[feat: Artist]"`, `"(ft. Artist)"` — all still match, since they're preceded by whitespace/bracket/string-start, which are real boundaries) while excluding all the false-positive mid-word cases above. ## Testing Added `test_featuring_pattern_does_not_match_mid_word` (parametrized over draft/left/gift/craft) to `test/autotag/test_distance.py`, asserting `string_dist(...) > 0.3` for these pairs (previously ~0.04-0.07). Verified red on `master` (`git stash` the source fix — all 4 cases fail with the near-zero distances shown above), green after. Full `test/autotag/test_distance.py` (58 passed) and the broader `test/autotag/` + `test/util/` + `test/autotag/test_match.py` + `test/test_importer.py` suites (352 + 142 passed, no regressions) all pass. `ruff check`/`ruff format --check`/`mypy` clean. Added a changelog entry per `CONTRIBUTING.rst`'s reminder. ## Duplicate-work check Searched open PRs for `distance.py`/"featuring" — found #6681 (Source class refactor) also touches `distance.py`, but its diff doesn't touch `SD_PATTERNS` or this area at all. No overlap. --- This PR was drafted with AI assistance (Claude); I independently reproduced the bug against the real, unmodified module (including through the full `track_distance` matching pipeline) and verified the fix. I reviewed the change and take responsibility for it.
b75b841 to
5754351
Compare
Introduce Source class to encapsulate metadata extraction logic, replacing scattered `get_most_common_tags` calls throughout codebase. - Add Source class in importer.tasks with artist, name, and metadata - Simplify `distance()` to accept Source.data instead of items list - Update display functions to use Source objects - Cache Source creation with @cached_property on ImportTask This centralizes metadata handling and reduces coupling between autotag and library modules.
beets.autotagSource-Centric RefactorFixes: #6684
Core change
This PR restructures autotagging around a new
beets.autotag.source.Sourceobject.Before this change, album and singleton matching pulled "current metadata" from several places:
After this change, those flows all receive the same input shape: a single
Sourcevalue that carries:itemsartist,name)dataid,id_consensus)That makes
Sourcethe new "current thing being matched" abstraction for both albums and tracks.Architecture
1. New single input object:
SourceSourcecentralizes metadata extraction that used to be scattered across the autotag pipeline.This is the biggest architectural shift in the PR.
2.
tag_album()andtag_item()now operate onSourceThe matching entry points now take a
Sourceinstead of a loose mix of:itemsitemThat gives album and track matching the same calling convention and reduces special-case plumbing.
3.
distance()is now narrower and more explicitdistance()no longer derives "current metadata" internally from a list of items.Instead, it receives:
original: the already-extracted metadata containerunmatched_count: the number of unmatched local tracksThis is a good separation of responsibilities:
Sourceprepares current metadatadistance()only compares metadata4. Importer tasks now cache the current source
ImportTaskandSingletonImportTasknow expose a cachedsourceproperty:Source.from_items(...)Source.from_item(...)That means importer state no longer needs separate
cur_artistandcur_albumfields, and downstream code can read fromtask.sourceinstead.5. UI/display code now depends on the same source model
Import session and display functions now use
task.source.artistandtask.source.namewhen showing candidate changes or prompting for manual search/ID entry.This removes another copy of "current metadata" logic from the UI layer.
6.
AttrDictmoves tobeets.utilAttrDictwas previously defined insidebeets.autotag.hooks. It now lives inbeets.util.High-level effect:
7.
beets.autotagpublic API is flatterRelated cleanup continues by exporting more autotag types directly from
beets.autotag, includingSource.This reduces import-path churn for callers and makes the package boundary easier to understand.
High-level impact
What this improves
1. One place defines "the current metadata we are matching from"
The main benefit is conceptual clarity.
Reviewers can now think about autotagging as:
instead of following several separate paths that each rediscover the same metadata.
2. Album and singleton flows now look alike
Albums and single tracks now use the same structure and similar call patterns.
That should make future autotag changes easier because new behavior can usually be added once at the
Sourceboundary rather than twice in parallel code paths.3. Less coupling between importer, matcher, distance logic, and UI
Previously, each layer needed to know how to reconstruct "current artist/album/common tags".
Now:
SourceSourceSourcedistance()receives prepared metadataEach layer does less guessing about the others.
4. Simpler data flow for debugging and future refactors
When a match looks wrong, there is now a clearer place to inspect the original input:
Source.That should help with debugging, logging, and future changes to metadata extraction rules.
Data flow at a glance
Notable follow-on cleanup included here
get_most_common_tags()now returns only the extracted metadata dictionary, not a(likelies, consensus)pairid_consensusorva_likelySource-based flow.git-blame-ignore-revswas updated for mechanical refactor commitsReviewer guide
If reviewing this PR, the easiest way to read it is:
beets/autotag/source.pytag_album()andtag_item()inbeets/autotag/match.pydistance()signature changetask.sourceThat path shows the architectural intent without getting lost in the mechanical updates.