refactor(logging): follow Python library best practices (closes #834)#1072
Open
christianescamilla15-cell wants to merge 1 commit intobilby-dev:mainfrom
Open
Conversation
…-dev#834) The bilby logger previously attached a StreamHandler at import time via an automatic call to setup_logger() in bilby/core/utils/__init__.py. This goes against Python's recommended practice for libraries: https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library "It is strongly advised that you do not add any handlers other than NullHandler to your library's loggers." Adding handlers at import time can interfere with downstream applications that want to configure their own logging (e.g., unit test output, structured logs, custom formatters). Downstream pipeline developers have reported having to hijack the bilby logger to work around this. Changes: 1. bilby/core/utils/log.py - Install a NullHandler on the bilby logger at module load time - setup_logger() remains fully functional but is no longer called automatically; users must call it explicitly if they want console output - New helper enable_default_logging() for one-line opt-in - Updated docstrings to explain the new behaviour - Fixed a subtle bug where FileHandler also matched the StreamHandler check (FileHandler is a subclass of StreamHandler); now the check excludes FileHandler explicitly - NullHandlers are no longer level-adjusted in setup_logger() 2. bilby/core/utils/__init__.py - Removed the unconditional setup_logger() call at import time - Still respects --log-level command line argument (if provided explicitly, we call setup_logger to match pre-2.8 behaviour) Migration notes for users: - Most users running bilby scripts will see no change because: * Running with --log-level=DEBUG etc. still triggers setup_logger * Scripts can add `bilby.core.utils.enable_default_logging()` at top - Downstream packages (bilby_pipe, user pipelines) may want to call setup_logger() once at startup - Test suites that previously had to suppress bilby output no longer need to do so
This file contains hidden or 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
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.
Summary
Implements the logger refactor discussed in #834. The bilby logger now follows the Python logging recommendation for libraries — it attaches only a `NullHandler` at import time instead of a pre-configured `StreamHandler`.
Closes #834.
Why
The Python docs are explicit:
As @mick-wright noted in the issue, downstream pipeline developers currently have to hijack the bilby logger to get correct behaviour. This PR fixes that.
Changes
`bilby/core/utils/log.py`
`bilby/core/utils/init.py`
Migration
Per @ColmTalbot's suggestion in the issue, this tries to make the old behaviour easy to recover:
For examples and tutorials, we can add `bilby.core.utils.enable_default_logging()` as the first line in a follow-up PR, once this is merged.
Test plan
Follow-ups
Once this is merged, we'll want:
References