erdogant
Distfit modifies the root logger globally
Description
Importing distfit modifies all logs in our project, this does not happen with any other packages. This happens because distfit modifies the root logger globally: https://github.com/erdogant/distfit/blob/master/distfit/distfit.py#L26
Since distfit is a library it should at least use a local logger with logger = logging.getLogger(__name__), and ideally it should not be configuring logs for its downstream applications.
Minimal reproducible example
import logging
logging.basicConfig(
format="%(asctime)s %(name)-12s %(levelname)-8s %(message)s",
datefmt="%Y-%m-%d %H:%M:%S",
level=logging.INFO,
)
logger = logging.getLogger(__name__)
logger.info("This uses the basic configuration")
from distfit import distfit
logger.info("This is modified by distfit")
Output:
2025-02-26 17:24:55 __main__ INFO This uses the configuration
[distfit] >INFO> This is modified by distfit
Expected behaviour
Importing distfit should not modify existing logger
The workaround I have for this at the moment is:
from contextlib import contextmanager
@contextmanager
def preserve_root_logger():
"""Preserve the root logger when executing code.
This is to stop distlib from modifying the root logger:
https://github.com/erdogant/distfit/issues/51
"""
# Save original logger state
root_logger = logging.getLogger()
original_handlers = root_logger.handlers[:]
original_level = root_logger.level
try:
yield # Allow code to run inside the context
finally:
# Restore original handlers
root_logger.handlers.clear()
for handler in original_handlers:
root_logger.addHandler(handler)
root_logger.setLevel(original_level)
with preserve_root_logger():
import distfit
Thank you for the feedback. When I take over your example, the logger name also seems not to change when using another library. So I created a few adjustments on your example:
logger = logging.getLogger('')
[logger.removeHandler(handler) for handler in logger.handlers[:]]
logging.basicConfig(
format="%(asctime)s [%(name)-12s] >%(levelname)-8s %(message)s",
datefmt="%d-%m-%y %H:%M:%S",
level=logging.INFO)
logger = logging.getLogger(__name__)
I first set the logger to <empty>, then I remove previous handles, and finally set it to the new __name__ again.
Would this scenario also work in your case? In my examples, this works now.
pip install git+https://github.com/erdogant/distfit
It should be version >= 1.8.2
Where can we get 1.8.2? Pip can't find it, and I don't see a tag or release 1.8.2. Thanks.
Ah I see. Typo. Should be 1.8.1 If you can install from github source you will get the latest version.
Hello,
i think it might be also affected by recent changes in pypickle dependency.
Besides having changed log format in our project, we also started getting ValueError: unsupported format character ']' (0x5d) at index 20 on any logger call attempts, which does not happen if we manually specify pypickle version as 1.1.1
Thank you for pointing this out! it was a syntax error in the logger. I released a new version. Can you check whether this solves your issue?
pip install - U pypickle
@erdogant
Thank you for a quick fix and response. It solves the issue with logging error indeed, but it still causes log format changes in our project. With 1.1.0 it is not the case, probably because we are using older distfit version (distfit==1.4.5)
I changed the logging. It should keep other logging intact now. Can you check?
pip install -U distfit
The version should be >= 1.8.2
Your library functionality is absolutely amazing, but the use of logging.basicConfig() in a library is fundamentally flawed. The check for not logger.hasHandlers() does not work as intended, because the library is typically imported before the top level script has a chance to configure loggers and handler. So that mechanism will never work. My script might look like the following:
import logging
import distfit # <- distfit calls logging.basicConfig() already here! Before I have set up any log handlers.
def main():
logging.basicConfig(level=logging.INFO) # <- This one has no effect anymore.
print(distfit.__version__)
The problem becomes even more confusing since logging.basicConfig() is called the same way in both distfit and colourmap.
It is OK that you configure the local _logger in distfit/__init__.py, but it is bad practice to call logging.basicConfig() anywhere in a library.
So I suggest that you remove the following calls to logging.basicConfig():
- https://github.com/erdogant/distfit/blob/5735025a0c73c51c56c061c5a686683e60f49350/distfit/distfit.py#L27
- https://github.com/erdogant/colourmap/blob/d8748b111e6faf8ae9c398aeca54298b424fe813/colourmap/colourmap.py#L17
I also notice that you use the same mechanism in many of your other libraries.
Just as a side note, I also notice that the imports in this library are a bit quirky. The top-level package/module is called distfit, a module inside it is called distfit and the class inside the distfit module is also called distfit. On top of that, you import the distfit class into the top level distfit module which basically shadow the entire distfit module. This practice makes it difficult/impossible to use common Python import practices. For instance, it is recommended to keep the module name as a namespace when importing something. If I want to use both the distfit class and k_distribution class from the distfit.distfit module, the Pythonic way is to import the distfit module as follows:
from distfit import distfit # <- trying to get the distfit module here
def main():
fit = distfit.distfit()
k = distfit.k_distribution()
This does not work, because the import ends up importing the distfit.distfit.distfit class, not the distfit.distfit module. So I can not get hold of the k_distribution class. One possible improvement could be to move all the code from the distfit.distfit module into the distfit.__init__.py file, such that everything is accessible from the top-level distfit module.
This has been bugging me for a while now. If you @endrebjorsvik-sensibel would be so friendly to create a pull request. We may be able to fix this for good. Then I can also use the same logic for my other libraries
I have made PR https://github.com/erdogant/distfit/pull/52 to remove the configuration of the global logger and moving the global warning filter.