Send logs to stderr by default #646
Conversation
…ng library. - Changing order of astromodels import in init. It was using astromodels' setup_logger, not threeml's setup_logger - This revealed a circular import. I patched it with a local import and added a FIXME not about it.
|
🤔 I think the failing tests are not related to my changes, are they? The Or at least that's what it seems to me, I'm not very familiar with the tests suite. Any ideas? I at least verified that all tests passed in my machine. |
This looks more like a temporary glitch of the internet connection as it fails to download the catalog files. I roll them back to see if this solves the issue. They don't look related to this PR. |
|
Thanks @ndilalla! Most are passing now, except for "test_dynesty_nested" on (macos13, python 3.9). It's passing in both (macos13, python 3.11) and (macos-latest, python 3.9). It also doesn't seem related to the changes here. Is is possible that the results of that test are random and that the test values tolerance is too small to account for the spread? |
It shouldn't be random, but you never know! By looking at the fit results in the log, it looks like the fit didn't converge properly: |
|
Thanks @ndilalla, that fixed it indeed. |
|
I forgot this was still open. @israelmcmc shall we go ahead and merge this PR? |
|
Thanks, @ndilalla. Yeah, I think so. At least I'm not planning to add anything else to this PR. I can work on #647 on a separate PR, if that OK with you and @omodei. I'd recommend merging this one together with threeML/astromodels#217 |
|
I am currently dealing with the circular import issue in the __initi__.py that would be fixed by this PR - is the plan still to merge this one soon, @ndilalla? |
Python's builtin logging library prints by default to stdout, not stderr. This is the convention since it allows you to easily separate the applications output from the logs. This is, for example, useful when working on larger pipelines, where the logs and output are usually redirected to different places.
This is related to this other issue in Rich, which threeML uses:
Textualize/rich#2022
While the maintainer hasn't changed the default to stderr, the thread has multiple arguments from various developers of why this should be the default. ThreeML uses a custom
Consolewhen callingRichHandler, so the Rich default doesn't matter, but I thought it was still related.In principle, just adding the flag
stderr=Trueduring theConsoleinstantiation should had been enough, but the log were still being outputted to stdout.The problem was that the main
__init__is importing everything fromastromodels, including the astromodel'ssetup_logger, which was shadowing theeML'ssetup_logger.Fixing the order of the imports revealed a circular import, which was preventing importing threeML at all. The
io/loggingmoduled needed theconfigmodule to configure the logger, and theconfigmodule neededio/loggingto create its own logs. I patched this by moving thesetup_loggerimport inconfigto local import within the functions where it was needed.While this patch worked for now to achieve the initial goal of using logging to stderr by default, a circular import is usually a symptom telling you that there's something in the code design that needs to changed. In this case, I think the way to go to solve this circular import is to not configure the logging within 3ML at all. This is also the recommended way to use the logging library. I opened the issue #647 about this, which I can address in a separate PR.
The equivalent PR for astromodels is: threeML/astromodels#217