Repository navigation
refactor: import the shared seismometer logger from core.logger - #199
Open
Sanilas-ai wants to merge 1 commit into
Open
Sanilas-ai wants to merge 1 commit into
Sanilas-ai wants to merge 1 commit into
Conversation
Add a module-level logger to seismometer.core.logger and import it in
each module instead of calling logging.getLogger("seismometer") there.
Drop the logger from five modules that defined it but never logged.
The seismometer.telemetry loggers are unchanged.
Closes epic-open-source#37
Signed-off-by: Sanilas-ai <konstantinsanilas@gmail.com>
| from seismometer.core.decorators import DiskCachedFunction | ||
|
|
||
| logger = logging.getLogger("seismometer") | ||
| from seismometer.core.logger import logger |
Contributor
There was a problem hiding this comment.
Since this is not used here and you are making the change, can we just remove it?
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Closes #37
Description of changes
Most modules defined their own
logger = logging.getLogger("seismometer"). This adds one module-levelloggertoseismometer.core.loggerand has every module import it:import loggingis dropped where nothing else used it.api/templates.py,controls/explore.py,controls/thresholds.py,plot/mpl/timeseries.py,table/fairness.py). I removed it there rather than add an unused import (flake8 F401).seismometer.core.loggerwas already imported at package import time (throughseismometer.api), so itslogging.basicConfig()runs at the same point as before. The import adds no cycle:core/logger.pyonly imports the standard library.seismometer.loggerstill exists and is the same object as before.seismometer.telemetryloggers (core/autometrics.py,data/otel.py,data/telemetry.py) use a different name on purpose, so I left them alone. Happy to give them a shared accessor too if you'd like.Verification
Python 3.12, in a clean venv with
pip install -e ".[dev]":pytest tests: 1552 passed, 1 skipped (1550 + the 2 new tests)isort,black --line-length 119andflake8(versions from.pre-commit-config.yaml) clean on the changed filesAuthor Checklist
tests/core/test_logger.py: the shared logger islogging.getLogger("seismometer"), and modules use that same object).. versionchanged:: NEXT_VERSION_PLACEHOLDERdirective to affected docstrings. (n/a, not a breaking change)changelog/ISSUE.TYPE.rstfiles. (changelog/37.misc.rst)