From 8e23a05d82cb04846d512b22d94ec4a40664fdf2 Mon Sep 17 00:00:00 2001 From: Ale Rigazzi Date: Fri, 2 Feb 2024 10:36:57 -0600 Subject: [PATCH 1/4] Use developer setting, protect logger defaults in test --- smartsim/log.py | 27 ++++++++++++++------------- tests/test_logs.py | 6 ++++-- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/smartsim/log.py b/smartsim/log.py index 44e26339e..1bf4b22f2 100644 --- a/smartsim/log.py +++ b/smartsim/log.py @@ -63,7 +63,7 @@ _PR = ParamSpec("_PR") -def _get_log_level() -> str: +def _get_log_level() -> t.Tuple[str, str]: """Get the logging level based on environment variable SMARTSIM_LOG_LEVEL. If not set, default to info. @@ -74,20 +74,20 @@ def _get_log_level() -> str: - developer: Shows everything happening during execution extremely verbose logging. - :returns: Log level for coloredlogs - :rtype: str + :returns: Log level for coloredlogs and value of SMARTSIM_LOG_LEVEL + :rtype: tuple[str,str] """ log_level = os.environ.get("SMARTSIM_LOG_LEVEL", "info").lower() if log_level == "quiet": - return "warning" + return "warning", log_level if log_level == "info": - return "info" + return "info", log_level if log_level == "debug": - return "debug" + return "debug", log_level # extremely verbose logging used internally if log_level == "developer": - return "debug" - return "info" + return "debug", log_level + return "info", log_level def get_exp_log_paths() -> t.Tuple[t.Optional[pathlib.Path], t.Optional[pathlib.Path]]: @@ -205,16 +205,17 @@ def get_logger( """ # if name is None, then logger is the root logger # if not root logger, get the name of file without prefix. - user_log_level = _get_log_level() - if user_log_level != "developer": + user_log_level, env_log_level = _get_log_level() + + if env_log_level != "developer": name = "SmartSim" logging.setLoggerClass(ContextAwareLogger) logger = logging.getLogger(name) - if log_level: - logger.setLevel(log_level) - else: + if not log_level: log_level = user_log_level + if isinstance(log_level, str): + logger.setLevel(log_level.upper()) coloredlogs.install(level=log_level, logger=logger, fmt=fmt, stream=sys.stdout) return logger diff --git a/tests/test_logs.py b/tests/test_logs.py index 908a6550a..a1281fb90 100644 --- a/tests/test_logs.py +++ b/tests/test_logs.py @@ -109,14 +109,16 @@ def test_add_exp_loggers(test_dir): assert err_file.is_file() -def test_get_logger(test_dir: str, turn_on_tm): +def test_get_logger(test_dir: str, turn_on_tm, monkeypatch): """Ensure the correct logger type is instantiated""" + monkeypatch.setenv("SMARTSIM_LOG_LEVEL", "developer") logger = smartsim.log.get_logger("SmartSimTest", "INFO") assert isinstance(logger, smartsim.log.ContextAwareLogger) -def test_exp_logs(test_dir: str, turn_on_tm): +def test_exp_logs(test_dir: str, turn_on_tm, monkeypatch): """Ensure that experiment loggers are added when context info exists""" + monkeypatch.setenv("SMARTSIM_LOG_LEVEL", "developer") test_dir = pathlib.Path(test_dir) test_dir.mkdir(parents=True, exist_ok=True) From b69708e9dfe11a1bb143230da47d55979a4f5977 Mon Sep 17 00:00:00 2001 From: Ale Rigazzi Date: Fri, 2 Feb 2024 10:53:15 -0600 Subject: [PATCH 2/4] Simplify log_level translation logic --- smartsim/log.py | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/smartsim/log.py b/smartsim/log.py index 1bf4b22f2..ef0551bd2 100644 --- a/smartsim/log.py +++ b/smartsim/log.py @@ -63,7 +63,7 @@ _PR = ParamSpec("_PR") -def _get_log_level() -> t.Tuple[str, str]: +def _translate_log_level(user_log_level: t.Optional[str] = "info") -> str: """Get the logging level based on environment variable SMARTSIM_LOG_LEVEL. If not set, default to info. @@ -74,20 +74,19 @@ def _get_log_level() -> t.Tuple[str, str]: - developer: Shows everything happening during execution extremely verbose logging. - :returns: Log level for coloredlogs and value of SMARTSIM_LOG_LEVEL - :rtype: tuple[str,str] + :returns: Log level for coloredlogs + :rtype: str """ - log_level = os.environ.get("SMARTSIM_LOG_LEVEL", "info").lower() - if log_level == "quiet": - return "warning", log_level - if log_level == "info": - return "info", log_level - if log_level == "debug": - return "debug", log_level + if user_log_level == "quiet": + return "warning" + if user_log_level == "info": + return "info" + if user_log_level == "debug": + return "debug" # extremely verbose logging used internally - if log_level == "developer": - return "debug", log_level - return "info", log_level + if user_log_level == "developer": + return "debug" + return "info" def get_exp_log_paths() -> t.Tuple[t.Optional[pathlib.Path], t.Optional[pathlib.Path]]: @@ -205,17 +204,16 @@ def get_logger( """ # if name is None, then logger is the root logger # if not root logger, get the name of file without prefix. - user_log_level, env_log_level = _get_log_level() - - if env_log_level != "developer": + user_log_level = os.environ.get("SMARTSIM_LOG_LEVEL", "info") + if user_log_level != "developer": name = "SmartSim" logging.setLoggerClass(ContextAwareLogger) logger = logging.getLogger(name) - if not log_level: - log_level = user_log_level - if isinstance(log_level, str): - logger.setLevel(log_level.upper()) + if log_level: + logger.setLevel(log_level) + else: + log_level = _translate_log_level(user_log_level) coloredlogs.install(level=log_level, logger=logger, fmt=fmt, stream=sys.stdout) return logger From 532d6a86e214a6969e57253afa7a3a48143a0175 Mon Sep 17 00:00:00 2001 From: Ale Rigazzi Date: Tue, 6 Feb 2024 12:34:07 -0600 Subject: [PATCH 3/4] Address reviews --- smartsim/log.py | 17 +++++++++-------- tests/test_logs.py | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/smartsim/log.py b/smartsim/log.py index ef0551bd2..620d9a5e4 100644 --- a/smartsim/log.py +++ b/smartsim/log.py @@ -63,9 +63,9 @@ _PR = ParamSpec("_PR") -def _translate_log_level(user_log_level: t.Optional[str] = "info") -> str: - """Get the logging level based on environment variable - SMARTSIM_LOG_LEVEL. If not set, default to info. +def _translate_log_level(user_log_level: str = "info") -> str: + """Translate value of CONFIG.log_level to one + accepted as ``level`` option by Python's logging module. Logging levels - quiet: Just shows errors and warnings @@ -74,15 +74,16 @@ def _translate_log_level(user_log_level: t.Optional[str] = "info") -> str: - developer: Shows everything happening during execution extremely verbose logging. + :param user_log_level: log level specified by user, defaults to info + :type user_log_level: str :returns: Log level for coloredlogs :rtype: str """ + user_log_level = user_log_level.lower() + if user_log_level in ["info", "debug", "warning"]: + return user_log_level if user_log_level == "quiet": return "warning" - if user_log_level == "info": - return "info" - if user_log_level == "debug": - return "debug" # extremely verbose logging used internally if user_log_level == "developer": return "debug" @@ -204,7 +205,7 @@ def get_logger( """ # if name is None, then logger is the root logger # if not root logger, get the name of file without prefix. - user_log_level = os.environ.get("SMARTSIM_LOG_LEVEL", "info") + user_log_level = CONFIG.log_level if user_log_level != "developer": name = "SmartSim" diff --git a/tests/test_logs.py b/tests/test_logs.py index a1281fb90..bc81edeb7 100644 --- a/tests/test_logs.py +++ b/tests/test_logs.py @@ -116,6 +116,25 @@ def test_get_logger(test_dir: str, turn_on_tm, monkeypatch): assert isinstance(logger, smartsim.log.ContextAwareLogger) +@pytest.mark.parametrize( + "input_level,exp_level", + [ + pytest.param("INFO", "info", id="lowercasing only, INFO"), + pytest.param("info", "info", id="input back, info"), + pytest.param("WARNING", "warning", id="lowercasing only, WARNING"), + pytest.param("warning", "warning", id="input back, warning"), + pytest.param("QUIET", "warning", id="lowercasing only, QUIET"), + pytest.param("quiet", "warning", id="translation back, quiet"), + pytest.param("DEVELOPER", "debug", id="lowercasing only, DEVELOPER"), + pytest.param("developer", "debug", id="translation back, developer"), + ], +) +def test_translate_log_level(input_level: str, exp_level: str, turn_on_tm): + """Ensure the correct logger type is instantiated""" + translated_level = smartsim.log._translate_log_level(input_level) + assert exp_level == translated_level + + def test_exp_logs(test_dir: str, turn_on_tm, monkeypatch): """Ensure that experiment loggers are added when context info exists""" monkeypatch.setenv("SMARTSIM_LOG_LEVEL", "developer") From a4c07376c39d7abeed507975dbda354d9356a7d1 Mon Sep 17 00:00:00 2001 From: Ale Rigazzi Date: Tue, 6 Feb 2024 13:39:06 -0600 Subject: [PATCH 4/4] Remove unused import --- smartsim/log.py | 1 - 1 file changed, 1 deletion(-) diff --git a/smartsim/log.py b/smartsim/log.py index 620d9a5e4..eb4af0611 100644 --- a/smartsim/log.py +++ b/smartsim/log.py @@ -26,7 +26,6 @@ import functools import logging -import os import pathlib import sys import threading