[Sabrina04072003]

Description

Fixes #6830.

During an import, the lastgenre plugin prints the fetched genres to the terminal. This output can appear on top of the importer's prompt for the next album, making the import look frozen.

This PR fixes the issue by preventing the imported hook from printing genre changes during an import. The genres are still fetched and saved, and beet lastgenre continues to display changes as before.

I investigated the code to identify the likely cause. I added a regression test to verify that the import stage does not print anything and that the genres are still assigned correctly. I also tested the change on Windows and confirmed that the new test fails with the original code and passes with the fix.

If someone can reproduce the original issue, I would appreciate confirmation that this fix resolves it.

AI usage

I used Claude to help me with this contribution. I ran the tests and linter myself .

[henry-oberholtzer]

Looks like this completely removes the ability to view changes for the user when running automatically, which is an unfortunate loss of information communicated to the user. I'd personally rather see this implemented as a configuration option when running in auto mode, as being able to choose between viewing the output or not is useful.

I think the crux of this issue has to do with parallelism, and the fact that the plugin only triggers on a completed import, which can run in a different thread, and may lag behind the UI updates for the user.

It's also tricky, as until a candidate is chosen, choosing the right genres from Last.FM based on information we're attempting to tag would be inaccurate. A possible workaround would be to run LastGenre per candidate, before they're presented to the user, and show the tags with the candidate, but that might be an excessive number of extra queries to make.

I'll defer to @JOJ0 on this though since they know the most about the plugin!

[JOJ0]

@henry-oberholtzer but in general when a plugin is doing something in auto: yes mode during an import, we don't show it.

[JOJ0]

@henry-oberholtzer but in general when a plugin is doing something in auto: yes mode during an import, we don't show it.

So what I'm trying to say Henry, is that actually printing out the changes (ui.show_model_changes() here is actually always been a misbehavior of this plugin that I didn't notice myself since I don't have auto enabled for it.

What at first I was not sure about and that would have been a question for @snejus is if deciding to show output is something to be decided in the plugin code or rather be solved within that show_model_changes function already? From investigating other use-cases of this function, I can see that show_model_changes should not decide that itself, since it is used in a lot of places and a versatile helper.

So I think this change is legitimate :-)

[JOJ0]

Hi @Sabrina04072003 could you rebase this branch with master please? I'll review it then. Thank you!

[henry-oberholtzer]

Thanks for clarifying @JOJ0 ! That makes a lot of sense. I think this is a great fix then & also happy to approve once rebased.

[snejus]

Hmm I'm not entirely sure we want to hide the output altogether. For example, lyrics plugin also prints stuff in auto mode, and as a user I want to see that output!

This output can appear on top of the importer's prompt for the next album, making the import look frozen.

@Sabrina04072003 are you importing using threaded: yes?

[JOJ0]

I agree that it's useful information but does it look pretty enough? How and where does lyrics print it? Can you print an example output.

Also for lastgenre an example output would be nice here (too lazy to test myself ATM. can do later!)

[snejus]

