lastgenre: Plugin tuning log (-vvv)
#6007
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntroduce a centralized _debug_extended wrapper for conditional extended_debug logging and refactor existing debug calls to use it, streamlining debug output management in the lastgenre plugin. Class diagram for updated logging in lastgenre pluginclassDiagram
class LastGenrePlugin {
+_debug_extended(msg, *args, **kwargs)
_last_lookup(entity, method, *args)
config
_log
_genre_cache
fetch_genre()
fetch_album_genre(obj)
_load_whitelist() set[str]
_load_c14n_tree()
whitelist
c14n_branches
canonicalize
}
LastGenrePlugin : _debug_extended is new
LastGenrePlugin : _last_lookup uses _debug_extended for logging
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a wrapper method to simplify extended debug logging in the lastgenre plugin. The change aims to reduce code clutter by centralizing conditional debug logging that is used when tuning whitelist and canonical files/settings.
- Adds a
_debug_extendedhelper method that conditionally logs debug messages based on theextended_debugconfiguration - Refactors existing extended debug logging to use the new wrapper method
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
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. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6007 +/- ##
==========================================
- Coverage 66.98% 66.98% -0.01%
==========================================
Files 118 118
Lines 18191 18192 +1
Branches 3079 3079
==========================================
Hits 12186 12186
- Misses 5346 5347 +1
Partials 659 659
🚀 New features to boost your workflow:
|
Intentionally not added an entry. Dear @beetbox/maintainers tell me if you think I should add one. Otherwise I think this is a no-brainer. Thanks for a quick approval. I wasn't sure about the name of that wrapper method. There might be room for improvement but on the other hand it's not super-important how it's called. |
JOJ0
force-pushed
the
lastgenre_ext_debug
branch
3 times, most recently
from
88fcbb4 to
1cc61d8
Compare
|
Short question: What is the exact benefit for this over normal debug messages? In theory you can edit the loglevel for each child(plugin) logger separately. I.e. Set lastgenre to debug and everything else to info or warning. If have not tested it, but I think something like this should work: class TestPlugin(BeetsPlug):
def __init__():
super.init()
self._log.setLevel("DEBUG") # Overwrite plugin logging |
|
Interesting, I've never heard about this possibility. I thought we are pretty stuck with the possibilities of info and debug log. We see info logs while executing plugins directly And debug only with -v And during import we don't even see info logs I thought that's kind of a fixed pattern we should follow The actual purpose of this to get extensive debug logging to really be able to tune lastgenre settings/files (future features!) and this should only be available to those who want it / need it (configurable) |
Let me rephrase: I know about loggers and levels and that I can change them but I kind of thought this is better not to be touched in beets and there is some thought/magic behind it. In other words, do you think this usecase can be done with your idea: in short: "configurable additional logs":
And do I understand correctly? You are saying I could shift all logging "up one level" info logs -> warning logs (actually the regular logs we see when running eg. and error logs stay error logs? Before I start digging and since you were deep into the plugin ecosystem recently @semohr : Where actually is it decided/redirected that those info logs we see during explicit plugin execution are not shown during import? This might all be dumb questions, but I never really questioned that it's an option in beets to fiddle with logging levels. Is something like this done anywhere else in beets yet? |
|
There is no need to create a helper method if that method is called only once. |
|
@JOJ0 Let quickly outline the requirements here as I'm a bit confused now: If yes, we should achieve that with a proper logging setup instead of a conditional. I had a look into the relevant code: At the moment each plugin creates a child logger. These loggers are assigned level 0 at the moment which might be the issue you are seeing when trying that. I think a generic way to configure each child logger in beets (each plugins logging) would be the correct way to continue here, even though it will take more effort and will most likely need some slight changes in core. Something like @snejus I think the function will be called more in the future and is just a preparation PR for more to come. This was a great choice imo as we can now properly talk about this one issue 👍 |
I'll explain it with my current situation to hopefully avoid confusion: I already use a lot of info and debug logs in the plugin. I want to be able to log even more debug logs but only when configured (or |
|
I see now. Please dont take this the wrong way! Maybe it is time to rethink/refactor the logging than. I would normally not expect anyone to need more verbose logging than debug level 😆 In my view, logging here should mainly serve two purposes: If the debug logging is just for developing purpose maybe switching to pdb for debugging and using breakpoints might be more useful. Of course this is subjective, so take it as you will, but to me, adding another debug level feels more like a workaround. A more maintainable approach might be to step back and reconsider whether all the existing logging is truly necessary in the first place. |
|
the logging is not for debugging/developing, it is for tuning the last.fm tree/whitelist/blacklist/aliases settings of lastgenre plugin users. we can't expect them to use pdb ore some kind of dev-centric tools. maybe you can't see the full picture yet because most of these features dont exists yet? I hope it's now more clear what the goal is here :-) |
Very much possible! Well if you feel like this is necessary I'm fine with approving it. Even though it seems a bit strange to me. |
again: this is for the future features i'm working on! i mentioned that here: #6007 |
|
thank you @semohr yes I think this is the right way! Are you a lastgenre user yet? |
I haven't made some time yet to configure it properly for me. But it is on my todo list :) |
go ahead and dot it and maybe you will then better understand my reasoning here :-) My personal opinion about this plugin is that it still does not have all the required features to really make it useful. last.fm is a beautiful but errorprone beast! you can't expect it to do the right thing always. it depends on how the user wants it. If we put everything that might be of relevance for that tuning, the already horribly unreadable beets log will have more last.tm logs than anything. maybe I just named this option wrong and it should be called something like |
|
Since I still have the feeling that I need to explain myself better here. Even with the current version of the plugin, we see the urge for this extended logging option already: This is an example for a various artists album. It is very interesting to see which things are considered during artist genre matching until a final artist genre can be decided on: now if we do the same without the So to summarize this in numbers, the extended debug log version consists of 189 lines (!) and contains what we actually get from last.fm - this is very important information if you want to know what to write into your whitelist and canonicalzation files! The "normal" version only contains 68 lines. I hope I coudl give you a better understanding now, why I think this additional level of information is useful for users but should not go into the general debug log - it is just too much! |
|
Makes sense! Just to throw yet another idea in here. How about creating another child logger for this information? This way it is also easier to filter it? And you can set the log level with a config option in the lastgenre plugin? |
|
I'm cherry-picking a couple of bits and pieces from this convo
We see the same logs during the import (as long as we provide
|
@snejus I'm pretty sure that you know... that I know.... that .... :lol: Reading thru the context here, I probably ment that we don't see it during a regular import (no |
Can never be sure! Well, only yesterday I learned that for some reason one needs to provide double v to the |
JOJ0
force-pushed
the
lastgenre_ext_debug
branch
2 times, most recently
from
2c35307 to
9573432
Compare
Fair enough! I have to admit that only now that you say it, I did kind of knew that, or at least I had known that once 🤣 but definitely forgot the details and needed this reminder. Yes this is something I always found a bit awkward. What I learned during this additional logger experiment is that generally python loggers spit out on stderr by default. At first I found the idea very tempting that I could redirect this additional logger to stdout to simplify filtering with So in short this is all too complicated and yes I agree we would access plugin-territorry that might not be expected and streamlined behavior anymore. So yes please, let's go back to a simple solution. I deleted my experiments, went back to the initial solution (also no renaming of the option/config) and rebased with current master. |
There was a problem hiding this comment.
Sorry for derailing this a bit. Thought there might be a better way 🙃
We may want to have a look at our logging setup in general tho. There might be some room for improvements, e.g. setting logging levels for plugins could be a bit more configurable.
Try |
|
Note that the problem I described is still present:
|
if extended_debug is enabled in the config you see it. |
I'd like to add: In that sense I'm understanding it to be consistent and expected plugin behavior. Think about it in general:
Does that help @snejus? |
|
But why adding this extra step? And why When I use What prevents you from including this extra debugging output as part of the normal debugging logs? |
Dear @snejus, we are running in circles here. Please read the why's I already posted a while ago repeatedly. @semhor linked it for you. I think I explained in detail already why I don't think it's a good idea to include all this in the regular log. Please try to find the time to read again. Many thanks! Let's get back to being productive. Please! Thank you! I'll do you the favor and post it again. Start reading here: #6007 (comment) Thank you very much! |
|
|
OK, an idea: what about checking the global level, i.e., the number of v's given for the verbose option in See class LastGenrePlugin(plugins.BeetsPlugin):
def __init__(self):
print(" --- VERBOSITY --- ")
print(config["verbose"].as_number())
print(" --- --------- --- ")$ beet lastgenre index::100000
--- VERBOSITY ---
0
--- --------- ---
$ beet -v lastgenre index::100000
user configuration: /home/sarunas/.config/beets/config.yaml
data directory: /home/sarunas/.config/beets
plugin paths: []
Loading plugins: autobpm, bandcamp, bareasc, bench, deezer, discogs, edit, embedart, fetchart, importreplace, info, inline, lastgenre, lastimport, lyrics, mbcollection, mbsync, missing, mpdstats, mpdupdate, musicbrainz, random, replaygain, scrub, spotify, types, unimported
fetchart: google: Disabling art source due to missing key
inline: adding item field has_lyrics
inline: adding item field label_or_albumartist
inline: adding item field singleton_track_artist
inline: adding item field track_artist
inline: adding item field album_name
inline: adding item field track_identification
inline: adding item field withdrawn
inline: adding album field label_or_albumartist
inline: adding album field multiple_artists
--- VERBOSITY ---
1
--- --------- ---
lastgenre: Loading whitelist /home/sarunas/repo/beets/beetsplug/lastgenre/genres.txt
lastgenre: Loading canonicalization tree /home/sarunas/repo/beets/beetsplug/lastgenre/genres-tree.yaml
Sending event: pluginload
library database: /home/sarunas/.music/beets/library.db
library directory: /run/media/sarunas/music/Music
Sending event: library_opened
Parsed query: AndQuery([RegexpQuery('index', re.compile('100000'), fast=False)])
Parsed sort: NullSort()
Sending event: cli_exit
$ beet -vv lastgenre index::100000
user configuration: /home/sarunas/.config/beets/config.yaml
data directory: /home/sarunas/.config/beets
plugin paths: []
Loading plugins: autobpm, bandcamp, bareasc, bench, deezer, discogs, edit, embedart, fetchart, importreplace, info, inline, lastgenre, lastimport, lyrics, mbcollection, mbsync, missing, mpdstats, mpdupdate, musicbrainz, random, replaygain, scrub, spotify, types, unimported
fetchart: google: Disabling art source due to missing key
inline: adding item field has_lyrics
inline: adding item field label_or_albumartist
inline: adding item field singleton_track_artist
inline: adding item field track_artist
inline: adding item field album_name
inline: adding item field track_identification
inline: adding item field withdrawn
inline: adding album field label_or_albumartist
inline: adding album field multiple_artists
--- VERBOSITY ---
2
--- --------- ---
lastgenre: Loading whitelist /home/sarunas/repo/beets/beetsplug/lastgenre/genres.txt
lastgenre: Loading canonicalization tree /home/sarunas/repo/beets/beetsplug/lastgenre/genres-tree.yaml
Sending event: pluginload
library database: /home/sarunas/.music/beets/library.db
library directory: /run/media/sarunas/music/Music
Sending event: library_opened
Parsed query: AndQuery([RegexpQuery('index', re.compile('100000'), fast=False)])
Parsed sort: NullSort()
^[[A^[[DSending event: cli_exit
$ beet -vvv lastgenre index::100000
user configuration: /home/sarunas/.config/beets/config.yaml
data directory: /home/sarunas/.config/beets
plugin paths: []
Loading plugins: autobpm, bandcamp, bareasc, bench, deezer, discogs, edit, embedart, fetchart, importreplace, info, inline, lastgenre, lastimport, lyrics, mbcollection, mbsync, missing, mpdstats, mpdupdate, musicbrainz, random, replaygain, scrub, spotify, types, unimported
fetchart: google: Disabling art source due to missing key
inline: adding item field has_lyrics
inline: adding item field label_or_albumartist
inline: adding item field singleton_track_artist
inline: adding item field track_artist
inline: adding item field album_name
inline: adding item field track_identification
inline: adding item field withdrawn
inline: adding album field label_or_albumartist
inline: adding album field multiple_artists
--- VERBOSITY ---
3
--- --------- ---
lastgenre: Loading whitelist /home/sarunas/repo/beets/beetsplug/lastgenre/genres.txt
lastgenre: Loading canonicalization tree /home/sarunas/repo/beets/beetsplug/lastgenre/genres-tree.yaml
Sending event: pluginload
library database: /home/sarunas/.music/beets/library.db
library directory: /run/media/sarunas/music/Music
Sending event: library_opened
Parsed query: AndQuery([RegexpQuery('index', re.compile('100000'), fast=False)])
Parsed sort: NullSort()
Sending event: cli_exit |
|
@JOJ0 I understand your frustration, but I hope you can appreciate that this PR has grown quite lengthy with extensive discussion across multiple threads. When I asked my questions, I was looking for a concise summary of the core technical justification rather than having to parse through all the detailed examples and back-and-forth. I found your response dismissive when you suggested I hadn't taken the time to read your explanations. As a maintainer, I need to efficiently evaluate PRs, and asking for focused clarification is part of that process - not a sign of laziness or inattention. I've now provided a technical alternative using config["verbose"].as_number() >= 3 to check for -vvv flag, which would address your volume concerns while using existing beets infrastructure. Let's focus on evaluating the technical merits of both approaches constructively. I'm genuinely trying to help find the best solution here. |
The justification is not technical but organizational and I posted it initially in the PR's description (which I just posted and highlighted the words that are important again): and @semohr pointed you to relevant parts of the thread recently:
Ok sorry but I felt ignored here 🤷 since what I wrote above ^^^ . Re-explaining again felt like useless work since I did that repeatedly already. Sorry, maybe a misunderstanding and nevermind. Let's move on with your suggestion....
I like your suggestion. Let's try that... |
snejus
left a comment
snejus
left a comment
There was a problem hiding this comment.
Looks great - it seems there's a conflict (probably to do with the changes I merged earlier). You're good to go once that's resolved
JOJ0
changed the title
-vvv)
JOJ0
force-pushed
the
lastgenre_ext_debug
branch
from
05625f6 to
bd04f0b
Compare
Replace extended_debug config and CLI option with -vvv and add a helper function.
JOJ0
force-pushed
the
lastgenre_ext_debug
branch
from
bd04f0b to
4b1e505
Compare
Description
Tuning log messages come in handy for instance when setting up personal
whitelistandcanonicalfiles. Futurelastgenrefeatures will bring more of these messages, which might clutter the plugin's code as well as theDEBUGlog. This PR adds a wrapper that only logs those messages when the user runsbeet -vvv lastgenre ...(orbeet -vvv import ...).Tests.