Improved logging#252
Conversation
rozyczko
added
[scope] enhancement
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #252 +/- ##
===========================================
+ Coverage 82.55% 82.92% +0.36%
===========================================
Files 62 62
Lines 4946 5082 +136
===========================================
+ Hits 4083 4214 +131
- Misses 863 868 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
damskii9992
left a comment
damskii9992
left a comment
There was a problem hiding this comment.
I think we should have an ADR on the logger to explain why we implemented it, and instructions on how to use it, both when writing to it, and when ignoring it (I guess the last one can simply refer to the .md file you wrote).
| @pytest.fixture(autouse=True) | ||
| def _quiet_easyscience(): | ||
| with global_object.log.at_level(logging.ERROR): | ||
| yield |
There was a problem hiding this comment.
How does this work? You're only muting the log in the context, so how would this actually mute the log in a test? Wouldn't
global_object.log.set_level('ERROR')Be better?
There was a problem hiding this comment.
autouse fixture yields inside with at_level(logging.ERROR), so the test body runs within the context and is muted. Your set_level('ERROR') also works and is marginally simpler, but the code is correct
|
|
||
| ```python | ||
| from easyscience import global_object | ||
| import logging |
There was a problem hiding this comment.
You don't need to import logging, right?
| - **No default stream handlers** — EasyScience does not attach handlers | ||
| that write to `stdout` or `stderr`. It only creates log records. | ||
| Applications and test frameworks decide where those records go. |
There was a problem hiding this comment.
Isn't this bad? What about users that use easyscience as a standalone fitting package? Do they not see any messages without configuring it? They should . . .
There was a problem hiding this comment.
Python's logging.lastResort handler emits WARNINGs and above to stderr when no handlers are
configured, so standalone easyscience users do see warnings/errors out of the box.
Only INFO/DEBUG are hidden without change of setup.
| import contextlib | ||
| import logging | ||
| import os | ||
| from typing import Optional |
There was a problem hiding this comment.
Ehh, no Optional typing, use |
| upper = stripped.upper() | ||
| if upper in _LEVEL_NAME_MAP: | ||
| return _LEVEL_NAME_MAP[upper] | ||
| return default |
There was a problem hiding this comment.
You should probably raise in the end. If you try to set a logging level, but make a spelling mistake, we shouldn't silently return the default.
Also, why the hell is the default a separate parameter? Why not just the default of raw? Which probably also should have a better parameter name (level?)
There was a problem hiding this comment.
Actually, this method probably shouldn't have any defaults . . .
There was a problem hiding this comment.
silently falling back to a default on a bad value is the right behaviour. You don't want import to crash on a typo'd env var. This is for env var parsing.
FOr public API (set_level/at_level) tthis should raise. I think we need to split the concerns here
Strict parse for the API
lenient parse (default) for env var
| print(f'Unable to auto generate bindings.\n{e}') | ||
| logging.getLogger('easyscience.fitting.calculators').warning( | ||
| 'Unable to auto generate bindings.\n%s', e | ||
| ) |
There was a problem hiding this comment.
I assume you will ctrl+f to replace this everywhere, but just adding another comment here so you don't forget.
| f'Fit did not converge within the maximum optimizer steps of {max_evaluations} ' | ||
| f'({n_evaluations} objective evaluations). ' | ||
| 'Consider increasing the maximum number of evaluations or adjusting the tolerance.', | ||
| UserWarning, |
There was a problem hiding this comment.
Is it a problem that we lose the categories?
| # TODO make this a proper message (use logging?) | ||
| warnings.warn( | ||
| 'LMFit minimization is not available. Probably lmfit has not been installed.', | ||
| ImportWarning, |
| logging.getLogger('easyscience.deprecated').warning( | ||
| 'Call to deprecated function %s.', | ||
| func.__name__, | ||
| stacklevel=3, |
There was a problem hiding this comment.
stacklevel? Is that a feature in the logging module? You haven't used this elsewhere.
There was a problem hiding this comment.
stacklevel is a supported kwarg on logging methods and here it correctly points the warning at the caller of the deprecated function.
Not having it elsewhere is a fair point. I'll add it to other deprecation log locations for consistency
| 'COM812', # https://docs.astral.sh/ruff/rules/missing-trailing-comma/ | ||
| # The following is replaced by 'D'/[tool.ruff.lint.pydocstyle] and [tool.pydoclint] | ||
| 'DOC', # https://docs.astral.sh/ruff/rules/#pydoclint-doc | ||
| # The following is replaced by 'D'/[tool.ruff.lint.pydocstyle] and [tool.pydoclint] 'DOC', # https://docs.astral.sh/ruff/rules/#pydoclint-doc |
There was a problem hiding this comment.
You also accidentally reverted this to comment out the DOC rule :)
There was a problem hiding this comment.
Cause pixi run check is messing it up!!!
I need to change it once and for all
|
2 participants
Switched from direct
warnings.warnandprintstatements to Python's standardloggingmodule throughout the codebase.Replaced all uses of
warnings.warnandprintfor runtime and deprecation warnings with appropriate calls to theloggingmodule, using named loggers under theeasysciencehierarchy for better control and filtering.Added a new user guide page,
controlling-log-output.md, detailing how to suppress, filter, or redirect EasyScience log messages.No global reconfiguration, no default stream handlers, and all loggers inherit configuration, giving users and downstream libraries full control over log output.