dont disable existing loggers when dvc is set up #3345
Conversation
|
Hi @ttekampe ! Thanks for the PR! Looks like 1 test on windows is failing, due to some |
|
Hi, thanks a lot for the prompt response and also for maintaining this nice package! |
dvc/logger.py
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@skshetry I would just disable everything except DVC, to avoid chasing every weird dependency logger out there.
There was a problem hiding this comment.
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).
gurobokum
force-pushed
the
master
branch
2 times, most recently
from
2a5704b to
18f70ac
Compare
gurobokum
force-pushed
the
master
branch
2 times, most recently
from
f2bf2c7 to
2ad8431
Compare
| # see https://github.com/iterative/dvc/issues/3167 | ||
| git_logger = logging.getLogger("git") | ||
| git_logger.setLevel(logging.CRITICAL) | ||
| disable_other_loggers() |
There was a problem hiding this comment.
It'd be great if we could move this to conftests. Not super important at the moment, but it does not belong here.
4 participants
❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
very small change that fixes #3344