Remove sklearn logger default StreamHandler

[cdeil]

This PR removes the StreamHander which was attached to the sklearn logger on import.

Relevant: #15122 and #15122 (comment) which suggests that this was added for Python 2.
Please note that since Python 3.2, a logging.lastResort default handler was added to the logging standard library, which means that by default your messages will show up for users, even if you don't add this streamhandler.

Note that the standard advice for Python libraries is to not attach handlers:
https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library

Why is it bad to have the handler?

Because it causes log messages to appear twice, if people add a root logger, which is added by calling e.g. logging.info, like this:

>>> import sklearn
>>> import logging
>>> logging.info("123")
>>> sklearn.logger.info("123")
123
INFO:sklearn:123

Note that the problem of duplicate log messages is an old one, here's a blog post on the issue, and nice tool to figure how how logging works, and which libraries do what:
https://rhodesmill.org/brandon/2012/logging_tree/

Thoughts?

[jnothman]

It seems I had a review comment lying around for a long time... Maybe we can get this into the upcoming release.

[jnothman]

Surely we shouldn't be doing this either?

[adrinjalali]

agreed.

[jnothman]

I'm confused what effect this has on the user now. Does setting the level on a logger that has no handler do anything useful?

[amueller]

seems like consensus :)

[adrinjalali]

Should we then remove this one @thomasjpfan ?

[thomasjpfan]

Ah yes, we should remove the setLevel as well, updating PR.

[thomasjpfan]

Will removing the handler silence the logging output of the dataset module?

[jnothman]

It should not on python >= 3.2, due to the last resort logger, but we can check.

[thomasjpfan]

Since the logging.lastResort logging is set to output to sys.stderr with level WARNING, the logger used in datasets module will not output anything unless they are at the WARNING level.

Currently, they are all at debug or info.

[jnothman]

Hmm so maybe we need docs to supplement this change.

[thomasjpfan]

The "quick" way to work through this is to add a verbose option to the fetch_* datasets and change the logging to use print.

It would break backwards compatibility with users that are actively using the logging API and capturing the logs from the fetch_* calls. (I think this is kind of rare.)

[adrinjalali]

we really should move the other way around and do more logger rather than going the print way I'd say.

[cdeil]

Maybe leaving the INFO level default as in this PR is the best thing to do, to keep the info messages that you want users to see by default. I think removing the handler should be OK, i.e. messages should appear by default on the console without it.

[thomasjpfan]

Honestly, I am okay with removing the handler and not having anything print out. We do not have these messages print out when downloading using fetch_openml and I do not think we have any complains.

[jnothman]

Yes, let's hide them by default and at most make a what's new note that users should set the logging level if they want these messages to continue

[thomasjpfan]

Updated PR with whats new entry stating that a handler should be added to continue to receive messages from the datasets module.

[cdeil]

I think the statement as-is in the changelog isn't 100% correct. In many cases the last resort handler is in place and will log messages by default to the console. If the user wants to change logging, they can add handlers to the sklear.logger, but e.g. also the root logger.

Maybe not try to explain how logging handlers work via the what's new entry, and to just explain why the handler was removed?

Suggested change

	- |API| The `StreamHandler` was removed from `sklearn.logger`. To continue to
	receive logging messages from :mod:`sklearn.datasets`, a handler should
	be added to `sklearn.logger`. :pr:`16451` by :user:`Christoph Deil <cdeil>`.
	- |API| The `StreamHandler` was removed from `sklearn.logger` to avoid
	double logging of messages in common cases where a hander is attached
	to the root logger, and to follow the Python logging documentation
	recommendation for libraries to leave the log message handling to
	users and application code. :pr:`16451` by :user:`Christoph Deil <cdeil>`.

[thomasjpfan]

I think the statement as-is in the changelog isn't 100% correct. In many cases the last resort handler is in place and will log messages by default to the console.

The last resort handler will log warnings to stderr. The datasets module logs with info and debug levels, and nothing is shown on the console.

Although, I am okay with updating the whats new to your suggestion.

[thomasjpfan]

For example:

from sklearn.datasets import fetch_kddcup99
fetch_kddcup99(data_home="data")

will print Downloading https://ndownloader.figshare.com/files/5976042 on master, and nothing with this PR.

[cdeil]

I see. So here the sklearn.datasets._kddcup99 logger is created, it has the default level of warning and thus the info message isn't shown.

You could add a logger.setLevel(logging.INFO) there if you want it to appear by default for users.

My understanding is that messing with handlers in libraries is bad (causes confusion and double log messages for users), but messing with log levels is OK in libraries, to get the default behaviour you want.

Another option could be to use the sklearn.logger there to emit the message, but then I think you have to be very careful to avoid circular imports, probably is tricky.

[thomasjpfan]

The lastresort handler will still filter everything out.

The only want to get the logging messages to appear is to change the logger to logger.warning.

Anyways, I am okay with removing the messages so we can be a better citizen of using logging.

[amueller]

Hm that seems fine as info and debug seem to be the correct logging levels. We could set the level to 'warning' if we think that downloading the data is unexpect, but 'info' seems right and so it shouldn't be printed by default.