See

  MusicBrainz, George Michael, Careless Whisper, 7" Vinyl, 1984, 1984, None, None, A4603, Epic, single, Official, XE, ['pop', 'blue-eyed soul', 'electronic', 'smooth jazz', 'smooth soul', 'sophisti-pop', 'soul', 'ballad']
  https://musicbrainz.org/release/c5d0895c-ce45-4def-bd6f-6a2a46a6d71f
  * Artist: George Michael
     ≠ (#0) Creless Whisper (5:03) -> (#1) Careless Whisper (5:04)
Missing tracks (1/2 - 50.0%):
 ! Careless Whisper (instrumental) (#2) (5:02)
➜ [A]pply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit, edit Candidates? 
Sending event: import_task_choice
Sending event: import_task_apply
0 of 1 items replaced
Sending event: database_change
Sending event: database_change
Sending event: database_change
Sending event: database_change
fetchart: trying source Bandcamp[default] for album George Michael - Careless Whisper
bandcamp: Not fetching art for a non-bandcamp album URL
fetchart: trying source filesystem[default] for album George Michael - Careless Whisper
fetchart: trying source cover_art_url[default] for album George Michael - Careless Whisper
fetchart: Cover art URL not found for #12891: Album Careless Whisper by George Michael | Epic | A4603 | 1984 | XE
fetchart: trying source lastfm[default] for album George Michael - Careless Whisper
fetchart: getting URL: https://ws.audioscrobbler.com/2.0?method=album.getinfo&api_key=4924d46cb9d421095c8a6926a86ecbe0&mbid=c5d0895c-ce45-4def-bd6f-6a2a46a6d71f&format=json
artresizer: method is ImageMagick
fetchart: downloading image: https://lastfm-img.freetls.fastly.net/i/u/300x300/1a7a9b256cbc7f4f5b030585f569620a.jpg
fetchart: downloaded art to: /tmp/beets/beetsplug_fetchart/6tax7efs.jpg
fetchart: image size: (300, 300)
fetchart: using remote image /tmp/beets/beetsplug_fetchart/6tax7efs.jpg
fetchart: image size: (300, 300)
Parsed query: OrQuery([AndQuery([SubstringQuery('data_source', 'bandcamp', fast=False)]), AndQuery([RegexpQuery('genres', re.compile('techno'), fast=True)]), AndQuery([SubstringQuery('data_source', 'soundcloud', fast=False)]), AndQuery([SubstringQuery('albumstatus', 'bootleg', fast=True)]), AndQuery([SubstringQuery('albumtype', 'broadcast', fast=True)])])
Parsed sort: NullSort()
lyrics: LyricsPlugin: Fetching lyrics for George Michael - Careless Whisper
lyrics: LRCLib: Fetching JSON from https://lrclib.net/api/search?artist_name=George+Michael&track_name=Careless+Whisper
lyrics: LyricsPlugin: 🟢 Found lyrics: 59214 | 1984 / Careless Whisper: George Michael - Careless Whisper
lyrics: Translator: 🔵 Lyrics are already in the target language EN

Maybe we should only print the output in verbose -vv mode?

[JOJ0]

Thanks @snejus , but that is different situations we are comparing. lyrics seems to output already as DEBUG or INFO log. lastgenre ussually does that but only the show_model_changes output is printed "normally".

So I agree we should print it only as actual log, but I'd still want to see that (very helpful) output in standalone runs of the plugin. So let's make it an INFO log rather than a DEBUG log.

So insetad of

if show_changes:

do

if self._log.isEnabledFor(logging.INFO):

[JOJ0]

or @snejus another idea: ui.show_model_changes() gets a flag to be passed telling it to log instead of regular printing? What do you think?

Do _print():

ui.show_model_changes()

Log info:

ui.show_model_changes(output=logging.INFO)

Log debug:

ui.show_model_changes(output=logging.DEBUG)

[snejus]

@JOJ0 I think I prefer the initial idea using isEnabledFor - let's generalise it later once we got multiple places that needs it :)

[JOJ0]

Hi @Sabrina04072003 do you want to make the here suggested changes #7107 (comment) or is it ok if I push them? (I have it locally already), either way is fine, just tell us if you will find the time to come back to finishing this PR! Thanks for your submission!

[Sabrina04072003]

Hi @JOJ0, thank you for the suggestions! I will make the changes myself (using self._log.isEnabledFor(logging.INFO)) and rebase on master today or tomorrow.

@snejus about your question: I did not reproduce the whole scenario with a real interactive import. It needs Last.fm and MusicBrainz access and albums that wait for input, and I did not set that up. I found the likely cause by reading the code: show_model_changes() prints directly to the terminal during the import stage. threaded is yes by default in beets' default config, so I think this stage can run in another thread while the importer waits for input on the next album.

What I did check: with automated tests (also on Windows), the import stage printed the genre changes with the original code, and with the fix it prints nothing by default.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.67%. Comparing base (e4fdcf5) to head (d036ecc).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7107      +/-   ##
==========================================
+ Coverage   80.65%   80.67%   +0.01%     
==========================================
  Files         150      150              
  Lines       21298    21300       +2     
  Branches     3294     3295       +1     
==========================================
+ Hits        17178    17183       +5     
+ Misses       3291     3289       -2     
+ Partials      829      828       -1     

Files with missing lines	Coverage Δ
beetsplug/lastgenre/__init__.py	90.87% <100.00%> (+1.04%)	⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JOJ0]

maybe shorten this. a very short comment why it is here would be sufficient, leaving out the technical details we read in the code anyway. thanks

[JOJ0]

Please rebase your branch with master again, I hope you don't get too much conflicts, there were quite some linting/code-style changes in the last days that might lead to such. Other than that, the regression tests look ok and we can merge it once rebase is done. Thank you!