Lastgenre: Fix track-level handling, multi-genre keep, force behaviour, logging #4982

JOJ0 · 2023-10-29T13:52:42Z

Description

Edit 2023-09: The original idea of this PR was:

Several fixes I had in the queue for months. Some of it required fixes in the library code which are through by now.

Fix the force option: Don't always overwrite comma-separated multi-genres, compile a list and keep what's in the whitelist.
Fix lastgenre -A in combination with config option source: track - Tracks receive the album's genre even when this option is set
- When an album-level genre is set already, single tracks should don't fall back to receiving the album's genre.
Adjust log-level and message when lastgenre handles tracks to look similar to when handling albums.

Edit 2023-09: Additional option `keep_allowed`

During review and discussions it turned out that besides the existing force option a second option would be required to really achieve a typical (expected) behaviour of a force option. This is what we came up with (copied over and slightly edited from #4982 (comment)):

Two config options, force and keep_allowed, i.e. 4 possible settings:

Possible setups with keep_allowed/force

Case 1

Overwrite all. Only fresh last.fm genres remain.

force: yes
keep_allowed: no

Case 2

Add new last.fm genres when empty. Present tags stay untouched.

force: no
keep_allowed: no

Case 3

Add new last.fm genres. Keep whitelisted genres in present tags.

force: yes
keep_allowed: yes

Case 4 (default)

Add new last.fm genres when empty. Keep whitelisted genres in present tags.

force: no
keep_allowed: yes

Edit 2025-01: Change option name `keep_allowed` to `keep_existing`

which feels slightly more self-explanatory. Also decide on Case 3 as default
(force always was default, with keep-existing it might be even more helpful)

Possible setups with keep_existing/force

Setup 1

Overwrite all. Only fresh last.fm genres remain.

force: yes
keep_existing: no

Setup 2

Add new last.fm genres when empty. Present tags stay untouched.

force: no
keep_existing: no

Setup 3 (default)

Add new last.fm genres. Combine genres in present tags with new ones
(depending on the setting, whitelisted or any).

force: yes
keep_existing: yes

Setup 4 (currently not supported)

Add new last.fm genres when empty. Keep whitelisted (or any) genres in present tags.

force: no
keep_existing: yes

Why is it not supported?

After testing and playing around with this option combination a lot I realized two things:

It is pretty likely that a the outcome is empty tags when the whitelist is active. For example when non-whitelisted genres are in tags already the only thing this option combination does is kick out genres. Any untidy genre like "DnB, Drum n bass" instead of the whitelisted version "Drum And Bass" will be kicked out. This very often can lead to just empty genres.

Therefore I decided to leave this combination unsupported. A much better alternative if the user would like to keep existing genres ALWAYS but use lastgenre with entirely empty tags is just setting force off and keep_existing off. This would ensure never-ever pre-polutated tags will be touched while enjoying the help of lastrgenre for ebntirely empty tags.

To Do

JOJ0 · 2023-11-02T16:35:21Z

I'd request a review from you @sampsyo since I think you initially created it. Also @rain0r would be good since 5 years ago they added the -A option. Hi @rain0r , you wanna take a look? :-)

In short: I think I fixed the plugin to now really reflect what's documented. Any nitpicking in my code or functionality-wise is appreciated.

One question already. Here we do not state that a -a/--album option exists: https://beets.readthedocs.io/en/latest/plugins/lastgenre.html#running-manually

When I started out with using this plugin I was confused a verry long time about this option. As far as I understand it now: It doesn't do anything since it is default. So why keep it? Or is having a -a option that is the default anyway a common thing in beets? I know we have a lot of -a commands which streamlines usablity, and that is a very good thing! Usuall they change behaviour to not do something with items but with albums. I'm just not sure about this one....do we have such a pattern anywhere else? So, just leave it? Should I add some words to the docs?

I think the both of you decided these options should look like that around here: #3220 (comment)

sampsyo · 2023-11-03T15:11:54Z

Thanks for the extra context, @JOJ0!

About the existence of -a (the default mode) specifically: it's not too uncommon… for example, the beet import command has several flags that are opposites of each other, one of which is the default. Of course, it's important in that case because the default mode can be set in the config, so the user needs a way to override the default in either direction. That's not the case here, so maybe it at least makes sense to add "(default)" to the -a option's help string, or to remove it altogether?

sampsyo

Thanks for the ping!! Here are a couple of straightforward comments.

beetsplug/lastgenre/__init__.py

JOJ0 · 2023-11-17T08:57:27Z

I'd like to pull out this conversation #4982 (comment) into a new thread, to make it more obvious for others as well. I think it could be a broader discussion of where this plugin should go. Basically we were talking about the current force: no behaviour being weird as well as the new behaviour I am initially proposing with this PR. I gave all this some thought and came up with this idea. Let's discuss it:

So from my point of view, the main problem with the current behaviour when force is disabled, is that it's not really what a user would typically expect. So what could we do to make force: no more predictable?

The following idea would require a new config setting as well as a whole new branch of behaviour (Case 3):

Case 1

force: yes
overwrite all, only fresh last.fm genres remain

Case 2

force: no

keep any string in present genre tag, only write last.fm genres when empty

Case 3

force: yes
keep_allowed: yes

keep present genres when whitelisted and add new last.fm genres (this is a new branch of behaviour and needs to be coded, I think there is open feature requests for it. Update: Something was feature-requested, but it might not be exactly as I'm proposing here: #4750)

Case 4

force: no
keep_allowed: yes

cleanup only - keep present genres when whitelisted but don't add new last.fm genres; Only when genre is empty, add last.fm genres.

That last combination is weird though....but it's what I proposed for force:no before!

Which of these would now make sense to be the new default? The new force: no (Case 2) would be the least invasive IMO...

@sampsyo brainstorming request 🧐

JOJ0 · 2023-11-17T09:14:04Z

Some more context / cross-linking:

The initial reason why I got my hands dirty with this plugin was when I realised that comma separated multi-genres where not recognized: #4751 (comment)

Here @arsaboo requests a feature that goes in direction of Case 3 above: #4750

arsaboo · 2023-11-17T16:07:48Z

So, we have two config options - force and keep_allowed, i.e., 4 options in all. Given that, keep_allowed is no in cases 1 and 2. Thus, here's a slightly modified behavior in the 4 cases above:

Case 1: overwrite all, only fresh last.fm genres remain

force: yes
keep_allowed: no

Case 2: Since keep_allowed is no, we only write last.fm genres when empty. There may be incorrect genres in pre-existing tags even after this, as this option is not touching pre-existing tags

force: no
keep_allowed: no

Case 3: keep present genres when whitelisted and add new last.fm genres

force: yes
keep_allowed: yes

Case 4: keep any string in the present genre tag; only write last.fm genres when empty. This will not touch pre-existing genre tags.

force: no
keep_allowed: yes

Thus, Case 4 seems like the best default choice. It does not affect existing genre tags and updates the empty ones. Case 3, on the other hand, is the most useful one (at least for me).

sampsyo · 2023-11-17T22:23:16Z

This brainstorming honestly sounds great, y'all. It is indeed really weird that the force: no mode can still update old genres; keeping all nonempty genres seems like it should at least be an option. I feel less specific about what the default should be, but I like your idea about decoupling the two aspects of the behavior (when to override existing, nonempty data and what to do to old data) into two different options.

JOJ0 · 2023-11-18T16:16:38Z

Ähem I might be slow or too tired already. Which of those 4 cases are now different from my proposal @arsaboo ? Sorry I must have missed it! Help! :-)

arsaboo · 2023-11-18T16:34:38Z

Not different....just a little more explicit about the force and keep_allowed config options. I think we have an agreement about the options.

github-actions · 2024-09-17T16:38:37Z

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.

JOJ0 · 2024-09-17T16:43:54Z

Hi @arsaboo! I finally managed to find time to almost finish this PR. The general behaviour and docs of the new config options combinations are finished. If you want to, an "early" review would be super helpful. Since it probably also for you is a long time ago it might be interesting what you think if you read through the docs. Is it 100% clear what force/keep_allowed options do? Certainly but only if you have the time, some playing around and checking if it also really works that way would be great. Thanks a ton!

arsaboo · 2024-09-17T20:18:17Z

@JOJ0 this is AWESOME 🎉🎉

The docs look reasonably clear. I will play with this. The debug logs are great to see what is going on.

in lastgenre plugin.

generation, instead of a simple set(). This way we keep the original order of genres.

- Default to False. - During PR#4982 discussions we came to the conclusion that the following behaviour would be a good new default choice: - Keep whitelisted existing genres - Only Fetch last.fm genres for empty tags. - To get this we also have to change the default of the force option!!! - Resulting in "force: no" and "keep_allowed: yes"; see Case 4 in PR#4982 description - Options are not put to use yet, just defined and defaults set!

Keep both options' "Configuration" chapter texts as compact as possible, while linking to a new chapter that describes all 4 possible combinations in detail.

- Retrieving, filtering and deduplicating present genres of Items/Albums via separate methods. - Implement all four cases of behaviour as described in PR#4982 - Issues: - There is quite some unnecessary spliting of genres from strings into lists and the other way round happening throughout the plugin. - In the case where existing genres get "augmented" with last.fm genres, we might end up with _more_ genres than the configured limit.

- Handle genre combination logic in a well documented helper function that also include type hints. - Throughout the _get_genre function rename the result variable to new_genres to make it clearly descriptive. - Rewrite thze _get_genre function's docstring.

trying to get a little order in the chaos. Maybe reordering and/or moving out of the main plugin logic would be a better idea for some methods but don't put much more refactoring into this PR to keep it readable.

when not stated otherwise whitelist_only must be disabled, we assume it that way in _get_genre calls.

This was the original behaviour and broke when _combine_and_label helper was introduced.

- Refactor and simplify logic of _get_genre() - Add a config validation function. - New default force: yes, keep_existing: yes (closest to original behaviour)

and decide to use the original default whitelist instead of trying to mock it. Some of the existing tests do it that way as well.

When original genres were kept (keep_existing option), the final genre count was "off". The reason was that reducing genres to that count is handled in _resolve_genre which wasn't run. - This fixes it by ensuring a run of _resolve_genre in _combine_and_label_genres. - There is a small caveat though: New genres have been run through _resolve_genres already. When they are combined with the old ones, they run through it again. Let's take this into account for now and hope performance doesn't suffer too much.

- Return fetched genres as a list from _resolve_genres(). - Format, limit to count and join to delimited string in helper function. - Fix docstring. - Leave a couple of temporary debug messages. - Fix original genre fallback - just keep as-is.

- No idea where a missing separator (which is default) could happen...just set it explicitely. - Since we now refactored fetch_genre to returning a list we can add mock multiple fetched gernes easier.

- Add detailed debug logging to learn when and why things go wrong here. - Shorten docstring

- _resolve_genres returns a list with not yet formatted genres. - Rename and adapt test_count -> test_to_delimited_string

otherwise deduplicate() can't handle it.

JOJ0 requested a review from sampsyo November 2, 2023 16:27

JOJ0 marked this pull request as ready for review November 2, 2023 16:27

JOJ0 added a commit to JOJ0/beets that referenced this pull request Nov 2, 2023

Add changelog for beetbox#4982

ceed54a

sampsyo reviewed Nov 3, 2023

View reviewed changes

beetsplug/lastgenre/__init__.py Outdated Show resolved Hide resolved

beetsplug/lastgenre/__init__.py Outdated Show resolved Hide resolved

beetsplug/lastgenre/__init__.py Outdated Show resolved Hide resolved

JOJ0 force-pushed the lastgenre_fixes branch from 8450a09 to a8479d2 Compare November 4, 2023 10:22

JOJ0 marked this pull request as draft November 8, 2023 08:08

JOJ0 force-pushed the lastgenre_fixes branch 2 times, most recently from 1e81209 to 89ae925 Compare November 16, 2023 12:33

JOJ0 added a commit to JOJ0/beets that referenced this pull request Nov 16, 2023

Add changelog for beetbox#4982

c8966e5

JOJ0 force-pushed the lastgenre_fixes branch from 89ae925 to 31ce30d Compare November 16, 2023 15:28

JOJ0 changed the title ~~Lastgenre: fix track-level handling, fix multi-genre keep, streamline singleton log~~ Lastgenre: Fix track-level handling, multi-genre keep, force behaviour, logging Nov 17, 2023

JOJ0 force-pushed the lastgenre_fixes branch 2 times, most recently from fb9f58d to c12b26b Compare September 17, 2024 16:34

JOJ0 marked this pull request as ready for review September 17, 2024 16:38

JOJ0 marked this pull request as draft September 17, 2024 16:39

JOJ0 force-pushed the lastgenre_fixes branch 2 times, most recently from 796a3bf to a56098f Compare October 31, 2024 14:47

JOJ0 force-pushed the lastgenre_fixes branch 2 times, most recently from 217aa33 to 8138708 Compare January 2, 2025 10:17

JOJ0 added 29 commits January 7, 2025 01:47

Use separator as configured instead of hardcoding

9c09074

in lastgenre plugin.

Use provided deduplicate function for keep_allowed

3b339c8

generation, instead of a simple set(). This way we keep the original order of genres.

Docs for lastgenre keep_allowed/force

0704034

Keep both options' "Configuration" chapter texts as compact as possible, while linking to a new chapter that describes all 4 possible combinations in detail.

Quickfix constant msgpack poetry issue

f1247e9

lastgenre: Add comments over groups of methods

197a26f

trying to get a little order in the chaos. Maybe reordering and/or moving out of the main plugin logic would be a better idea for some methods but don't put much more refactoring into this PR to keep it readable.

Experiment with test_lastgenre

a069e50

Fix default for _dedup_genre whitelist arg

f4ffc9f

when not stated otherwise whitelist_only must be disabled, we assume it that way in _get_genre calls.

_combine_and_label return None not empty str

0dd4ef4

Fallback to next stage when fetch_ returns None

5892870

This was the original behaviour and broke when _combine_and_label helper was introduced.

Refactor _get_genre test to parametrized pytest

b81f4ca

Add lastgenre testcase with unicode \0 separator

7a60c91

Temporary lastgenre debug messages for @arsaboo

b0fac0d

Clarify lastgenre _is_allowed docstring

b7f8716

Rewrite docs for keep_allowed/existing change

f86d483

Rename lastgenre option, refactor, new default

009e965

- Refactor and simplify logic of _get_genre() - Add a config validation function. - New default force: yes, keep_existing: yes (closest to original behaviour)

lastgenre test _get_genre for renamed keep_existing

c1253f4

and decide to use the original default whitelist instead of trying to mock it. Some of the existing tests do it that way as well.

Refactor lastgenre mocked fetchers return list

cb013d1

Fix/add lastgenre fallback tests

2c7f54f

Fix lastgenre limit to count test

3107fbe

- No idea where a missing separator (which is default) could happen...just set it explicitely. - Since we now refactored fetch_genre to returning a list we can add mock multiple fetched gernes easier.

lastgenre _is_allowed detailed logging

7ccedcb

- Add detailed debug logging to learn when and why things go wrong here. - Shorten docstring

Refactor again _combine_and_label_genres

399c8a9

Refactor resolve_genres tests, rename count test

d2fa51c

- _resolve_genres returns a list with not yet formatted genres. - Rename and adapt test_count -> test_to_delimited_string

Add new test for _resolve_genres (popularity sort)

d9a4801

Fix _dedup_genre, ensure lower case

2b75847

otherwise deduplicate() can't handle it.

JOJ0 force-pushed the lastgenre_fixes branch from a8efc71 to 2b75847 Compare January 7, 2025 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lastgenre: Fix track-level handling, multi-genre keep, force behaviour, logging #4982

Lastgenre: Fix track-level handling, multi-genre keep, force behaviour, logging #4982

JOJ0 commented Oct 29, 2023 •

edited

Loading

JOJ0 commented Nov 2, 2023 •

edited

Loading

sampsyo commented Nov 3, 2023

sampsyo left a comment

JOJ0 commented Nov 17, 2023 •

edited

Loading

JOJ0 commented Nov 17, 2023

arsaboo commented Nov 17, 2023 •

edited

Loading

sampsyo commented Nov 17, 2023

JOJ0 commented Nov 18, 2023

arsaboo commented Nov 18, 2023

github-actions bot commented Sep 17, 2024

JOJ0 commented Sep 17, 2024 •

edited

Loading

arsaboo commented Sep 17, 2024

Lastgenre: Fix track-level handling, multi-genre keep, force behaviour, logging #4982

Are you sure you want to change the base?

Lastgenre: Fix track-level handling, multi-genre keep, force behaviour, logging #4982

Conversation

JOJ0 commented Oct 29, 2023 • edited Loading

Description

Edit 2023-09: The original idea of this PR was:

Edit 2023-09: Additional option keep_allowed

Case 1

Case 2

Case 3

Case 4 (default)

Edit 2025-01: Change option name keep_allowed to keep_existing

Setup 1

Setup 2

Setup 3 (default)

Setup 4 (currently not supported)

To Do

JOJ0 commented Nov 2, 2023 • edited Loading

sampsyo commented Nov 3, 2023

sampsyo left a comment

Choose a reason for hiding this comment

JOJ0 commented Nov 17, 2023 • edited Loading

Case 1

Case 2

Case 3

Case 4

JOJ0 commented Nov 17, 2023

arsaboo commented Nov 17, 2023 • edited Loading

sampsyo commented Nov 17, 2023

JOJ0 commented Nov 18, 2023

arsaboo commented Nov 18, 2023

github-actions bot commented Sep 17, 2024

JOJ0 commented Sep 17, 2024 • edited Loading

arsaboo commented Sep 17, 2024

JOJ0 commented Oct 29, 2023 •

edited

Loading

Edit 2023-09: Additional option `keep_allowed`

Edit 2025-01: Change option name `keep_allowed` to `keep_existing`

JOJ0 commented Nov 2, 2023 •

edited

Loading

JOJ0 commented Nov 17, 2023 •

edited

Loading

arsaboo commented Nov 17, 2023 •

edited

Loading

JOJ0 commented Sep 17, 2024 •

edited

Loading