Use developer log level, protect logger defaults in test

[al-rigazzi]

The current log.get_logger() function has two issues.

1. it uses _get_log_level() to access the value of SMARTSIM_LOG_LEVEL, and uses its return value to define whether the logger name should just be SmartSim, or the user-provided one. The problem is that the user-provided one is used only when _get_log_level() returns developer, but that is never the case, as developer gets translated to debug.
2. As a consequence of 1, calling get_logger(name="MY_LOGGER", log_level="INFO") will always get the logger named SmartSim and set its level to info. When this is done in a test, then the log level will be set for all subsequent tests, no matter what the name is set to.

I proposed to replace the _get_log_level() with _translate_log_level() and use the return value to set the log level when log level is not set to developer. To be honest, I think it is kind of weird that users can provide a name for the logger and that it gets thrown away 99.9% of the times, but I guess they could also use a different logger if they wanted something more flexible, and with the changes of this PR, the API is consistent with the docs.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (106d70f) 90.43% compared to head (a4c0737) 90.83%.
Report is 3 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #473      +/-   ##
===========================================
+ Coverage    90.43%   90.83%   +0.39%     
===========================================
  Files           60       60              
  Lines         3837     3818      -19     
===========================================
- Hits          3470     3468       -2     
+ Misses         367      350      -17     

Files	Coverage Δ
smartsim/log.py	96.26% <100.00%> (+2.62%)	⬆️

... and 6 files with indirect coverage changes

[MattToast]

Great catch! I have a couple of small nits for you, but otherwise LGTM!!

[MattToast]

Minor typing nit: t.Optional[str] implies that

_translate_log_level(user_log_level=None)

is a valid call. Did you mean

Suggested change

	def _translate_log_level(user_log_level: t.Optional[str] = "info") -> str:
	def _translate_log_level(user_log_level: str = "info") -> str:

?

[al-rigazzi]

You know, I don't know. I think it's better as you say, as it is an internal function we should only call one way.

[MattToast]

Consider adding

Suggested change

	user_log_level = os.environ.get("SMARTSIM_LOG_LEVEL", "info")
	user_log_level = os.environ.get("SMARTSIM_LOG_LEVEL", "info").lower()

otherwise

export SMARTSIM_LOG_LEVEL=QUIET

will default to "info" which would technically be an API break

[al-rigazzi]

Sure, totally right

[ankona]

can we shorten this to:

if user_log_level in ['quiet', 'info', 'debug','warning']:
    return user_log_level

if user_log_level == "developer":
    return "info"

[al-rigazzi]

I think you missed one case, but I got the point and modified implementation accordingly.

[ankona]

I wonder what you & @MattToast think about trying to move all os.environ['xyz'] calls into the smartsim._core.config.Config

[al-rigazzi]

+1

[al-rigazzi]

It was already there...

[ankona]

Consider adding direct unit test of new method:

@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

[al-rigazzi]

I don't think I need turn_on_tm, do I? @ankona

[MattToast]

LGTM, pending tests!!