search

roocs / clisops

Climate Simulation Operations
https://clisops.readthedocs.io/en/latest/
Other
21 stars 9 forks source link

Logging adjustments #216

Closed Zeitsperre closed 2 years ago

Zeitsperre commented 2 years ago

Pull Request Checklist:

This reimplements logging in CLISOPS using Loguru, an alternative logging library that is virtually identical by way of API but much more streamlined in its configuration.

The current logging setup initializes Handlers on import, which are then propagated to upstream libraries. This causes issues upstream when outputs to stdout and stderr can falsely cause build failures on CI and ReadTheDocs in other libraries and applications (see note about creating Handlers in libraries: https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library).

The changes presented here ~disable logging~ remove the rootlogger for CLISOPS by default, allowing for logging to be explicitly set for the library using a function (clisops.utils.common.enable_logging()), or by setting a handler in the main() loop of a running process using the loguru's logger class (e.g. from loguru import logger; logger.add(sink=sys.stdout)).

Yes. Logging is no longer dependent on the standard logging library. In order to enable logging for CLISOPS, the called process must enable logging with clisops.utils.common.enable_logging() (This can be moved somewhere/renamed if needed). This will create the handlers for CLISOPS to output to stdout, stderr` (and elsewhere, if we want).

Since there are pytests relying on caplog and loguru does not write to the standard logging handler, I used a pytest plugin that passes messages from loguru to caplog. For tests that examine the logger, there is no difference beyond ensuring that logging is enabled for clisops (i.e. logger.enable("clisops"))

cehbrecht commented 2 years ago

If loguru makes are logging easier it would be nice. It has also something to handle in multiprocessing which we might want to use in pywps.

I'm a bit worried about the transition of the codes and the activation of the loguru logging.

Zeitsperre commented 2 years ago

I wasn't going to plunge into the depths of asynchronous logging, but yes it looks as though there is robust support for safely-threaded logging in loguru. I'm leaving this here as an example, and perhaps we can talk about this change in our next meeting.

Logging in general is something that I would like us to make more use of on the Ouranos side. @huard mentioned that we could try deploying this on some Ouranos projects first to get a sense of its behaviour. I'll find an afternoon to give that a try.

agstephens commented 2 years ago

@Zeitsperre, this looks great. Let me know when it is ready to merge.

Zeitsperre commented 2 years ago

Hey Ag, there are a few final changes to this that I want to make, based on the work done in https://github.com/Ouranosinc/xclim/pull/1041. The biggest breaking change is that the default behaviour for logging should be set to 'disabled'; The running process/user should be responsible for enabling logging:

import sys
from loguru import logger

logger.enable("clisops")
logger.add(sys.stdout, level="INFO||DEBUG||etc.")

This whole process should also be documented for clarity as well, so I'll add something similar based on the other PR too. It should be finished today!

Zeitsperre commented 2 years ago

@agstephens This PR is now fully functioning for me!

agstephens commented 2 years ago

Hi @Zeitsperre , could you please fix the conflicts and then we can merge this one as well.

agstephens commented 2 years ago

Thanks @Zeitsperre, I have merged this in now.