openml / automlbenchmark

OpenML AutoML Benchmarking Framework
https://openml.github.io/automlbenchmark
MIT License
401 stars 132 forks source link

Logger defaults to printing to stderr instead of stdout #409

Open eddiebergman opened 3 years ago

eddiebergman commented 3 years ago

While using automlbenchmark on a slurm cluster, specifying a specific out and error stream, I noticed automlbenchmark outputs all logging to stderr.

Reproduce

python runbenchmark -s only flaml > out.txt 2> err.txt
cat out.txt # Empty
cat error.txt # All the normal output

I'm not sure how logging should be configured overall in terms of what gets printed to stdout and stderr but I've left a simple change here in case you'd like to change it in the short term.

Short fix

# amlb/logger.py:55, def setup(...): ...
console = logging.StreamHandler(stream=sys.stdout)

Here's the constructor just to show that id defaults to sys.stderr

class StreamHandler:
    def __init__(self, stream=None):
            """
            Initialize the handler.

            If stream is not specified, sys.stderr is used.
            """
            Handler.__init__(self)
            if stream is None:
                stream = sys.stderr
            self.stream = stream
PGijsbers commented 3 years ago

I'm not sure why stderr is configured as default output stream. @sebhrusen?

sebhrusen commented 3 years ago

It's Python core library's decision to log to stderr by default. It's common for logs to be redirected to stderr as stdout is usually reserved for a program's output/result: e.g. calls to print() are usually considered as proper output, and separated from logs.

sebhrusen commented 3 years ago

@eddiebergman instead of fixing the logger, it rather looks like you expect the result scores to be printed to stdout, which is a fair expectation, we can add a print() statement for this.

eddiebergman commented 3 years ago

Hi @sebhrusen,

While yes they default to stderr, i generally expect a program to only output to stderr when an error of some kind has occurred, where the program has exited with a failed state. For example, if the framework failed to setup, for the error message to be put there with the general setup output going to stdout. I find it an odd choice that python's logging StreamHandler defaults to stderr.

However I would think this is not so simple given the various other possible logging event's like 'debug' and 'warning', I don't really know where I would expect them to go, perhaps stdout as they are not explicitly errors.

Please feel free to continue with this how you wish, I just wanted to bring this to your attention :)

sebhrusen commented 3 years ago

@eddiebergman, yes, there are basically 2 common practices for command line apps. A common one—who lead to Python's decision—is to redirect logs to stderr and reserve stdout for result or any "useful output"; the rationale being that this way,stdout can easily be pipped to other commands, usually only interested in the result. Actually,stderr is supposed to handle all diagnostics, not only errors, and, imo, it makes sense to consider all logs as diagnostics. Another is to consider that stderr should get content only in case of errors. I think it's perfectly fine for short-running programs on the CLI, but long running ones like this one need logs, and splitting console logs, depending on their levels, between stdout and stderr would be even more confusing. Personally, I think that the exit code should be source of truth to decide if a program ended normally or not, not if stderr has any content. And in case of a relatively complex app like this one, where a single run can trigger multiple jobs, some of them passing, some others failing, you can have a program ending normally but with a couple of internal jobs failing, which is very different from a complete failure (in this case, it's not the app itself that failed, it's one/some of the framework's task).

I'll have a look at what we can do and will try to print all—and only—the benchmark results to stdout. I think this will make a consistent usage of both streams, according to the first common practice mentioned above.

Thanks for raising this, I didn't pay much attention to it before, will definitely try to improve it.

eddiebergman commented 3 years ago

Hi @sebhrusen,

Thanks for the detailed response and your rationale, this all makes sense and I agree with your decisions :) I also agree that only the end results should be piped to stdout in this case. I don't see many cases where this would be used but it's at least in line with other programs in that case.

Feel free to close this issue if you would like!