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

Logging configuration #52

Closed
elgow opened this issue Mar 15, 2018 · 14 comments
Closed

Logging configuration #52

elgow opened this issue Mar 15, 2018 · 14 comments

Comments

@elgow
Copy link

elgow commented Mar 15, 2018

First let me say I'm a big fan of defopt. I'd like to suggest a new feature related to #7 but simpler. defopt is designed to allow functions to be used as library functions from Python or as CLI programs. To incorporate logging for both cases requires some care. I'd like to suggest some small enhancements that would handle most of it.

  1. Have defopt do
if not logging.getLogger().handlers:
  logging.getLogger().addHandler(logging.NullHandler(logging.WARN))

during module loading, as having some null handler avoids generating error messages from logging calls in libraries.

  1. Add kwargs to defopt.run() along the lines of:
log_level=logging.WARN
log_file=None
log_handler=None

for specifying those things from the main block to take effect only when functions are run from the defopt CLI.

  1. Add a switch argument to defopt.run() like log_args=False that could be set to enable a standard set of CLI args for logging config as if the function had the following in its argument list:
:param str log_level: choice of standard levels 
:param str log_file: path to log file, or special constants 'stderr', 'stdout'

If this were added to defopt then fairly complete logging configuration would be available with minimal effort and w/o compromising the promise that defopt won't interfere with using your functions from Python.

@anntzer
Copy link
Owner

anntzer commented Mar 16, 2018

On Py3 at least, I think 1) is redundant with the lastResort handler (https://docs.python.org/3/library/logging.html#logging.lastResort). Doesn't mean we can't add it on Py2 (but would defer to @evanunderscore on that as I don't really care about Py2 :-)).
Not sure what 2) provides given that it is basically just as easy to add the corresponding calls immediately before the call to defopt.run(). (As defopt itself doesn't use logging, any logs would come from the passed-in functions.)
For 3) I personally like using environment variables and https://coloredlogs.readthedocs.io/en/latest/#environment-variables, which gives a much nicer and more general solution (although it doesn't allow logging to a file, but that can generally be solved using redirection (obviously, not if you're already writing to stderr intentionally yourself)).

@elgow
Copy link
Author

elgow commented Mar 16, 2018

Thanks @anntzer for the reply. I'm on Py3 so I share your lack of concern about Py2. On point 2) I'd only say that if the defaults for defopt run() were such that no logging config would be required to get basic stderr logging then it would eliminate some boilerplate from defopt apps. Similarly for 3) it could be boilerplate free, and IMHO CLI arguments are clearer and easier than env vars because they are explicit and can't be inadvertently set.

That said, coloredlogs seems to do most of what I need and it is better than any logging library that I'd evaluated (there are dozens on PyPI). I do want the option of log files with rotation which it doesn't seem to provide, but that's involved enough that explicitly coding it is probably better anyway.

@anntzer
Copy link
Owner

anntzer commented Mar 16, 2018

On point 2) I'd only say that if the defaults for defopt run() were such that no logging config would be required to get basic stderr logging then it would eliminate some boilerplate from defopt apps.

Well, the stdlib default is to log warnings (and higher) via the lastresorthandler.
Requesting that defopt incorporates logging support just so that you can write import defopt, logging; defopt.run(..., log_level=logging....) instead of import defopt, logging; logging.basicConfig(logLevel=logging....); defopt.run(...) seems a bit rich, doesn't it? :-)

@elgow
Copy link
Author

elgow commented Mar 16, 2018

My real interest in point 2) was writing only defopt.run(func) and getting sane logging with zero additional boilerplate code. I view logging as a basic thing that should be in all applications, so it seems to me that it would not be a bad thing if defopt put it there by default. Certainly writing one additional line every time I write a defopt CLI utility won't kill me, but it doesn't do me any great service either. Built-in logging setup would be one more small way in which defopt can make writing proper CLI utilities easier.

My real interest, though, is in point 3). I programmed in Java for years and I liked the ability to adjust logging levels from the CLI. I have not yet found a Python logging library that supports that. The one you pointed me to, coloredlogs, does support setting the level from env vars, which is a close second, and even that seems to be a rare capability for Python logging libraries. The stdlib logging.config allows for setting levels w/o modifying the Python code, but setting it up with proper defaults adds a lot of extra effort to the beautiful simplicity of defopt and it still leaves you modifying a file to adjust logging.

By pointing me to coloredlogs you have given me a reasonable solution for my immediate needs, and I thank you for that. I would only suggest that you and @evanunderscore think about whether logging is something that should be in every CLI app and, if so, whether defopt should somehow support it.

