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.

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

Jump to bottom

Replace --trace with --logdest in ArgumentParser #3

Merged
merged 1 commit into from
Sep 3, 2015
Merged

Replace --trace with --logdest in ArgumentParser #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 28 additions & 27 deletions python/lsst/pipe/base/argumentParser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ def __init__(self, name, usage = "%(prog)s input [options]", **kwargs):
usage = usage,
fromfile_prefix_chars = '@',
epilog = """Notes:
* --config, --configfile, --id, --trace and @file may appear multiple times;
* --config, --configfile, --id, --loglevel and @file may appear multiple times;
all values are used, in order left to right
* @file reads command-line options from the specified file:
* data may be distributed among multiple lines (e.g. one option per line)
Expand All @@ -239,9 +239,10 @@ def __init__(self, name, usage = "%(prog)s input [options]", **kwargs):
help="config override(s), e.g. -c foo=newfoo bar.baz=3", metavar="NAME=VALUE")
self.add_argument("-C", "--configfile", dest="configfile", nargs="*", action=ConfigFileAction,
help="config override file(s)")
self.add_argument("-L", "--loglevel", help="logging level")
self.add_argument("-T", "--trace", nargs="*", action=TraceLevelAction,
help="trace level for component", metavar="COMPONENT=LEVEL")
self.add_argument("-L", "--loglevel", nargs="*", action=LogLevelAction,
help="logging level; supported levels are [debug|warn|info|fatal] or an integer; "
"trace level is negative log level, e.g. use level -3 for trace level 3",
metavar="LEVEL|COMPONENT=LEVEL")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not immediately obvious to me that the reference to -n meant --loglevel -n --- it looks like another argument name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point.

self.add_argument("--debug", action="store_true", help="enable debugging output?")
self.add_argument("--doraise", action="store_true",
help="raise an exception on error (else log a message and continue)?")
Expand Down Expand Up @@ -328,8 +329,8 @@ def parse_args(self, config, args=None, log=None, override=None):
the value of which is a DataIdArgument that includes public elements 'idList' and 'refList'
- log: a pex_logging log
- an entry for each command-line argument, with the following exceptions:
- config is Config, not an override
- configfile, id, logdest, loglevel are all missing
- config is the supplied config, suitably updated
- configfile, id and loglevel are all missing
- obsPkg: name of obs_ package for this camera
"""
if args == None:
Expand Down Expand Up @@ -415,17 +416,6 @@ def parse_args(self, config, args=None, log=None, override=None):
if namespace.logdest:
namespace.log.addDestination(namespace.logdest)
del namespace.logdest

if namespace.loglevel:
permitted = ('DEBUG', 'INFO', 'WARN', 'FATAL')
if namespace.loglevel.upper() in permitted:
value = getattr(pexLog.Log, namespace.loglevel.upper())
else:
try:
value = int(namespace.loglevel)
except ValueError:
self.error("log-level=%s not int or one of %s" % (namespace.loglevel, permitted))
namespace.log.setThreshold(value)
del namespace.loglevel

namespace.config.validate()
Expand Down Expand Up @@ -723,28 +713,39 @@ def __call__(self, parser, namespace, values, option_string):
ident = getattr(namespace, argName)
ident.idList += idDictList

class TraceLevelAction(argparse.Action):
"""!argparse action to set trace level


class LogLevelAction(argparse.Action):
"""!argparse action to set log level
"""
def __call__(self, parser, namespace, values, option_string):
"""!Set trace level

@param[in] parser argument parser (instance of ArgumentParser)
@param[in] namespace parsed command (an instance of argparse.Namespace); ignored
@param[in] values a list of trace levels;
each item must be of the form component_name=level
each item must be of the form 'component_name=level' or 'level',
where level is a keyword (not case sensitive) or an integer
@param[in] option_string option value specified by the user (a str)
"""
permittedLevelList = ('DEBUG', 'INFO', 'WARN', 'FATAL')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't need permittedLevelList. If you don't want the below error to say "set(...)", then cast it to a list then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Casting a set to a list loses its order, and the order is useful in the error message. I agree it is annoying to save two such similar variable, but I didn't think of a simple, fast alternative. Two other solutiosn I did think of:

  • Cast to a set for each test, or search the list instead of the set. Neither of these slows parsing very much, but it irks me to use linear search.
  • Save the info as a dict of name: severity, then sort by severity when printing the message. In my opinion this adds too much code.

permittedLevelSet = set(permittedLevelList)
for componentLevel in values:
component, sep, levelStr = componentLevel.partition("=")
if not levelStr:
parser.error("%s level %s must be in form component=level" % (option_string, componentLevel))
try:
level = int(levelStr)
except Exception:
parser.error("cannot parse %r as an integer level for %s" % (levelStr, component))
pexLog.Trace.setVerbosity(component, level)

levelStr, component = component, None
logLevelUpr = levelStr.upper()
if logLevelUpr in permittedLevelSet:
logLevel = getattr(namespace.log, logLevelUpr)
else:
try:
logLevel = int(levelStr)
except Exception:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

except ValueError

And include the exception message in the error?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the value of including the error message. In my opinion the reported error says all that the user needs to know, and I worry about adding needless clutter. What am I missing?

parser.error("loglevel=%r not int or one of %s" % (namespace.loglevel, permittedLevelList))
if component is None:
namespace.log.setThreshold(logLevel)
else:
namespace.log.setThresholdFor(component, logLevel)


def setDottedAttr(item, name, value):
Expand Down
29 changes: 12 additions & 17 deletions tests/testArgumentParser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,40 +268,35 @@ def testLogLevel(self):
"""Test --loglevel"""
for logLevel in ("debug", "Info", "WARN", "fatal"):
intLevel = getattr(pexLog.Log, logLevel.upper())
print "testing logLevel=%r" % (logLevel,)
namespace = self.ap.parse_args(
config = self.config,
args = [DataPath, "--loglevel", logLevel],
)
self.assertEqual(namespace.log.getThreshold(), intLevel)
self.assertFalse(hasattr(namespace, "loglevel"))

fooBarLevel = intLevel + 27
bazLevel = -500
namespace = self.ap.parse_args(
config = self.config,
args = [DataPath, "--loglevel", str(intLevel)],
args = [DataPath, "--loglevel", str(intLevel),
"foo.bar=%d" % (fooBarLevel,),
"baz=0",
"baz=%s" % bazLevel, # test that later values override earlier values
],
)
self.assertEqual(namespace.log.getThreshold(), intLevel)

self.assertEqual(namespace.log.getThresholdFor("foo.bar"), fooBarLevel)
self.assertEqual(namespace.log.getThresholdFor("baz"), bazLevel)

self.assertRaises(SystemExit, self.ap.parse_args,
config = self.config,
args = [DataPath,
"--loglevel", "INVALID_LEVEL",
],
)

def testTrace(self):
"""Test --trace

I'm not sure how to verify that tracing is enabled; just see if it runs
"""
self.ap.parse_args(
config = self.config,
args = [DataPath,
"--trace", "afw.math=2", "meas.algorithms=3",
"--trace", "something=5",
],
)
pexLog.Trace("something", 4, "You should see this message")
pexLog.Trace("something", 6, "You should not see this message")

def testPipeVars(self):
"""Test handling of $PIPE_x_ROOT environment variables, where x is INPUT, CALIB or OUTPUT
"""
Expand Down