-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use recommended dictConfig for logger configuration #83
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danielhuppmann
approved these changes
Apr 19, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to admit that I still did not fully understand Python logging, but the changes look ok to me and given that @meksor is on annual leave for another week, I think it's ok to merge, do a patch-release, and then discuss later if that is ok for server-side deployment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Downstream in iiasa/message-ix-models#173, our most recent release coupled with a recent pyam release led to them using pyam 2.2.0 instead of 1.3.1, which also brought ixmp4 on board. Immediately, their tests for python 3.12 started failing, though I don't know for sure why the tests for other python versions didn't fail.
Either way, the failure is caused by ixmp4 (and pyam) overwriting the root logger on import, which causes several existing handlers to go missing, one of them being the pytest
caplog
handler. Thus, tests that were expected to capture certain logging messages didn't receive them anymore.After reading up on logging configuration, I found that the
FileConfig()
method we currently use has been sort-of-deprecated starting with Python 3.2 -- at least, that's when the more moderndictConfig()
method was introduced and that's when migration to it has been recommended "in your own time". One major advantage of thedictConfig()
method is that we don't need to specify anything for theroot
logger, whereas this was required as a key in theFileConfig
config file. So instead of digging around for a way to set theroot
logger's handlers to the existing ones, I migrated our config todictConfig()
:)The dictionary in question can be generated in any which way you like, but I wanted to keep the structure, i.e. keep them in separate files. First, I considered yaml, but this would add a dependency. Next, I considered toml, but our toml dependency has last been released in 2020, and I'm actually surprised it still works. Python gained native toml support via the
tomllib
library with 3.11, so if we want to keep supporting 3.10 (which I think we should), we can't just use that instead. We would still need to specify that for 3.10, thetomli
dependency is required, which is also different from what we have now but a direct predecessor of the native library. But all this sounds like the topic for a new issue, so I finally opted for json. We can't store comments in json, but apart from that, I'm mostly happy with it.Locally, the tests are running fine. However, I don't know if we have any tests for confirm that the logging configuration works in the other modes as well. Currently, we have config files for
debug, development, production, server
, but it looks like only the first three are actually allowed and used. Maybe @meksor can confirm: is theserver.conf
used anywhere? If so, we might want to confirm that thefilename
properties of its handlers are evaluated correctly.Lastly, I'd like to merge these changes soon and publish another patch release to fix the issues downstream. I'll also add a similar PR to pyam.