Skip to content

Conversation

@casperdcl
Copy link
Contributor

@casperdcl casperdcl commented May 15, 2020

[...] if you are convinced that you need custom levels, great care should be exercised when doing this, and it is possibly a very bad idea to define custom levels if you are developing a library [...] if multiple library authors all define their own custom levels [...]

-- https://docs.python.org/3/howto/logging.html#custom-levels

We already do disable_other_loggers() so muhuhu hahaha.

-- @casperdcl

# before
dvc (logging-counts)$ dvc status -v | wc -l
34
# after moving state commands to trace (`-vv`)
dvc (logging-counts)$ python -m dvc status -v | wc -l
20

@casperdcl casperdcl added enhancement Enhances DVC refactoring Factoring and re-factoring ui user interface / interaction research discussion requires active participation to reach a conclusion labels May 15, 2020
@casperdcl casperdcl requested review from efiop and shcheklein May 15, 2020 20:14
@casperdcl casperdcl self-assigned this May 15, 2020
@casperdcl casperdcl changed the title logger: initial verbosity/quiet counts logger: add trace, count --verbose/--quiet May 15, 2020
Comment on lines 41 to 49
if args.quiet:
logger.setLevel(logging.CRITICAL)

elif args.verbose:
logger.setLevel(logging.DEBUG)
logger.setLevel(
{
-2: logging.CRITICAL,
-1: logging.ERROR,
0: logging.INFO,
1: logging.DEBUG,
2: logging.TRACE,
}[max(-2, min(args.verbose - args.quiet, 2))]
)
logger.trace(args)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this pretty much summarises this PR.

Comment on lines +77 to 80
"TRACE": colorama.Fore.GREEN,
"DEBUG": colorama.Fore.BLUE,
"WARNING": colorama.Fore.YELLOW,
"ERROR": colorama.Fore.RED,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not particularly fussed about colours - feel free to suggest others

@casperdcl casperdcl marked this pull request as ready for review May 16, 2020 00:11
@efiop efiop requested a review from skshetry May 16, 2020 11:14
@skshetry
Copy link
Collaborator

We already do disable_other_loggers() so muhuhu hahaha.

We only disable loggers on cli commands.

@casperdcl
Copy link
Contributor Author

We could still merge this and if/when a user takes issue with this we can look into work-arounds. Really using DVC API + another package which adds a logging level other than the fairly common TRACE=5 is probably very unlikely.

@efiop efiop requested a review from pmrowla May 20, 2020 02:40
Copy link
Contributor

@pmrowla pmrowla left a comment

Choose a reason for hiding this comment

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

agree with @casperdcl, even if defining custom log levels isn't recommended for libraries, it makes sense for us to have a trace level in DVC, and it seems pretty unlikely that it will actually cause any issues in the future.

@efiop efiop merged commit 184028f into iterative:master May 20, 2020
@efiop efiop mentioned this pull request Jul 30, 2020
2 tasks
efiop added a commit to efiop/dvc that referenced this pull request Jul 30, 2020
Part of the issue is a bug from iterative#3806

Kudos to @karajan1001 for finding this bug while working on iterative#4282
efiop added a commit that referenced this pull request Jul 30, 2020
Part of the issue is a bug from #3806

Kudos to @karajan1001 for finding this bug while working on #4282
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discussion requires active participation to reach a conclusion enhancement Enhances DVC refactoring Factoring and re-factoring research ui user interface / interaction

Projects

None yet

Development

Successfully merging this pull request may close these issues.

logger: disable state logging by default

4 participants