Add upgrade option for duplicates

[Serene-Arc]

Description

This adds another option for the duplicate menu, as well as the option for duplicates in the configuration. This is for the (admittedly specific) use case where there exists a file in the library and a duplicate with a higher bitrate is being added. The bitrate of the two files/albums is compared. If the existing one is higher, then the duplicate is skipped. If the new file/album is a higher bitrate, the version in the library will be replaced like in the 'remove old' option.

The main intent of this is to be used as a configuration option. Obviously when the duplicate action is 'ask', the user can see for themselves which version is better, but this would allow the 'upgrade' action to be set in the configuration for more hands-free and automatic applications.

To Do

• Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
• Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
• Tests. (Encouraged but not strictly required.)

[arsaboo]

Thanks for the contribution. One of the problems with the current Remove Old option is it removes all the old items even if one new item is being added, i.e., all the items in the album are considered part of the old, and they are removed even if only one new item is being added.

Wondering if you had a chance to test that out. Specifically, let's say you have an existing album with 4 items and you add an upgraded duplicate item. It should only upgrade one item and the other ones should be left untouched.

[Serene-Arc]

I did not test that. I tested with singletons and albums, but I'll look into fixing this. Thanks for letting me know a case! I tried to hook into the old process as much as possible but I might have to get more complex here.

[arsaboo]

I did not test that. I tested with singletons and albums, but I'll look into fixing this. Thanks for letting me know a case! I tried to hook into the old process as much as possible but I might have to get more complex here.

I did not mean to add a pre-existing bug to your plate.

The upgrade option is already a welcome addition and should be added. It is fine to have a bug fix as a separate PR if that makes it easier.

[Serene-Arc]

@arsaboo then I'll fix that bug while I'm at it! I have a solution in mind, I just have to test it and make sure all of the code works. The import process is pretty complex but I'm slowly mapping it out in my head.

So is the desired behaviour for all duplicates that only things with a replacement incoming should be replaced?

[arsaboo]

Indeed...deleting/upgrading should happen only at the item level, i.e., only the duplicate items should be deleted and not the entire album.

Right now, when we use album import, even if there is one duplicate item, the entire album is marked for deletion. See below:

beets/beets/ui/commands.py

Lines 1308 to 1311 in 50bd693

	def remove_items(lib, query, album, delete, force):
	"""Remove items matching query from lib. If album, then match and
	remove whole albums. If delete, also remove files from disk.
	"""

Hope that clarifies. You can test things out easily. Just try to re-add one of the items and use Remove old.

[sampsyo]

Hi there—super interesting feature; thanks for getting it started! This seems like a version of a very old feature idea, #116.

I have a couple of high-level thoughts:

• What do you think about making this "config only" for now, rather than also an interactive option? As you say, this seems most useful that way. Putting this in the interactive option list is likely to have pretty narrow appeal, and not having it appear there could help reduce the cognitive load of dealing with duplicates.
• I wonder if we'd ever want to allow other criteria (besides average bitrate) for upgrading. It's pretty good, but Quality-based Trumping during import #116 (for example) has a lot of discussion of prioritizing specific LAME presets. This is certainly better left to future iterations, but I thought I'd just mention it to get started.

[Serene-Arc]

@sampsyo it does make more sense as a config specific option, can do that for sure. As for other variables for judging the quality, I broke out the function for judging the quality, so it'd be very simple to turn that into a generalised 'score' based on several different factors, though I'll have to look into what would be best for that.

[Serene-Arc]

So right now the files are only replaced when there's a replacement. However the tests are a bit messed up by this behaviour, I'll try and fix those next. Also, the new files are placed in a different directory, presumably because the folder name already exists. I'll need to fix this as well but some feedback on the progress so far would be nice. Not sure if it's going in the right direction.

[sampsyo]

Looking really good so far! Here are a couple of notes/suggestions on the code as it stands.

[sampsyo]

I confess I don't quite understand this logic yet. Is the idea to only remove duplicates that have MBID matches (as long as everybody has an MBID)?

If so, I wonder if that shouldn't be a separate feature… that is, the logic that decides what is a duplicate would seem to be somewhat orthogonal to deciding what to do with those duplicates, i.e., picking the best one for an "upgrade." Does this make sense?

[Serene-Arc]

This is a part that I did a quick and dirty fix and meant to ask about. I'm not entirely sure how to compare the tracks to know that they're duplicates in the first place. I know that they are duplicates by this stage, but actually finding the 1:1 match is hard because there's no unique ID in the file objects.

My initial thought was to use the MB ID since that's guaranteed to be unique to a replacement song, but not all tracks have that, so the fallback is those three fields. It's clunky and if there's a simple solution that I'm missing then please share, because this isn't a great solution.

