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

Fix a bug where logs are missing when two or more loggers were set #1015

Merged
merged 2 commits into from
May 20, 2021

Conversation

ritosonn
Copy link
Contributor

@ritosonn ritosonn commented May 9, 2021

Motivation

When the users want to check multi-logs like both the CLI and the other (like tensorboard, wandb, mlflow), current implementation may be missing some values because of their reset_flag option. This PR fixed these bugs.

Modification

this PR includes 2 commits.

  1. LoggerHook.before_run(runner) set a flag whether self is the last logger or not. Then, other LoggerHooks have to call this method.
  2. The reset_flags should be initialized False in order to before_run() works well.

BC-breaking (Optional)

nothing.

Use cases (Optional)

For example, using MlflowLoggerHook and TextLoggerHook as follows:

from mmcv.runner import IterBasedRunner
from mmcv.runner.hooks.logger.text import TextLoggerHook
from mmcv.runner.hooks.logger.mlflow import MlflowLoggerHook
from logging import Logger
class Model:
    def train_step(self, *args, **kwargs):
        pass
model = Model()
logger = Logger(__name__)
runner = IterBasedRunner(model, logger=logger, work_dir="./")
runner.register_hook(MlflowLoggerHook())
runner.register_hook(TextLoggerHook())
runner.call_hook("before_run")
for hook in runner.hooks:
    print(hook.__class__.__name__, hook.reset_flag)

original output: both LoggerHooks resets the values, then TextLoggerHook miss to print some logging values.

>>> MlflowLoggerHook True
>>> TextLoggerHook True

assumed output & fixed output: only TextLoggerHook reset the values.

>>> MlflowLoggerHook False
>>> TextLoggerHook True

Checklist

  • Pre-commit or other linting tools are used to fix the potential lint issues.
    • The diff is small, so I didn't install the pre-commit environment, and visually checked.
  • The modification is covered by complete unit tests. If not, please add more unit test to ensure the correctness.
    • It seems to need another unit test, but I don't know how to construct unit tests. Would you please tell me how to do.
  • If the modification has potential influence on downstream projects, this PR should be tested with downstream projects, like MMDet or MMCls.
    • I couldn't confirmed because I'm not familiar with downstream projects.
  • The documentation has been modified accordingly, like docstring or example tutorials.
    • It seems the documentation for this diff is not needed.

@CLAassistant
Copy link

CLAassistant commented May 9, 2021

CLA assistant check
All committers have signed the CLA.

@hellock hellock requested a review from nbei May 10, 2021 08:53
@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #1015 (5982749) into master (db6b054) will increase coverage by 0.02%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1015      +/-   ##
==========================================
+ Coverage   64.57%   64.59%   +0.02%     
==========================================
  Files         152      152              
  Lines        9792     9796       +4     
  Branches     1779     1779              
==========================================
+ Hits         6323     6328       +5     
- Misses       3141     3142       +1     
+ Partials      328      326       -2     
Flag Coverage Δ
unittests 64.59% <75.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
mmcv/runner/hooks/logger/tensorboard.py 34.37% <0.00%> (-1.11%) ⬇️
mmcv/runner/hooks/logger/mlflow.py 81.25% <100.00%> (+0.60%) ⬆️
mmcv/runner/hooks/logger/pavi.py 69.56% <100.00%> (+0.44%) ⬆️
mmcv/runner/hooks/logger/wandb.py 69.69% <100.00%> (+0.94%) ⬆️
mmcv/runner/hooks/logger/base.py 70.87% <0.00%> (+1.94%) ⬆️

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 db6b054...5982749. Read the comment docs.

@ZwwWayne ZwwWayne mentioned this pull request May 14, 2021
9 tasks
@nbei
Copy link
Contributor

nbei commented May 19, 2021

This modification is very helpful and makes the logger system robust in some cases. Currently, most users write the config file like:

log_config = dict(
    interval=100,
    hooks=[
        dict(type='TextLoggerHook'),
        dict(type='TensorboardLoggerHook')])

Thus, they will not meet the bug mentioned in this PR. However, once we change the order of the hooks like this:

log_config = dict(
    interval=100,
    hooks=[
        dict(type='TensorboardLoggerHook'),
        dict(type='TextLoggerHook')])

The reset_flag in the TensorboardLoggerHook will cause the missing of some important values.

@ZwwWayne ZwwWayne merged commit 1a66977 into open-mmlab:master May 20, 2021
@OpenMMLab-Assistant005
Copy link

Hi @ritosonn !First of all, we want to express our gratitude for your significant PR in the MMCV project. Your contribution is highly appreciated, and we are grateful for your efforts in helping improve this open-source project during your personal time. We believe that many developers will benefit from your PR.

We would also like to invite you to join our Special Interest Group (SIG) private channel on Discord, where you can share your experiences, ideas, and build connections with like-minded peers. To join the SIG channel, simply message moderator— OpenMMLab on Discord or briefly share your open-source contributions in the #introductions channel and we will assist you. Look forward to seeing you there! Join us :https://discord.gg/UjgXkPWNqA

If you have WeChat account,welcome to join our community on WeChat. You can add our assistant :openmmlabwx. Please add "mmsig + Github ID" as a remark when adding friends:)
Thank you again for your contribution❤

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.

5 participants