Dedupe individual tracks during import based on stable identifier#6723
Dedupe individual tracks during import based on stable identifier#6723tommyschnabel wants to merge 15 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6723 +/- ##
==========================================
+ Coverage 74.84% 74.87% +0.03%
==========================================
Files 163 163
Lines 20970 21070 +100
Branches 3300 3326 +26
==========================================
+ Hits 15694 15777 +83
- Misses 4521 4531 +10
- Partials 755 762 +7
🚀 New features to boost your workflow:
|
Move the import-time duplicate-track handling out of the `duplicates` plugin and into importer core, next to the existing duplicate machinery. Add the `import.duplicate_track_resolution` option (off by default). When enabled, album imports check each track against the library using `import.duplicate_keys.item` and resolve matches via the existing `import.duplicate_action`: - `skip` drops the already-imported tracks and imports the rest of the album (a fully-duplicate album is skipped); - `remove` removes the matching old library items; - `keep`/`merge` import everything unchanged; - `ask` prompts the session. The check runs before the autotag lookup so dropped tracks are excluded from the match. The `duplicates` plugin is left untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Match track duplicates against all library items (including album members, not just singletons), so re-importing catches tracks already present in an existing album. When per-track resolution prunes some tracks of an album, suppress the album-level duplicate check so the remaining new tracks are still imported instead of the whole (partial) album being skipped. Add a dedicated `duplicate_track_action` option (inheriting `duplicate_action` when unset) with a new `fold` action that adds the remaining new tracks to the existing album they belong to, completing a partially-imported album. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collapse the separate `fold` track duplicate action into `skip`: skipping per-track duplicates now folds the remaining new tracks into the existing album they belong to (falling back to a new album when the matches do not resolve to a single album). Removes the `fold` action, prompt option, and `FOLD` resolution. Show the per-track "Skipping duplicate track" message and the whole-album "Skipping album, all tracks are duplicates" message in the warning color. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6ece05a to
41c25aa
Compare
|
@semohr this is ready for review |
|
Hi @tommyschnabel, thanks for this work! I would suggest to rebase as there has been a considerable amount of changes and see how refactored duplicates handling by introducing |
…_actions # Conflicts: # beets/importer/session.py # beets/importer/stages.py # beets/importer/tasks.py # beets/test/helper.py # docs/changelog.rst
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@snejus got your changes integrated! Ready for a review 😄 |
…_actions # Conflicts: # docs/changelog.rst
|
@semohr ready for review again 😄 |
There was a problem hiding this comment.
Pull request overview
grug see PR add per-track duplicate handling during album import. goal is make repeat beet import more idempotent by matching tracks on stable key (like mb_trackid) and then skip/remove/keep/ask for only the dup tracks.
Changes:
- add
import.duplicate_track_resolution+import.duplicate_track_actionconfig, plus docs + changelog entry - add importer pipeline stage to detect/resolve per-track dupes before autotag lookup, with optional fold-into-existing-album behavior
- add tests for skip/remove/keep/ask flows and fold behavior
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_importer.py | add tests for per-track duplicate resolution behaviors |
| beets/ui/commands/import_/session.py | add interactive prompt for track-duplicate resolution in terminal session |
| beets/test/helper.py | extend ImportSessionFixture to simulate track-duplicate prompt answers |
| beets/importer/stages.py | implement per-track duplicate detection + resolution pipeline stage |
| beets/importer/tasks.py | factor duplicate-item deletion helper; support folding new tracks into existing album; remove track-dup items during manipulate stage |
| beets/importer/session.py | add ImportSession abstract hook for track-duplicate resolution + stage wiring |
| beets/config_default.yaml | add default config keys for new options |
| docs/reference/config.rst | document new config options and behavior notes |
| docs/changelog.rst | add changelog entry for new feature |
snejus
left a comment
There was a problem hiding this comment.
Well done! The first round of reviews :)
| duplicate_action: ask # whole-album duplicates | ||
| duplicate_track_action: skip # per-track duplicates: fold new tracks into the existing album | ||
| duplicate_keys: | ||
| item: mb_trackid # match on a stable id (recommended when autotagging) |
There was a problem hiding this comment.
Users will find it helpful to see the default values as well!
There was a problem hiding this comment.
Playing around with this more I think I'm going to add a note here that
duplicate_keys:
album: albumartist album
item: artist title
is also a really good config if some of the tracks you're importing are missing mb_trackid. I ran into this situation and had a bunch of duplicates, despite using this branch for imports because the tracks we're labeled correctly
|
|
||
| # Optionally drop or replace album tracks that already exist in | ||
| # the library before the autotag lookup runs. | ||
| stages += [stagefuncs.resolve_track_duplicates(self)] |
There was a problem hiding this comment.
How is this expected to interact with the default threaded: yes mode?
Looking at the pipeline ordering, I wonder whether this stage may check a second task before the first one reaches add(). For example, if two one-track albums with the same duplicate keys are imported in one invocation, could both pass this check in threaded mode while the second one would be skipped in sequential mode? I'm not sufficiently familiar with the pipeline internals to tell whether this is possible, so could you clarify what guarantees we have here?
Have you tested such scenario yourself?
There was a problem hiding this comment.
Yeah this is completely valid. I see two ways forward here:
- We make the concession that we're only checking against already-imported tracks, or
- We add an extra layer of key caching so that we can check against two of the same tracks being imported in the same session
2 adds more complexity, but I do like the completeness of it. Do you have any opinions on either of these? It does seem like 1. is a pretty narrow case, but it could be hit
There was a problem hiding this comment.
Actually, I went ahead and added 2. It's in an isolated commit so if you want me to remove it then that's no problem with me 😄
| task.duplicate_tracks_resolved = True | ||
| # Fold the remaining new tracks into the existing album, if the | ||
| # matched duplicates all belong to a single one. | ||
| album_ids = { |
There was a problem hiding this comment.
Could filtering out matches with album_id is None affect how the fold target is selected here?
For example, if one duplicate belongs to an album and another duplicate is a singleton, would album_ids contain only the first duplicate's album ID and cause the remaining tracks to be folded into that album? The documentation says that new tracks are imported as their own album when the matching tracks do not all belong to a single album, so I wonder whether singleton matches should prevent folding in this case. Is this scenario handled elsewhere?
I would like to see a test covering the following situation:
- Incoming album: tracks A, B, and C.
- Existing track A belongs to album X.
- Existing track B is a singleton.
- Both A and B match incoming tracks.
Would you mind adding this case and making the intended outcome for track C explicit? I think this would clarify whether it should be folded into album X or imported as its own album.
There was a problem hiding this comment.
Ah yeah this is a bug, I'll fix it so that A and B are skipped but C is imported
| def choose_item(self, task: SingletonImportTask) -> TrackMatch | Action: | ||
| raise NotImplementedError | ||
|
|
||
| def resolve_track_duplicates(self, task: ImportTask, duplicates) -> str: |
There was a problem hiding this comment.
Presumably this is going to be a list of Items?
| def resolve_track_duplicates(self, task: ImportTask, duplicates) -> str: | |
| def resolve_track_duplicates(self, task: ImportTask, duplicates: list[Item]) -> str: |
There was a problem hiding this comment.
Good call here, this is confusing. It's actually a dict[Item, list[Item]], a map of incoming item to any found matches. I'll add the type
- Return DuplicateAction from track duplicate resolution instead of single-letter strings; drop the fixture-only Resolution enum and drive tests via config. - Cast quiet/duplicate_verbose_prompt config reads with .get(bool) and add type annotations to resolve_track_duplicates. - Only fold remaining tracks into an existing album when every matched duplicate belongs to that album (a singleton match imports as a new album). - Add tests for the singleton-fold case, within-run sequential dedup, and the interactive ask prompt via the terminal session. - docs: use a yaml code-block and show default values. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…duplicates_import_actions
A per-track duplicate that matches an existing singleton is skipped individually but no longer prevents folding the remaining new tracks into the album that the matched album members belong to. New tracks are imported as their own album only when the matched members span multiple albums or no match belongs to an album. Makes the reviewer's A/B/C scenario explicit: track C is folded into the existing album X. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sent Matching per-track duplicates on mb_trackid skips files that lack the tag, letting them import as duplicates. Document that the default artist/title keys catch more when files are correctly named but not always tagged with MusicBrainz IDs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Under the default threaded import, the track-level duplicate check (resolve_track_duplicates) could run for a later task before an earlier task had been added to the library, so two albums sharing duplicate keys within a single run were not deduplicated. Record each imported track's duplicate key on the session as tasks pass through the (single-threaded) resolve_track_duplicates stage, and treat a track as a duplicate when its key was already claimed earlier in the run, even before it reaches the database. No lock is needed because only that one stage touches the cache. Add a deterministic test that drives the stage over two tasks with no add() in between and confirms the second is caught by the cache alone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@snejus thanks for the review! I think I've address all your comments! |
There was a problem hiding this comment.
I might be misunderstanding the intended workflow here. Even after reading the documentation, the PR description, and the code itself, I'm still not entirely sure what the intended use case is.
Couldn't duplicate resolution happen as a separate step in the pipeline after a match has been chosen? That would align better with my mental model, since we'd be working with the normalized metadata rather than the "raw" tags. Track and album level de-duplication should happen depending on the chosen candidate not the audios metadata imo.
I understand that the duplicate check is currently performed before autotagging, but I'm struggling to see what advantages that provides over doing it afterward.
Could we clearly define the intended behavior and use case at a high level. Having an abstract description of the workflow and its goals would make it much easier to reason about whether the current implementation is the right approach.
| task.skip | ||
| or not task.is_album | ||
| or not task.items | ||
| or not config["import"]["duplicate_track_resolution"].get(bool) |
There was a problem hiding this comment.
We should use session.config for consistency. This allows allows us to have multiple sessions with different configurations in the same runtime. Not too necessary for beets but for the beets as library usecase.
| if not task.items: | ||
| # Every track was a duplicate: skip the whole album. | ||
| log.info( | ||
| colorize( |
There was a problem hiding this comment.
Do not colorize log messages. If we need to show a warning just use log.warning.
| """Return library items matching `item` on all `keys`, excluding the | ||
| item itself (so re-imports do not match their own files). | ||
|
|
||
| Unlike :meth:`Item.duplicates_query`, this matches *every* library item, |
| """Decide what to do with album tracks already in the library.""" | ||
| log.warning("Some tracks are already in the library!") | ||
|
|
||
| if config["import"]["quiet"].get(bool): |
There was a problem hiding this comment.
Why not reuse the get_duplicate_action function? It already supports AnyModel and this function does not seem to do much more different to me.
| print(f" {item}") | ||
|
|
||
| ui.print_("New: " + summarize_items(list(duplicates), True)) | ||
| if config["import"]["duplicate_verbose_prompt"].get(bool): |
There was a problem hiding this comment.
Use _report_item_summary instead.
|
|
||
| # Optionally drop or replace album tracks that already exist in | ||
| # the library before the autotag lookup runs. | ||
| stages += [stagefuncs.resolve_track_duplicates(self)] |
There was a problem hiding this comment.
Why do we run this before autotagging? My assumption is that the metadata at this stage won't necessarily match the better-normalized metadata that ends up in the database after autotagging. Am I missing something?
For example:
# Original metadata (non-normalized)
artist: djFresH
# Metadata after autotagging and selecting a match
artist: dj-fresh
| super().__init__(toppath, paths, items) | ||
| # Existing library items to remove because individual tracks of this | ||
| # album duplicate them (see ``duplicate_track_resolution``). | ||
| self.duplicate_track_items_to_remove: list[library.Item] = [] |
There was a problem hiding this comment.
Im not too keen on adding these state variables here in task. Any other ways we could solve this?
|
Hey @semohr thanks for the review! I'm pretty new to this repo still, and so I'm really relying on your and @snejus's architectural suggestions. Making this a new step really makes sense, let me look into doing that. For reference, I had an earlier PR adding this check to the As a general point, it's rarely wanted behavior to import the same track multiple times, and obviously deduping is the way to stop this (my original use-case was re-importing a whole downloads dir multiple times before clearing it, and ending up with duplicates because of that). In my original implementation there was no concept of deduping albums; I was specifically targeting the track level only, and only looking at the album as a whole if it becomes empty due to skipping dedupes. So that brings me to the high level behavior of: I really do appreciate the reviews and the pointers on how you guys want this organized. Again, I'm very new to the repo and am happy to change whatever to help see this feature through. |
|
Alright that clears things up a bit for me.
What do you mean by incoming track? In general, I would avoid using I would approach track-level deduplication in the same way for consistency and unification.
I would fold this into a single decision point for the user (config-based behavior also possible). From the UI layer perspective, I would expect something like this: We haven't really but this in the docs yet but if you need a primer on the notations and different parts of the matching flows have a look here: #6683 |

Description
This PR adds the
duplicate_track_resolution(bool) andduplicate_track_action(skip,remove,keep/merge,ask) config options toimportto enable deduplication of individual tracks when importing.The recommended config for this is:
With the above config, any duplicate imports of tracks will be dropped, and if the whole album has been imported before, the album import itself will be dropped. If part of an album is deduped, the new tracks will be folded into the existing album. This config allows for idempotency on any repeat
beet importexecutions.Copied from the new docs, here's what each option does:
The
duplicate_keysconfig option is what's used to dedupe, so set it to something stable likemb_trackid.In the previous PR, @snejus suggested adding this as an addition to the album deduplication that already exists. This implementation is separate from the album dedupe logic because the use-cases seemed incompatible.
The album dedupe skips/merges/removes based on an album already being imported, but the use-case I wanted was a deeper track-based mechanism. Bolting this onto the album dedupe produced some really weird results, and I wasn't able to achieve parity with my last impl with the available config options. This is a better, more flexible, approach.
To Do
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)