[arsaboo]

I thought all tracks have MBID. Even if you use any other source, mbid is still populated - for example, when I use Spotify, the mbid is the Spotify track or album id respectively.

[Serene-Arc]

@arsaboo What if you disable the autotagger?

[arsaboo]

If you disable the autotagger (assuming that means as-is), will beet still prompt for duplicates? I will let @sampsyo confirm, but my feeling is we should leave those cases out as duplicates are not well defined there.

[sampsyo]

Ah, I see. No, with -A, there would be no duplicate prompt.

As for how to know that objects are duplicates: I may be just super slow here, but I would have expected self.duplicate_items(lib) to suffice? That is, if the question is "which items are duplicates of the current item?", then the answer could be "whatever self.duplicate_items(lib) returns." Does that make any sense at all?

[Serene-Arc]

@sampsyo For the importer, should it leave items that are not getting replaced in general, or is that something that should be only for the upgrade option?

Like, if an album has a missing track when it is imported but the existing album has that, currently the importer throws out everything, including the track that will not be replaced. Is that something I should fix in general or is that desired behaviour?

[sampsyo]

Good question. I think that is the desired behavior, in the sense that it's most predictable: "merging" albums by combining tracks from different bundles could easily become surprising for users, even if it has the downside that it yields "incomplete" albums more often.

[Serene-Arc]

Ah okay, then that makes everything much easier.

[arsaboo]

Good question. I think that is the desired behavior, in the sense that it's most predictable: "merging" albums by combining tracks from different bundles could easily become surprising for users, even if it has the downside that it yields "incomplete" albums more often.

I'm still trying to understand why removing all the tracks when adding one duplicate track (the current behavior) would be desirable or acceptable. Users can manually delete files if files come from different bundles, but loosing files may be a bigger worry.

[Serene-Arc]

@arsaboo I've already fixed that, files are only removed if there's something incoming to replace it. As I understand it, the fact that this wasn't the case already was a bug.

[arsaboo]

Awesome... did not realize that bug was fixed. Thanks 👍

[Serene-Arc]

I'm mostly done with this one, as far as I can tell it's working well. There probably just needs to be some tests added, either by me or someone else.

[arsaboo]

Tests are not required...so let us hope this gets merged soon 🎉

[Serene-Arc]

There is one thing I need to fix. The quality score function needs to be improved and maybe be per file. At the moment, for example, if you have a FLAC album with one track missing and upgrade an MP3 album, there will be one MP3 track in the album with the rest FLAC. This pulls down the total average bitrate so that the importer will always reimport the FLAC album because the average bitrate is higher than what's on disc.

[Serene-Arc]

Actually I'm not sure if this is even possible. When importing albums, there doesn't seem to be a way to separate or choose tracks, nor does that seem all that desirable. Perhaps it's simply an issue with using average bitrate over an album but some guidance on this would be nice. Should I try and make a solution that pics and chooses which tracks to add, rather than which ones to remove?

[sampsyo]

Ah, yes, indeed, separating out the tracks from an album seems quite a bit more complicated. How about we stick with the "average bitrate" thing for now (and only replacing complete albums) and revisit the "break albums apart into tracks" thing in a separate effort?

FWIW, my concerns about that are (a) it could be pretty hard to implement, given the importer's focus on complete albums, and (b) it could surprise some users, who might prioritize having "uniform" albums over having the best quality for individual tracks.

[Serene-Arc]

That seems like the way to go. There is that slight loss in efficiency but it can be revisited at a later date for sure. I think that the PR is done then, unless there's tests that you'd like to add. It seems to work pretty well from my manual checks,

[arsaboo]

@Serene-Arc looks like there are couple of lint errors still left.

[Serene-Arc]

@arsaboo sorry about that, fixed it.

[Serene-Arc]

Sorry I didn't realise that there's a docs command to test, it should actually be fixed now. Everything is running fine on my end.

[JOJ0]

Hi @Serene-Arc, I'd like to test this feature since I have a personal use-case for it often. It sounds like it is pretty stable already and capable of "exchanging" albums as a whole. Did I get that correctly?

And while I'm at it I might even come around to reviewing the code or at least the docs, we will see how that goes, since it is areas wher I didn't cover any ground yet in the codebase. But it sounds like it has been reviewed be @sampsyo already quite thorougly and maybe it's not far from "ready to merge"? What else do you think would help to get this one done?

HTH

[Serene-Arc]

It is capable of that. I'd like to take another look at it since I've learnt more about the code base since I've written it. Some more qualitative testing, just playing around with the feature, would be great, but I do need to add some tests that make sure the new feature is covered.

