Add CLI options to control logging verbosity #897
Conversation
This commit adds the `-v` and `-q` flag that control verbosity, respectively making the code more verbose or more quiet. Also adds these options to the throughput examples.
stephenswat
added
feature
|
| m_desc.add_options()( | ||
| "verbose,v", new verbosity_counter(&m_verbosity_decr), | ||
| "Increase verbosity (can be specified multiple times)")( | ||
| "quiet,q", new verbosity_counter(&m_verbosity_incr), | ||
| "Decrease verbosity (can be specified multiple times)"); |
There was a problem hiding this comment.
Interesting idea. But I think a way over-complicated one.
You can choose to either just make it into an integer option (my preference), or a string option. Which would be trivially translated into traccc::Logging::Level values.
I personally would find an --output-level=1 flag much more user friendly than having to write --verbose --verbose. 🤔
There was a problem hiding this comment.
The idea is that you write -vv, which is standard across a very broad range of CLI applications. Requiring the user to specify an integer level is much less ergonomic.
There was a problem hiding this comment.
I can tell you that in HEP applications specifying the output level is what we usually do. Be it ROOT, Athena, etc. I know you're right that some projects in the wild use the -vv formalism. But the -qq one I've definitely not seen yet. 🤔 So I don't think that's such a strong argument for this formalism.
I was trying to find examples underlining my case in https://github.com/acts-project/acts/tree/main/Examples/Scripts/Python, but none of those scripts allow you to set the output level. 😕 At least none that I would've found so far...
There was a problem hiding this comment.
Well, I don't recall ever seeing something like --output-level=X in the wild, and I think there is a good reason for that:
- The
-vflag is so engrained in the minds of people using CLI applications that anything other than it feels foreign. - It is not clear what numerical verbosity levels mean; is 0 the INFO level? Is 0 the most verbose level? Is 0 the most quiet level? There is no rhyme or reason to it.
- Writing the log levels out fully requires the user to know which levels are available, and this differs per application.
There was a problem hiding this comment.
🏳️ Fine, let's use -vv. This command line interface is not super important in the long run after all.
But you could still simplify the logic here a bit. Could you not have verbosity_counter be able to both increment and decrement the counter, based on a second constructor argument? Then you could have just a single integer member variable, which would have the "correct value" by the time the command line parsing finishes.
There was a problem hiding this comment.
I tried really hard to make that work, but I have found that Boost program options starts to really break down if you try to have multiple flags set the same variable. I kept running into some edge cases that would break the situation. I agree it would be nicer to have a single variable for this; I'd be very open to any suggestions to make this work.
| if (lvl == traccc::Logging::Level::VERBOSE) { | ||
| verbosity_string = "Verbose"; | ||
| } else if (lvl == traccc::Logging::Level::DEBUG) { | ||
| verbosity_string = "Debug"; | ||
| } else if (lvl == traccc::Logging::Level::INFO) { | ||
| verbosity_string = "Info"; | ||
| } else if (lvl == traccc::Logging::Level::WARNING) { | ||
| verbosity_string = "Warning"; | ||
| } else if (lvl == traccc::Logging::Level::ERROR) { | ||
| verbosity_string = "Error"; | ||
| } else if (lvl == traccc::Logging::Level::FATAL) { | ||
| verbosity_string = "Fatal"; | ||
| } |
There was a problem hiding this comment.
- This would seem like a perfect place for
switch--case; - Why change the capitalization on the printout wrt. the enum names? I would prefer to keep the same capitalized names for the printout at well.
There was a problem hiding this comment.
Hmm, I personally prefer to avoid switch statements whenever convenient because they more confusing and error prone than if-else statements, but I don't feel tremendously strongly about it.
Neither do I feel strongly about the capitalization! We can keep it all-caps in line with the enum values.
There was a problem hiding this comment.
Also, we could just use the following heere:
https://github.com/acts-project/acts/blob/main/Core/include/Acts/Utilities/Logger.hpp#L168
There was a problem hiding this comment.
Oh that's a really nice idea!
This commit adds the
-vand-qflag that control verbosity, respectively making the code more verbose or more quiet. Also adds these options to the throughput examples.