From 84083689b565a87f31eca61b3e35f3032a737ec3 Mon Sep 17 00:00:00 2001
From: Al Rigazzi <al.rigazzi@hpe.com>
Date: Thu, 8 Feb 2024 09:35:18 +0100
Subject: [PATCH] Use developer log level, protect logger defaults in test
 (#473)

This PR fixes a bug which prevented the expected behavior when the `SMARTSIM_LOG_LEVEL` environment variable was set to `developer`.

[ committed by @al-rigazzi ]
[ reviewed by @MattToast @ankona ]
---
 smartsim/log.py    | 25 ++++++++++++-------------
 tests/test_logs.py | 25 +++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/smartsim/log.py b/smartsim/log.py
index 44e26339e..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
@@ -63,9 +62,9 @@
     _PR = ParamSpec("_PR")
 
 
-def _get_log_level() -> 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,18 +73,18 @@ def _get_log_level() -> 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
     """
-    log_level = os.environ.get("SMARTSIM_LOG_LEVEL", "info").lower()
-    if log_level == "quiet":
+    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 log_level == "info":
-        return "info"
-    if log_level == "debug":
-        return "debug"
     # extremely verbose logging used internally
-    if log_level == "developer":
+    if user_log_level == "developer":
         return "debug"
     return "info"
 
@@ -205,7 +204,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 = _get_log_level()
+    user_log_level = CONFIG.log_level
     if user_log_level != "developer":
         name = "SmartSim"
 
@@ -214,7 +213,7 @@ def get_logger(
     if log_level:
         logger.setLevel(log_level)
     else:
-        log_level = user_log_level
+        log_level = _translate_log_level(user_log_level)
     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..bc81edeb7 100644
--- a/tests/test_logs.py
+++ b/tests/test_logs.py
@@ -109,14 +109,35 @@ 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):
+@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")
     test_dir = pathlib.Path(test_dir)
     test_dir.mkdir(parents=True, exist_ok=True)