Please don't configure logging in the top-level __init__.py #15122
Comments
|
See #9240 for discussion leading to the introduction of this log configuration. |
|
Yes, that change flew under my radar. It seems it was intended for python 2
compat (see
#9240 (comment))
so we should just remove it.
|
|
I think using logging overall makes sense, and https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library is an interesting read. If I understand the problem correctly we would like to use logging but that means that messages below the level of That is very different from the current scikit-learn API where estimators have a My question is if we use the standard logger and don't manually set/unset logging level how can we make |
|
We don't need the verbose parameter to work as before, as long as all
instances of the same class should share the same verbosity... we would
just define a logger name for each verbose estimator that matches the
public import path of the class.
|
|
Does than mean deprecating the Or alternatively setting Overall logging is nice application level; library level it sounds complicated. |
|
Another data point, is that joblib is using logging for inter-process logging, but progress messages from the |
|
I think we should avoid neglecting this for more releases. This bad-citizen code was inserted accidentally. |
|
Resolved by #16451 |
import sklearncreates aStreamHandlerand attaches it to thesklearnlogger:scikit-learn/sklearn/__init__.py
Line 24 in 0eebade
I'm not sure what the motivation for this is, but it's a deviation from the normal "best practices" for logging, namely that libraries should restrict themselves to issuing log messages, but let the application do all logging configuration (setting up handlers, changing logger levels, and the like). There's lots written about this elsewhere, but here's one relevant blog post: http://pieces.openpolitics.com/2012/04/python-logging-best-practices/
In practice, this caused a hard-to-diagnose bug in our IPython- and sklearn-using application (actually, in more than one such application):
sys.stdoutandsys.stderrfor its own custom streams, which rely on a lot of fairly complicated machinery (extra threads, ZMQ streams, the asyncio event loop, etc.)sklearnwas imported while that IPython kernel was running.sys.stderrstream instead of the usual one.loggingmodule'sshutdownfunction, which is registered as anatexithandler). Because the IPython machinery was no longer active, we got a hard-to-understand traceback.If the intent of the handler is to suppress the "No logger configured ..." messages from the std. lib., perhaps a
logging.NullHandlercould be used for that purpose instead? I'm happy to create a PR for this if the proposed change sounds acceptable.The text was updated successfully, but these errors were encountered: