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.

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

Resolves the duplicate logs issue #1164

Merged
merged 4 commits into from
Jul 20, 2021

Conversation

qizhuli
Copy link
Contributor

@qizhuli qizhuli commented Jul 1, 2021

Motivation

The console handler present in the root logger leads to duplicate messages being printed on the console, creating unnecessary clutter. This has also been discussed here. This PR fixes this issue, and resolves #1000.

Modification

In mmcv.utils.get_logger(), all StreamHandler present in the root logger are set to log at the ERROR level, instead of NOTSET which is the default.

@CLAassistant
Copy link

CLAassistant commented Jul 1, 2021

CLA assistant check
All committers have signed the CLA.

@zhouzaida
Copy link
Collaborator

hi @qizhuli ,please fix the CI

@ZwwWayne ZwwWayne mentioned this pull request Jul 2, 2021
20 tasks
@qizhuli
Copy link
Contributor Author

qizhuli commented Jul 2, 2021

@zhouzaida Hi there. I have revised my PR to satisfy the tests. Now it sets any StreamHandler associated with the root logger to log at the ERROR level (instead of NOTSET by default).

Over the past few days, I did a little more searching on this matter, and it seems to me that the cleanest solution would be to do logger.propagate = False right after calling logger = logging.getLogger(name), which would stop messages from being sent to the root logger completely. Coincidentally, that's exactly the approach adopted by Detectron2. Unfortunately, this approach is problematic with the caplog fixture in mmcv's tests, as killing child-to-parent propagation would cut off the LogCaptureHandler too, on which caplog depends. People have been talking about it at lengths in eisensheng/pytest-catchlog#44 and pytest-dev/pytest#3697, but there seems to be no official fix after more than 5 years...

@codecov
Copy link

codecov bot commented Jul 5, 2021

Codecov Report

Merging #1164 (b203441) into master (b035fe9) will decrease coverage by 0.03%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1164      +/-   ##
==========================================
- Coverage   68.01%   67.98%   -0.04%     
==========================================
  Files         159      159              
  Lines       10421    10429       +8     
  Branches     1895     1899       +4     
==========================================
+ Hits         7088     7090       +2     
- Misses       2966     2969       +3     
- Partials      367      370       +3     
Flag Coverage Δ
unittests 67.98% <33.33%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmcv/utils/logging.py 95.23% <33.33%> (-4.77%) ⬇️
mmcv/ops/saconv.py 83.58% <0.00%> (-5.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b035fe9...b203441. Read the comment docs.

mmcv/utils/logging.py Outdated Show resolved Hide resolved
Adds a more detailed explanation in comments
@zhouzaida zhouzaida mentioned this pull request Jul 10, 2021
12 tasks
@ZwwWayne
Copy link
Collaborator

Can be merged after verification in downstream repos like MMSeg/MMDet

@ZwwWayne ZwwWayne merged commit 261618f into open-mmlab:master Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

log repeat twice
4 participants