Make arctic training logging manage all logging #53
Conversation
sfc-gh-lmerrick
requested review from
sfc-gh-mwyatt,
sfc-gh-jrasley,
sfc-gh-caxu,
sfc-gh-zhyao,
sfc-gh-srajbhandari and
sfc-gh-sbekman
as code owners
| @@ -79,10 +148,10 @@ def setup_logger(config: "LoggerConfig") -> None: | |||
| level=config.level, | |||
| ) | |||
| logger.info("Logger enabled") | |||
| set_dependencies_logger_level(config.level) | |||
| redirect_builtin_logging_and_set_level(config.level) | |||
There was a problem hiding this comment.
NOTE: I have chosen to only make setup_logger add the log redirection, while setup_init_logger is left unchanged. I am not sure if after these changes whether the setup_init_logger function is that useful, however.
arctic_training/trainer/trainer.py
Outdated
| @@ -264,6 +265,7 @@ def train(self) -> None: | |||
| """ | |||
| Main training loop. Calls the epoch method for each epoch of training. | |||
| """ | |||
| setup_logger(self.config.logger) | |||
There was a problem hiding this comment.
This change to have the trainer configure logging when trainer.train() is called is probably the most controversial idea introduced in this PR. For users relying on the Python API, they can instead import and call the setup_logger function themselves as part of their Python scripts. However, for users relying on the CLI, it seems necessary to plumb the setup_logger function in somewhere (though the CLI launching code is also a valid place to do this).
There was a problem hiding this comment.
No need to call setup_logger here as it will be called in the pydantic config model initialization. However, we might take this opportunity to re-think when/where we want to initialize the logger.
There was a problem hiding this comment.
I'll drop this file's changes from the PR for now -- we can always tweak the timing of logging setup flow in a separate change.
Currently arctic training jobs only configure logging by propagating logging levels to specific dependencies without actually setting up log handling for anything besides code inside the arctic training package itself (or code which imports and uses the logger from arctic training via
from arctic_training.logging import logger).For users who are accustomed to the prevalent
import logging; logging.basicConfig()idiom, this limited setup can come as a surprise, since their normal idiomatic approach sets logging level, logging format, and logging handling on the root logger and thus automatically gets their logs flowing nicely into their terminals across all of their packages and 3rd-party dependencies.This PR seeks to bring
arctic_training.logging.setup_logging()more in line withlogging.basicConfig()by having the provided logging config apply to all packages, not just the special "dependency" packages affected byarctic_training.logging.set_dependencies_logger_level(). To accomplish this, I have added functionality to redirect logging from the built-in Python logging mechanisms into theloguru-based logging used by arctic training. More information is available in thelogurudocumentation.