Spinalcordtoolbox: Change verbosity flag `-v 1` -> `-v`

[kousu]

It's more conventional in unix to pass --verbose or -v as a flag, not an option, i.e. it's existence determines whether it applies, not an argument passed after it.

Our wiki used to suggest an argument of -v 0 through -v 2 to control log level. I've changed this in sct_testing to be just `for "off" or-v` for "on". We should go through the rest of our CLIs and make them conform.

We can use action="count" so you can do $script -v -v -v or $script --verbose -vv or $script -vvv

Ref: https://github.com/neuropoly/spinalcordtoolbox/pull/2672#issuecomment-619124796_

[joshuacwnewton]

Hello! I'm interested in taking on this issue.

We should go through the rest of our CLIs and make them conform.

Just to confirm, it looks like each script in scripts/ implements its own get_parser(), each with separate verbosity options. And, you'd like each one to be updated to follow this new behaviour, right?

As a side note, a more long-term solution could be to use a centralized argparser that contains arguments common to each parser. Then, have each script inherit the common arguments from the base parser using parents, possibly. That way there's less boilerplate, and less need to tweak the same bits of code in many different places. That would be a bigger change, though, and affect more than just verbosity, so it might be better for a separate issue.

Either way, I'm happy to go with what you feel is best. Let me know what you think, and I can get started on a PR. :)

[jcohenadad]

@joshuacwnewton thank you so much for offering to help! 🎉

As you rightfully said, some CLIs are still using our custom parser via msct_parser, and we are (slowly) transitioning to Python's argparse https://github.com/neuropoly/spinalcordtoolbox/issues/1548. I think it would make more sense to first finalize the transition to a 100% argparse, and then change verbose behaviour. Would that be OK with you to work on that?

[joshuacwnewton]

Of course! Thank you @jcohenadad for clearing things up, as well as linking the relevant issue. I'll spend some time getting familiar with the previous work done on the argparse conversion (e.g. #2315, #2337) then we can continue chatting on the main issue, if that sounds good. :slightly_smiling_face:

[joshuacwnewton]

In addition to what @kousu says, I noticed as I was converting msct_parser to argparse that the verbosity isn't consistent between scripts, either.

Some have 2 verbose levels ('0' and '1') while others have 3 verbose levels ('0', '1', and '2'). Argparse description strings are also inconsistent: sometimes it's called 'off'/'on', sometimes it's called 'none'/'classic'/'extended', and sometimes the actual functionality is referenced, see below.

https://github.com/neuropoly/spinalcordtoolbox/blob/fce82d8c98fab1cc878c4e8c48fa1c515f0db35c/scripts/sct_flatten_sagittal.py#L123-L128

Might be good to hammer down how we want to communicate verbosity, and what types of output correspond to each level.

[jcohenadad]

Might be good to hammer down how we want to communicate verbosity, and what types of output correspond to each level.

💯 for the sake of simplicity, i would go with a single -v argument, that would be store_true, and the would bring logging level from INFO to DEBUG.

[joshuacwnewton]

for the sake of simplicity, i would go with a single -v argument, that would be store_true, and the would bring logging level from INFO to DEBUG.

@jcohenadad This makes sense to me, given that these are user-facing scripts! I now see that you also mentioned this here: https://github.com/neuropoly/spinalcordtoolbox/issues/2809#issuecomment-663619661 (for anyone else reading).

I've also updated the Wiki to clarify this information a little by adding a section called How logging appears to users of SCT scripts: https://github.com/neuropoly/spinalcordtoolbox/wiki/Programming:-Logging

[joshuacwnewton]

While looking at verbosity across SCT, I noticed that there are many places that rely on verbose == 2. Example:

https://github.com/neuropoly/spinalcordtoolbox/blob/bf3de687c20ac86717de3584296af1931e247784/scripts/sct_utils.py#L295-L298

I can simplify these cases to be simply if verbose: if our goal is to have verbosity be consistent across SCT.

But, if the distinction between verbose==1 and verbose==2 _does_ matter, then action="store_true" may not be sufficient.

[jcohenadad]

for these cases i would merge verbose 1 and 2, ie: if -v, then do what is currently under verbose 1 and 2

[joshuacwnewton]

I'm realizing now that this proposal has very similar issues as #2843. Many of our scripts have verbose output turned on by default, so action=store_true doesn't quite work nicely. Using 0/1 does indeed have a functional purpose: it allowed us to specify -v 0 to turn off verbose output.

I believe the suggestion made by @jcohenadad in https://github.com/neuropoly/spinalcordtoolbox/issues/2843#issuecomment-694586745 to harmonize usage with 0/1 (int) is applicable here, too.

EDIT: Please see https://github.com/neuropoly/spinalcordtoolbox/issues/2676#issuecomment-695128212.

[jcohenadad]

Indeed, but in this case there is an additional layer of complexity, because we might not _need_ the verbose=0 as initially designed. When I first designed this v=0, 1, 2 i had in mind the scenario where users would like no terminal output at all (v=0), but I realize now that this option seems irrelevant (ie: what's the problem with having those outputs?). So, we could also change the current behavior, and have:

• v=0: logging.level=info (default behaviour)
• v=1: logging.level=debug, with output of plotting and images for debugging purposes

or, we stay closer with the current design, and go for:

• v=0: logging.level=warning, ie minimum outputs.
• v=1: logging.level=info (default behaviour)
• v=2: logging.level=debug, with output of plotting and images for debugging purposes

[joshuacwnewton]

So, we could also change the current behavior, and have:

• v=0: logging.level=info (default behaviour)
• v=1: logging.level=debug, with output of plotting and images for debugging purposes

This is similar to what you proposed in https://github.com/neuropoly/spinalcordtoolbox/issues/2676#issuecomment-671135177, I believe. (INFO -> DEBUG)

This is what I had been following in my recent commits, so these changes are already done, oops. :sweat_smile:

[jcohenadad]

So, we could also change the current behavior, and have:

• v=0: logging.level=info (default behaviour)
• v=1: logging.level=debug, with output of plotting and images for debugging purposes

This is basically the same as what you proposed in #2676 (comment), right? (INFO -> DEBUG)

This is what I had been following in my recent commits, so these changes are already done, oops. 😅

Yes, you're absolutely right, this is what I have been proposing in #2676 (comment). I'm sorry to bring more confusion to this topic, but i'm still unsure as what is the best strategy here 😅

also, we should probably change the title of this issue for something like: "Update verbose arguments and usage to be integer 0/1 instead of string 0/1/2" (to reflect your PR)

[joshuacwnewton]

That was completely accidental, my apologies!

[joshuacwnewton]

Very sorry. There was a misunderstanding on my part.

• My comment, https://github.com/neuropoly/spinalcordtoolbox/issues/2676#issuecomment-695076627, was incorrect. I misunderstood.
• Julien's original suggestion, https://github.com/neuropoly/spinalcordtoolbox/issues/2676#issuecomment-671135177 is still good. action='store_true' is still a viable solution.

[joshuacwnewton]

SCT's current usage of verbosity is more complicated than I realized. It's used in subtly different ways in different parts of the project, e.g. sct.printv and parts of the core API, e.g. centerline/core.py, deepseg_legion/core.py, etc.) I describe this further in a draft PR: https://github.com/neuropoly/spinalcordtoolbox/pull/2897

I think trying to standardize this parameter requires a much deeper investment of time than I first thought.

[joshuacwnewton]

Removing this from the 5.0.0 milestone as to not hold up the release. If the remaining 5.0.0 tasks finish quickly and there's time to spare, perhaps it could be added back, but we can discuss that more in the upcoming Tuesday meeting. :slightly_smiling_face:

[joshuacwnewton]

Removing good first issue as to not mislead any new contributors (e.g. Hacktoberfest). This has become more complicated (and is lower priority compared to some other tasks).

[Drulex]

I know I'm late to the party, but I personally always use

usage: [...]
--log-level LOG_LEVEL     Logging level (eg. INFO, DEBUG, WARNING. See Python logging docs). Default INFO.

in my python CLI applications. It's simple and intuitive. There is no guessing of what the levels mean or what the default level is.

[jcohenadad]

i like that approach https://github.com/neuropoly/spinalcordtoolbox/issues/2676#issuecomment-723089582, it does indeed have the merit of being intuitive. The downside is that it implies large breakage in cross-compatibility.

[Drulex]

The downside is that it implies large breakage in cross-compatibility.

You can always support both for backwards compatibility and eventually deprecate one. Maybe print a warning when users use -v and say it will be deprecated in v6.0 or something. Meanwhile all docs use --log-level. It is a widely used approach.

[kousu]

3139 makes it easier to handle these generic parsers by centralizing a bunch of things in a SCTArgumentParser class.

I have a suggestion for how to handle moving all the redundant --help definitions into it at https://github.com/neuropoly/spinalcordtoolbox/pull/3139/files#r549895535:

    def add_argument_group(self, title, *args, **kwargs):
         g = [g for g in parser._action_groups if g.title == title]
         if g:
             return g
         else:
             return super().add_argument_group(title, *args, **kwargs)         

that should work to let you centralize -h, -v, -r, or anything else. And we can subclass SCTArgumentParser if there are subsets of scripts that need further shared parts that the rest of the scripts don't share.