@anntzer
Copy link
Owner

anntzer commented Mar 16, 2018

I think one of us (possibly me) is missing something fundamental on 2): AFAICT, you need zero configuration to get logging (on Py3):

$ python -c 'import logging; logging.warning("foo")'
WARNING:root:foo

(having temporarily disabled coloredlogs)
So I don't understand what exactly you want defopt to do here.

As for 3), I actually wrote the COLOREDLOGS_AUTO_INSTALL feature (the other environment variables were already present before) so obviously I'm a bit biased there :-) I guess a possible design for what you're looking for would be

defopt.run(..., log_args_prefix="foo-")

and redirect any --foo-* kwargs to logging.basicConfig (which right now has signature *args, **kwargs and no annotations but an shim typed signature can easily be provided).
I think it's important to have an explicit prefix because I don't think I'd be happy to have any risk of collision with parameters of the actual function I'm calling.

PRs are always welcome...

@elgow
Copy link
Author

elgow commented Mar 16, 2018

One of the key reasons that I use defopt is so that there is excellent built-in help with the -h argument. Would the --foo-... solution that you propose above provide help for the use of logging configuration arguments?

@elgow
Copy link
Author

elgow commented Mar 16, 2018

On 2), I was unaware that Py3 had fixed the dreaded "No handler found..." error, so thank you for educating me on that. If defopt aims to support Py2 then it may still be worthwhile to add a NullHandler when needed. Alternatively leaving it out could be viewed as a feature which encourages people to switch to Py3 :^)

@anntzer
Copy link
Owner

anntzer commented Mar 16, 2018

One of the key reasons that I use defopt is so that there is excellent built-in help with the -h argument. Would the --foo-... solution that you propose above provide help for the use of logging configuration arguments?

yes. that's provided by argparse (the underlying stdlib parser)

@elgow
Copy link
Author

elgow commented Mar 16, 2018

I think I see what you are getting at with an log_args_prefix="foo-"' argument to defopt.run().

One question I have is about the arg prefix. I would expect the log args to be optional keyword args. For those, defopt expects that the 1st character be unique in order to get unambiguous short args. How would that work with an arg prefix? One possibility seems to be the short={...} arg for run(), but that's pretty tedious. If there was a way to make arguments long-only then that could be the default for the log args and short= could be used by anyone who really wanted short alternatives.

It does appear that it would require a wrapper to logging.basicConfig because the docstring for basicConfig is not formatted properly for defopt. It also appears that many basicConfig args are not suitable for use from a CLI. Even for the most basic one, log level, some work seems to be required to represent it nicely as a choice by converting the keys in the logging._nameToLevel dict into an enum.

Surfacing logging.basicConfig in some fashion with CLI args does seem like a very good approach to what I was seeking in my item 3).

@anntzer
Copy link
Owner

anntzer commented Mar 16, 2018

Given that the support for basicConfig would have to be hardcoded anyways, we can also hardcode whatever short args we want for them (by default)...

@evanunderscore
Copy link
Collaborator

My real interest, though, is in point 3). I programmed in Java for years and I liked the ability to adjust logging levels from the CLI. I have not yet found a Python logging library that supports that.

I know begins does it: https://begins.readthedocs.io/en/latest/guide.html#logging

Doesn't mean we can't add it on Py2 (but would defer to @evanunderscore on that as I don't really care about Py2 :-)).

I no longer personally use Python 2 for anything, so I'm not overly inclined to make any more concessions for it unless someone asks for them specifically.

I don't have any time to work on this feature myself but you're welcome to keep discussing it and submit a PR if you'd like. Thanks for all the discussion so far.

@elgow
Copy link
Author

elgow commented Mar 31, 2018

Thanks for looking in on the discussion and for the pointer to begins. I'll look at what begins does and see if I can formulate something for defopt. I tried coloredlogs and found that the granularity was too coarse. To make it really useful really requires an ability like Java's to control logging at the individual logger level.

Thanks for creating defopt. I use it all the time. It's made creating CLI utils a pleasure and it's trained me to always make nice docstrings.

@evanunderscore
Copy link
Collaborator

Feel free to post ideas here and I'll weigh in when I can. I'm glad to hear you've been enjoying defopt!

@anntzer
Copy link
Owner

anntzer commented Jan 2, 2020

I'm going to close this as inactive and because I think coloredlogs-style environment variables work well enough (well, at least for me...), but would still be willing to review a PR implementing log_args_prefix or some variant thereof.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants