From 1182abecc86eb4a715a70884e3e7d3382498bce3 Mon Sep 17 00:00:00 2001 From: Stef Smeets Date: Fri, 8 Jan 2021 10:09:15 +0100 Subject: [PATCH 1/7] Move `configure_logging` to its own file --- esmvalcore/_config.py | 40 ------------------------------------ esmvalcore/_logging.py | 46 ++++++++++++++++++++++++++++++++++++++++++ esmvalcore/_main.py | 18 ++++++++--------- 3 files changed, 55 insertions(+), 49 deletions(-) create mode 100644 esmvalcore/_logging.py diff --git a/esmvalcore/_config.py b/esmvalcore/_config.py index 1d9559dfdf..1a50cf5c53 100644 --- a/esmvalcore/_config.py +++ b/esmvalcore/_config.py @@ -3,7 +3,6 @@ import logging import logging.config import os -import time from pathlib import Path import yaml @@ -146,45 +145,6 @@ def load_config_developer(cfg_file=None): read_cmor_tables(CFG) -def configure_logging(cfg_file=None, output_dir=None, console_log_level=None): - """Set up logging.""" - if cfg_file is None: - cfg_file = os.path.join(os.path.dirname(__file__), - 'config-logging.yml') - - cfg_file = os.path.abspath(cfg_file) - with open(cfg_file) as file_handler: - cfg = yaml.safe_load(file_handler) - - if output_dir is None: - cfg['handlers'] = { - name: handler - for name, handler in cfg['handlers'].items() - if 'filename' not in handler - } - prev_root = cfg['root']['handlers'] - cfg['root']['handlers'] = [ - name for name in prev_root if name in cfg['handlers'] - ] - - log_files = [] - for handler in cfg['handlers'].values(): - if 'filename' in handler: - if not os.path.isabs(handler['filename']): - handler['filename'] = os.path.join(output_dir, - handler['filename']) - log_files.append(handler['filename']) - if console_log_level is not None and 'stream' in handler: - if handler['stream'] in ('ext://sys.stdout', 'ext://sys.stderr'): - handler['level'] = console_log_level.upper() - - logging.config.dictConfig(cfg) - logging.Formatter.converter = time.gmtime - logging.captureWarnings(True) - - return log_files - - def get_project_config(project): """Get developer-configuration for project.""" logger.debug("Retrieving %s configuration", project) diff --git a/esmvalcore/_logging.py b/esmvalcore/_logging.py new file mode 100644 index 0000000000..de83fbf11a --- /dev/null +++ b/esmvalcore/_logging.py @@ -0,0 +1,46 @@ +"""Configure logging.""" + +import logging +import os +import time + +import yaml + + +def configure_logging(cfg_file=None, output_dir=None, console_log_level=None): + """Set up logging.""" + if cfg_file is None: + cfg_file = os.path.join(os.path.dirname(__file__), + 'config-logging.yml') + + cfg_file = os.path.abspath(cfg_file) + with open(cfg_file) as file_handler: + cfg = yaml.safe_load(file_handler) + + if output_dir is None: + cfg['handlers'] = { + name: handler + for name, handler in cfg['handlers'].items() + if 'filename' not in handler + } + prev_root = cfg['root']['handlers'] + cfg['root']['handlers'] = [ + name for name in prev_root if name in cfg['handlers'] + ] + + log_files = [] + for handler in cfg['handlers'].values(): + if 'filename' in handler: + if not os.path.isabs(handler['filename']): + handler['filename'] = os.path.join(output_dir, + handler['filename']) + log_files.append(handler['filename']) + if console_log_level is not None and 'stream' in handler: + if handler['stream'] in ('ext://sys.stdout', 'ext://sys.stderr'): + handler['level'] = console_log_level.upper() + + logging.config.dictConfig(cfg) + logging.Formatter.converter = time.gmtime + logging.captureWarnings(True) + + return log_files diff --git a/esmvalcore/_main.py b/esmvalcore/_main.py index 8da80d5efa..b562d5afdd 100755 --- a/esmvalcore/_main.py +++ b/esmvalcore/_main.py @@ -118,7 +118,7 @@ def _copy_config_file(filename, overwrite, path): import os import shutil - from ._config import configure_logging + from ._logging import configure_logging configure_logging(console_log_level='info') if not path: path = os.path.join(os.path.expanduser('~/.esmvaltool'), filename) @@ -191,7 +191,8 @@ def list(): """ import os - from ._config import DIAGNOSTICS_PATH, configure_logging + from ._config import DIAGNOSTICS_PATH + from ._logging import configure_logging configure_logging(console_log_level='info') recipes_folder = os.path.join(DIAGNOSTICS_PATH, 'recipes') logger.info("Showing recipes installed in %s", recipes_folder) @@ -220,7 +221,8 @@ def get(recipe): import os import shutil - from ._config import DIAGNOSTICS_PATH, configure_logging + from ._config import DIAGNOSTICS_PATH + from ._logging import configure_logging configure_logging(console_log_level='info') installed_recipe = os.path.join(DIAGNOSTICS_PATH, 'recipes', recipe) if not os.path.exists(installed_recipe): @@ -244,7 +246,8 @@ def show(recipe): """ import os - from ._config import DIAGNOSTICS_PATH, configure_logging + from ._config import DIAGNOSTICS_PATH + from ._logging import configure_logging configure_logging(console_log_level='info') installed_recipe = os.path.join(DIAGNOSTICS_PATH, 'recipes', recipe) if not os.path.exists(installed_recipe): @@ -344,11 +347,8 @@ def run(recipe, import os import shutil - from ._config import ( - DIAGNOSTICS_PATH, - configure_logging, - read_config_user_file, - ) + from ._config import DIAGNOSTICS_PATH, read_config_user_file + from ._logging import configure_logging from ._recipe import TASKSEP from .cmor.check import CheckLevels From 64f931f0cde7894bb27e41966a861b82a6ac9d47 Mon Sep 17 00:00:00 2001 From: Stef Smeets Date: Fri, 8 Jan 2021 11:08:47 +0100 Subject: [PATCH 2/7] Refactor logging configuration --- esmvalcore/_logging.py | 94 +++++++++++++++++++++++++++++++----------- 1 file changed, 70 insertions(+), 24 deletions(-) diff --git a/esmvalcore/_logging.py b/esmvalcore/_logging.py index de83fbf11a..0f93d2fc71 100644 --- a/esmvalcore/_logging.py +++ b/esmvalcore/_logging.py @@ -3,41 +3,87 @@ import logging import os import time +from pathlib import Path import yaml -def configure_logging(cfg_file=None, output_dir=None, console_log_level=None): - """Set up logging.""" +def _purge_handlers_with_filename(cfg: dict) -> None: + """Removes handlers with filename set. + + This is used to remove file handlers which require an output + directory to be set. + """ + cfg['handlers'] = { + name: handler + for name, handler in cfg['handlers'].items() + if 'filename' not in handler + } + prev_root = cfg['root']['handlers'] + cfg['root']['handlers'] = [ + name for name in prev_root if name in cfg['handlers'] + ] + + +def _get_log_files(cfg: dict, output_dir: str = None) -> list: + """Initialize log files for the file handlers.""" + log_files = [] + + handlers = cfg['handlers'] + + for handler in handlers.values(): + filename = handler.get('filename', None) + + if filename: + if not os.path.isabs(filename): + handler['filename'] = os.path.join(output_dir, filename) + log_files.append(handler['filename']) + + return log_files + + +def _update_stream_level(cfg: dict, level=None): + """Update the stream level for the stream handlers.""" + handlers = cfg['handlers'] + + for handler in handlers.values(): + if level is not None and 'stream' in handler: + if handler['stream'] in ('ext://sys.stdout', 'ext://sys.stderr'): + handler['level'] = level.upper() + + +def configure_logging(cfg_file: str = None, + output_dir: str = None, + console_log_level: str = None) -> list: + """Configure logging. + + Parameters + ---------- + cfg_file : str, optional + Logging config file. If `None`, defaults to `configure-logging.yml` + output_dir : str, optional + Output directory for the log files. If `None`, log only to the console. + console_log_level : str, optional + If `None`, use the default (INFO). + + Returns + ------- + log_files : list + Filenames that will be logged to. + """ if cfg_file is None: - cfg_file = os.path.join(os.path.dirname(__file__), - 'config-logging.yml') + cfg_file = Path(__file__).parent / 'config-logging.yml' + + cfg_file = Path(cfg_file).absolute() - cfg_file = os.path.abspath(cfg_file) with open(cfg_file) as file_handler: cfg = yaml.safe_load(file_handler) if output_dir is None: - cfg['handlers'] = { - name: handler - for name, handler in cfg['handlers'].items() - if 'filename' not in handler - } - prev_root = cfg['root']['handlers'] - cfg['root']['handlers'] = [ - name for name in prev_root if name in cfg['handlers'] - ] + _purge_handlers_with_filename(cfg) - log_files = [] - for handler in cfg['handlers'].values(): - if 'filename' in handler: - if not os.path.isabs(handler['filename']): - handler['filename'] = os.path.join(output_dir, - handler['filename']) - log_files.append(handler['filename']) - if console_log_level is not None and 'stream' in handler: - if handler['stream'] in ('ext://sys.stdout', 'ext://sys.stderr'): - handler['level'] = console_log_level.upper() + log_files = _get_log_files(cfg, output_dir=output_dir) + _update_stream_level(cfg, level=console_log_level) logging.config.dictConfig(cfg) logging.Formatter.converter = time.gmtime From 0a4003292bd426faabfb78ac5ed0b0fe3a515563 Mon Sep 17 00:00:00 2001 From: Stef Smeets Date: Fri, 8 Jan 2021 11:24:03 +0100 Subject: [PATCH 3/7] Fix word --- esmvalcore/_logging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/esmvalcore/_logging.py b/esmvalcore/_logging.py index 0f93d2fc71..4562af2271 100644 --- a/esmvalcore/_logging.py +++ b/esmvalcore/_logging.py @@ -43,7 +43,7 @@ def _get_log_files(cfg: dict, output_dir: str = None) -> list: def _update_stream_level(cfg: dict, level=None): - """Update the stream level for the stream handlers.""" + """Update the log level for the stream handlers.""" handlers = cfg['handlers'] for handler in handlers.values(): From 2c1e7f7e2d6e549d77663fb5ded9aa666c90ae72 Mon Sep 17 00:00:00 2001 From: Stef Smeets Date: Fri, 8 Jan 2021 11:25:32 +0100 Subject: [PATCH 4/7] Update function name --- esmvalcore/_logging.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/esmvalcore/_logging.py b/esmvalcore/_logging.py index 4562af2271..c5a09f95bb 100644 --- a/esmvalcore/_logging.py +++ b/esmvalcore/_logging.py @@ -8,7 +8,7 @@ import yaml -def _purge_handlers_with_filename(cfg: dict) -> None: +def _purge_file_handlers(cfg: dict) -> None: """Removes handlers with filename set. This is used to remove file handlers which require an output @@ -80,7 +80,7 @@ def configure_logging(cfg_file: str = None, cfg = yaml.safe_load(file_handler) if output_dir is None: - _purge_handlers_with_filename(cfg) + _purge_file_handlers(cfg) log_files = _get_log_files(cfg, output_dir=output_dir) _update_stream_level(cfg, level=console_log_level) From 3c79b853b4ec1cf23b8987cfdee2770abeb42ea3 Mon Sep 17 00:00:00 2001 From: Stef Smeets Date: Fri, 8 Jan 2021 11:27:34 +0100 Subject: [PATCH 5/7] Move `logging.config` import --- esmvalcore/_config.py | 1 - esmvalcore/_logging.py | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/esmvalcore/_config.py b/esmvalcore/_config.py index 1a50cf5c53..587552867a 100644 --- a/esmvalcore/_config.py +++ b/esmvalcore/_config.py @@ -1,7 +1,6 @@ """ESMValTool configuration.""" import datetime import logging -import logging.config import os from pathlib import Path diff --git a/esmvalcore/_logging.py b/esmvalcore/_logging.py index c5a09f95bb..6c88e8db00 100644 --- a/esmvalcore/_logging.py +++ b/esmvalcore/_logging.py @@ -1,6 +1,7 @@ """Configure logging.""" import logging +import logging.config import os import time from pathlib import Path From 35cd33020323414ccaa1e8f90793851408d95395 Mon Sep 17 00:00:00 2001 From: Stef Smeets Date: Fri, 8 Jan 2021 11:34:07 +0100 Subject: [PATCH 6/7] Address codacy issue --- esmvalcore/_logging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/esmvalcore/_logging.py b/esmvalcore/_logging.py index 6c88e8db00..acacecee16 100644 --- a/esmvalcore/_logging.py +++ b/esmvalcore/_logging.py @@ -10,7 +10,7 @@ def _purge_file_handlers(cfg: dict) -> None: - """Removes handlers with filename set. + """Remove handlers with filename set. This is used to remove file handlers which require an output directory to be set. From 2bb33af8984a9ce7f37557837171138e271fdabd Mon Sep 17 00:00:00 2001 From: Stef Smeets Date: Tue, 12 Jan 2021 12:12:49 +0100 Subject: [PATCH 7/7] Add tests for logging module --- tests/unit/test_logging.py | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 tests/unit/test_logging.py diff --git a/tests/unit/test_logging.py b/tests/unit/test_logging.py new file mode 100644 index 0000000000..7f5ad2ccc1 --- /dev/null +++ b/tests/unit/test_logging.py @@ -0,0 +1,38 @@ +"""Tests for logging config submodule.""" + +import logging +from pathlib import Path + +import pytest + +from esmvalcore._logging import configure_logging + + +@pytest.mark.parametrize('level', (None, 'INFO', 'DEBUG')) +def test_logging_with_level(level): + """Test log level configuration.""" + ret = configure_logging(console_log_level=level) + assert isinstance(ret, list) + assert len(ret) == 0 + + root = logging.getLogger() + + assert len(root.handlers) == 1 + + +def test_logging_with_output_dir(tmp_path): + """Test that paths are configured.""" + ret = configure_logging(output_dir=tmp_path) + assert isinstance(ret, list) + for path in ret: + assert tmp_path == Path(path).parent + + root = logging.getLogger() + + assert len(root.handlers) == len(ret) + 1 + + +def test_logging_log_level_invalid(): + """Test failure condition for invalid level specification.""" + with pytest.raises(ValueError): + configure_logging(console_log_level='FAIL')