refactor plugin base

[cademirch]

If I were to make one significant change to the logger plugin system, I think it might be better to replace LogHandlerBase with a class that is not itself a log hander but creates one from a factory method. The constructor and properties would stay more or less the same.

• Currently you cannot inherit from builtin handler classes like StreamHandler or FileHandler due to how LogHandlerBase's constructor works. You could get around this, probably by requiring __post_init__() to call the appropriate handler's __init__() method instead, but it's all a bit messy with the multiple/diamond inheritance.
• Many usages probably don't even need a custom Handler subclass, and could just use a builtin one with a custom formatter.
• It would be easier to change behavior according to config. The issue I'm running into is logging to stderr vs a file.

This could be made backwards compatible with the old method by having an implementation of the new class that just wraps the old LogHandlerBase instance.

Originally posted by @jlumpe in #37

TODO:

• solidfy new base class design
• decide how to handle backwards compatibility/deprecation of current base class
• think about log file paths
• determine changes needed upstream (pair this with feat: add event dataclasses #32 probably)

[jlumpe]

Think I could probably contribute a draft PR for this soon-ish.

I also really like the work you've done in #32. I've actually done something similar to that in my JSON logger plugin here. It would be great to have that in the plugin repo to formally define the API.

I've noticed that the actual events logged by Snakemake don't always match the schema defined in this repo's README. Part of that is probably because it's out of date, but I think a good change to the core Snakemake repo would be to actually create dataclass instances with the event data and pass those to the logging functions. Compared to passing a raw dictionary as the extra argument, this would help ensure the right arguments names are used. It would also support type checking. It would also be pretty easy to automatically set the corresponding attributes on the LogRecord instance for backward compatibility, either by customizing LogRecord creation directly or by just adding a method to the dataclasses that generate the correct extra dictionary. This is another PR I'd be happy to make.

[cademirch]

Think I could probably contribute a draft PR for this soon-ish.

Great! Please request me to review it whenever.

I also really like the work you've done in #32. I've actually done something similar to that in my JSON logger plugin here. It would be great to have that in the plugin repo to formally define the API.

Thanks, agreed.

I've noticed that the actual events logged by Snakemake don't always match the schema defined in this repo's README. Part of that is probably because it's out of date,

Yes this is something I need to address. The LogEvent idea came late in the refactoring and was rushed, so I would like to improve them, both in terms of matching their respective name and adding more that could be used (DAG builds/rebuilds for checkpoints, job retries, etc). If you have any suggestions, please lmk.

but I think a good change to the core Snakemake repo would be to actually create dataclass instances with the event data and pass those to the logging functions. Compared to passing a raw dictionary as the extra argument, this would help ensure the right arguments names are used. It would also support type checking. It would also be pretty easy to automatically set the corresponding attributes on the LogRecord instance for backward compatibility, either by customizing LogRecord creation directly or by just adding a method to the dataclasses that generate the correct extra dictionary. This is another PR I'd be happy to make.

Indeed, this is my plan! Happy to accept contributions and ideas towards this. I'd also like to move the current Snakemake default log formatting to these dataclasses (example in the PR), as the the current implementation holds way too much logic in the DefaultFormatter, and partially relies on passing None in the message of a LogRecord which is problematic.

All of this would help cleanup the logger setup and configuration in Snakemake, which I have not been too happy with, as its fairly fragile and difficult to reason about.