-
Notifications
You must be signed in to change notification settings - Fork 111
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.
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
No custom logger in library mode #7521
base: master
Are you sure you want to change the base?
No custom logger in library mode #7521
Conversation
ATM, if there is a root level logger set by Python application, which is a logical thing to do for an application, then datalad log messages end up duplicated -- once they are provided by the root level log handler and then by our very own one. E.g. this script import logging ## The simplest and documented way to enable logging for a Python app logging.basicConfig(level=logging.INFO, format='%(name)s : %(asctime)s - %(levelname)s - %(message)s') # Alternative way to enable some root level logging # which surprised me #logging.info('This is an info message') import datalad.support.gitrepo as r r.lgr.info("datalad logger talking") datalad_lgr = logging.getLogger("datalad") print(f"logging has handlers: {logging.getLogger().handlers}") print(f"DataLad has handlers: {datalad_lgr.handlers}" would produce following output: ❯ python simplest_app2.py [INFO ] datalad logger talking datalad.gitrepo : 2023-10-24 16:27:02,703 - INFO - datalad logger talking logging has handlers: [<StreamHandler <stderr> (NOTSET)>] DataLad has handlers: [<ProgressHandler (NOTSET)>] so you see our log line duplicated! With the proposed change, we will explicitly remove all assigned handlers when enabling library mode, and if we add to above script the datalad.enable_librarymode() we get ❯ python simplest_app2.py datalad.gitrepo : 2023-10-24 16:29:25,478 - INFO - datalad logger talking logging has handlers: [<StreamHandler <stderr> (NOTSET)>] DataLad has handlers: [] We also have to respect config variable so we could handle properly the DATALAD_RUNTIME_LIBRARYMODE environment variable.
…ned, takes its bool Otherwise even assigning =0 would make it into a library mode
Code Climate has analyzed commit 165a3a9 and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7521 +/- ##
==========================================
- Coverage 91.59% 91.34% -0.25%
==========================================
Files 325 325
Lines 43427 43435 +8
Branches 5821 5824 +3
==========================================
- Hits 39777 39676 -101
- Misses 3635 3744 +109
Partials 15 15
☔ View full report in Codecov by Sentry. |
I am not sure if this is a sensible thing to do. I believe it would turn of any progress reporting also. I am not familiar with what a library is supposed to do for logging, so I cannot be helpful here. However, I understand the desire to turn off datalad's logging verbosity. |
Indeed worth checking would happen to them! |
Has 2 commits, but it is the first one which is most important, and its commit message describes it:
Remove custom datalad log handlers while enable library mode
ATM, if there is a root level logger set by Python application, which is a
logical thing to do for an application, then datalad log messages end up
duplicated -- once they are provided by the root level log handler and then by
our very own one. E.g. this script
would produce following output:
so you see our log line duplicated! With the proposed change, we will
explicitly remove all assigned handlers when enabling library mode, and
if we add to above script the datalad.enable_librarymode() we get
We also have to respect config variable so we could handle properly the
DATALAD_RUNTIME_LIBRARYMODE environment variable.
edit 1: also it is likely to "interfere" in setting the level unconditionally ATM, I think the
set_level
is invoked and that results in changing the level here even though root logger might already be set some other level by "main library". I am leaning toward RFing this PR as doing our custom logger overall only when setting to "cmdline" mode.TODOs:
maint
???