Skip to content

dont disable existing loggers when dvc is set up #3345

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 27, 2020
Merged

dont disable existing loggers when dvc is set up #3345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c758d9
dont disable existing loggers when dvc is set up
ttekampe Feb 17, 2020
0d063bd
corrected code style
ttekampe Feb 17, 2020
c11b295
dont disable existing loggers, fixes
gurobokum Feb 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 8 additions & 16 deletions dvc/logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,13 @@ def _stack_trace(exc_info):
)


def disable_other_loggers():
root = logging.root
for (logger_name, logger) in root.manager.loggerDict.items():
if logger_name != "dvc" and not logger_name.startswith("dvc."):
logger.disabled = True


def setup(level=logging.INFO):
colorama.init()

Expand Down Expand Up @@ -183,22 +190,7 @@ def setup(level=logging.INFO):
"console_errors",
],
},
"paramiko": {
"level": logging.CRITICAL,
"handlers": [
"console_info",
"console_debug",
"console_errors",
],
},
"flufl.lock": {
"level": logging.CRITICAL,
"handlers": [
"console_info",
"console_debug",
"console_errors",
],
},
},
"disable_existing_loggers": False,
Copy link
Collaborator

@skshetry skshetry Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How noisy is gitpython (or, similar dependencies) on logging, @efiop? We might need to explicitly disable those for good, otherwise, it might mess with cli commands(?).

We need to reset loglevel of moto to logging.CRITICAL: getmoto/moto#2535, or just disable other loggers during test.

Copy link
Contributor

@efiop efiop Feb 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skshetry They are indeed fairly noisy. Maybe let's disable_existing_loggers in CLI and not touch anything in API? We do a similar thingy with some warnings in dvc/__init__.py, but that is rather incorrect. We should disable logs and warnings in dvc/main.py::main() most likely(this is where we init our log anyways). What do you think?

Copy link
Collaborator

@skshetry skshetry Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, handling in main.py is better. The question is, should we be explicitly disable other loggers there or disable every loggers except of DVC in the main.py?

Copy link
Contributor

@efiop efiop Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skshetry I would just disable everything except DVC, to avoid chasing every weird dependency logger out there.

Copy link
Collaborator

@skshetry skshetry Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note regarding adjustment of tests: If we disable all the loggers in main.py, tests where we do not work with main() such as Repo.push, etc will have all other loggers enabled. So, we might need to disable some loggers on the tests as well (such as moto which is making a test fail at the moment).

}
)
3 changes: 2 additions & 1 deletion dvc/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from dvc.exceptions import DvcParserError
from dvc.exceptions import NotDvcRepoError
from dvc.external_repo import clean_repos
from dvc.logger import FOOTER
from dvc.logger import disable_other_loggers, FOOTER
from dvc.utils import format_link
from dvc.remote.pool import close_pools

Expand All @@ -34,6 +34,7 @@ def main(argv=None):
"""
args = None
cmd = None
disable_other_loggers()

outerLogLevel = logger.level
try:
Expand Down
5 changes: 2 additions & 3 deletions tests/dir_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,12 @@

import os
import pathlib
import logging
from contextlib import contextmanager

import pytest
from funcy import lmap, retry

from dvc.logger import disable_other_loggers
from dvc.utils.fs import makedirs
from dvc.compat import fspath, fspath_py35

Expand All @@ -67,8 +67,7 @@


# see https://github.com/iterative/dvc/issues/3167
git_logger = logging.getLogger("git")
git_logger.setLevel(logging.CRITICAL)
disable_other_loggers()
Copy link
Collaborator

@skshetry skshetry Feb 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be great if we could move this to conftests. Not super important at the moment, but it does not belong here.



class TmpDir(pathlib.Path):
Expand Down