[JOJ0]

Hi,
I can't add much to implementation details at this point but thought I'll review the "user experience". I prepared a detailed report including cli outputs but I will spare you that and try to summarise what I think is worth mentioning. Hope that works out :-)

With

  duplicate_action: upgrade

I noticed the following:

• Trying to upgrade with exactly the same files looks like the album was imported newly - not very obvious that an upgrade was tried but it failed since there were no better quality files. Essentially from a user perspective this looks exactly as if a "skip" action was done, which is perfectly fine. 👍
• In verbose mode it shows that upgrade was actually choosen since it's configured but it's still not clear that it tried to upgrade but "it was not worth it". We'd just see:

Sending event: import_task_choice
found duplicates: [14]
default action for duplicates: u
Sending event: import
Sending event: cli_exit

but I think that's actually fine as well. 👍

• In a skipped upgrade with verbose we'd see exactly the same, except: default action for duplicates: s 👍

To summarize: A noop-upgrade is exactly the same as a skip.

Let's now move to

  duplicate_action: ask

Same album:

This album is already in the library!
Old: 8 items, MP3, 128kbps, 43:55, 40.4 MiB
New: 8 items, MP3, 128kbps, 43:55, 40.4 MiB
[S]kip new, Keep all, Remove old, Merge all?

Theoretically upgradeable album (plus verbose):

Apply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit, edit Candidates? a
Sending event: import_task_choice
found duplicates: [7]
default action for duplicates: a
This album is already in the library!
Old: 8 items, MP3, 128kbps, 43:55, 40.4 MiB
New: 8 items, MP3, 257kbps, 43:55, 83.0 MiB
[S]kip new, Keep all, Remove old, Merge all?

I know that @sampsyo suggested making the feature "configuration only" for now, which there might be good reasons, one being to not "add additional cognitive load" to the importer. I get that and I had major problems myself when I started out with beets and asked several times about that (Thanks @sampsyo you helped me out often ;-)

But I think though that during the last year there was a slight improvement in wording in the duplication prompt and I think these added to clarity of what it actually does a lot (eg. instead of "Remove", we now have "Remove old".

So that's why I actually wanted to make a proposal of how this new option could look like and went with some brainstorming but I hate to admit that I don't come to a conclusion what would be easily understandable here and this might even be the reason why @sampsyo was suggesting to make this "config only" for now.

This album is already in the library!
Old: 8 items, MP3, 128kbps, 43:55, 40.4 MiB
New: 8 items, MP3, 257kbps, 43:55, 83.0 MiB
[S]kip new, Keep all, Replace old, Merge all? 

or

[S]kip new, Keep all, Upgrade old, Merge all? 

or

[S]kip new, Keep all, Upgrade, Merge all? 

So, for now, I hope these example outputs clarify a little how this feature is supposed to work and what questions arise and motivates someone else maybe to brainstorm about it. HTH :-)

[arsaboo]

We can just keep it simple Upgrade (I prefer this over Upgrade old - as old is redundant here). This option could just upgrade the duplicate files and not remove the entire album even if one file is upgraded.

@JOJ0 would that be acceptable? I think that may be a more straightforward fix and a faster merge. The current behavior (of removing the entire album) is WRONG (also see #4755).

[Serene-Arc]

I still need to do some work and write some tests. There's still some things to be done. I've just had an unfortunate mix of health issues and semester crunch to work with, so it's been somewhat difficult finding the time

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Serene-Arc]

I'd like to go back over my logic for this and the code additions, as well as the merge conflicts from the UI overhaul. As such, I'll wait until after the formatting change is merged.

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[arsaboo]

I would love for this to be finished...

[Serene-Arc]

Yeah, what needs to happen is I think I need to restart it from scratch. This was the first thing I started contributing and I know the codebase a lot better now. It's not dead though!

[JOJ0]

Removed the "review-needed" label since you mentioned that if you continue it, you will start from scratch. Also set it to draft. I guess that is ok with you @Serene-Arc :-)

[Serene-Arc]

Yup! Thanks

[snejus]

@Serene-Arc Are you planing to come back to this?

[snejus]

I'll close this for now, but feel free to reopen it whenever you're ready @Serene-Arc!

[arsaboo]

@Serene-Arc I created a draft PR #6842 based on all the discussion in this PR. I hope that is fine. Happy to close it if you have the bandwidth and feel like working on this feature.

[Serene-Arc]

Not a problem at all! Given how much time has passed, I'd be starting from scratch again anyway so I'll take a look at yours @arsaboo

[arsaboo]

Thanks and please feel free to add commits to that branch. I would really like to get this feature merged.