Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use developer log level, protect logger defaults in test #473

Merged
merged 4 commits into from
Feb 8, 2024
Merged

Use developer log level, protect logger defaults in test #473

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8e23a05
Use developer setting, protect logger defaults in test
al-rigazzi Feb 2, 2024
b69708e
Simplify log_level translation logic
al-rigazzi Feb 2, 2024
532d6a8
Address reviews
al-rigazzi Feb 6, 2024
a4c0737
Remove unused import
al-rigazzi Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions smartsim/log.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
_PR = ParamSpec("_PR")


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Get the logging level based on environment variable
SMARTSIM_LOG_LEVEL. If not set, default to info.

Expand All @@ -77,15 +77,14 @@
:returns: Log level for coloredlogs
:rtype: str
"""
log_level = os.environ.get("SMARTSIM_LOG_LEVEL", "info").lower()
if log_level == "quiet":
if user_log_level == "quiet":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return "warning"
if log_level == "info":
if user_log_level == "info":
return "info"
if log_level == "debug":
if user_log_level == "debug":
return "debug"
# extremely verbose logging used internally
if log_level == "developer":
if user_log_level == "developer":

Check warning on line 87 in smartsim/log.py

View check run for this annotation

Codecov / codecov/patch

smartsim/log.py#L87 

Added line #L87 was not covered by tests
return "debug"
return "info"

Expand Down Expand Up @@ -205,7 +204,7 @@
"""
# 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 = os.environ.get("SMARTSIM_LOG_LEVEL", "info")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, totally right

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was already there...

if user_log_level != "developer":
name = "SmartSim"

Expand All @@ -214,7 +213,7 @@
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

Expand Down
6 changes: 4 additions & 2 deletions tests/test_logs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Expand Down
Loading