Spinalcordtoolbox: Replace usage of custom command-line parser (`msct_parser`) with argparse

[zougloub]

Original issue description hidden, click to show

Title: Some griefs with the command-line arguments parser

SCT implements its own command-line parser (vs. using the built-in argparse or 3rd party alternatives) and it's not very practical.

• Is there a good reason for this? I see that the parser code can do things such as doing basic checks on parameters (eg. making sure an image is a NIFTI, making sure a folder exists) but this can be done with standard ones too.

• There's a good practice of supplying both long options (--image-contrast) and short options (-c), with documentation favoring the long ones. The parser doesn't do that.

• Inconsistencies in usage of parameters (eg. -rtmp vs. -r). Not related to the parser per se.

• Parser output doesn't adapt to terminal width.

• Shell completion can't be implemented easily. In the same way that Python users could be using IDE completion for function parameters, having shell completion could be helpful. It could be implemented in 4 lines of code using the optional argcomplete if using a standard parser.

I'd suggest to perform an evaluation of replacement of the parser (ensuring compatibility with current uses, or not), and to go ahead and do it.

---

Replacement of SCT's custom msct_parser with Python's argparse is currently in-progress. The remaining uses of msct_parser that are exposed to users are:

• /scripts/isct_warpmovie_generator.py (non-functional script, see #2828)
• [x] /scripts/sct_dmri_display_bvecs.py
• [x] /scripts/sct_dmri_moco.py
• [x] /scripts/sct_dmri_separate_b0_and_dwi.py
• [x] /scripts/sct_dmri_transpose_bvecs.py
• [x] /scripts/sct_download_data.py
• [x] /scripts/sct_extract_metric.py
• [x] /scripts/sct_flatten_sagittal.py
• [x] /scripts/sct_fmri_compute_tsnr.py
• [x] /scripts/sct_fmri_moco.py
• [x] /scripts/sct_get_centerline.py
• [x] /scripts/sct_label_utils.py
• [x] /scripts/sct_label_vertebrae.py
• [x] /scripts/sct_process_segmentation.py
• [x] /scripts/sct_propseg.py
• [x] /scripts/sct_register_multimodal.py
• [x] /scripts/sct_register_to_template.py
• [x] /scripts/sct_resample.py
• [x] /scripts/sct_smooth_spinalcord.py
• [x] /scripts/sct_warp_template.py

Of note, msct_parser is also used in three non-user facing scripts:

• /dev/sct_nin_convert_dcm2nii.py
• /install/compile_external.py
• /spinalcordtoolbox/gui/cli.py

[zougloub]

Took a few minutes to port a command to using argparse's argument parser, and some more to report on it.

"before":

$ sct_deepseg_sc_orig --help
ERROR: argument --help does not exist. See documentation.
Aborted...

DESCRIPTION
Spinal Cord Segmentation using deep convolutional networks.

USAGE
sct_deepseg_sc_ref -i <image_nifti>

MANDATORY ARGUMENTS
 -i <image_nifti>             input image.                  

OPTIONAL ARGUMENTS
 -c {t1,t2,t2s,dwi}           type of image contrast.       
 -ofolder <folder_creation>   output folder.                
 -r {0,1}                     remove temporary files. Default value = 1
 -v {0,1}                     1: display on, 0: display off (default) Default value = 1
 -qc <folder_creation>        The path where the quality control generated content will be saved 
                              Default value = /home/cJ/qc_data
 -noqc                        Prevent the generation of the QC report
 -igt <image_nifti>           File name of ground-truth segmentation.

Traceback (most recent call last):
  File "../../../scripts/sct_deepseg_sc_ref.py", line 406, in <module>
    main()
  File "../../../scripts/sct_deepseg_sc_ref.py", line 220, in main
    arguments = parser.parse(args)
  File "/home/cJ/Work/exmakhina/neuropoly/code/spinalcordtoolbox/scripts/msct_parser.py", line 379, in parse
    self.usage.error("ERROR: argument " + arg + " does not exist. See documentation.")
  File "/home/cJ/Work/exmakhina/neuropoly/code/spinalcordtoolbox/scripts/msct_parser.py", line 572, in error
    self.generate(error)
  File "/home/cJ/Work/exmakhina/neuropoly/code/spinalcordtoolbox/scripts/msct_parser.py", line 565, in generate
    raise SyntaxError(error)
SyntaxError: ERROR: argument --help does not exist. See documentation.
$ echo $?
1

"after":

sct_deepseg_sc --help
usage: sct_deepseg_sc.py [-h] -i INPUT_IMAGE_NIFTI -c {t1,t2,t2s,dwi} [-ofolder OUTPUT_FOLDER] [-r {0,1}] [-v {0,1}] [-qc QC_FOLDER]

Spinal Cord Segmentation using deep convolutional networks.

optional arguments:
  -h, --help          show this help message and exit
  -ofolder OUTPUT_FOLDER    output folder
  -r {0,1}            remove temporary files.
  -v {0,1}            1: display on, 0: display off (default)
  -qc QC_FOLDER              The path where the quality control generated content will be saved

required arguments:
  -i INPUT_IMAGE_NIFTI                input image, eg. t1.nii.gz.
  -c {t1,t2,t2s,dwi}  type of image contrast.
$ echo $?
0

for this file, the command-line is compatible.

Further considerations:

• msct_parser doesn't use positional arguments, which is the standard way of passing required arguments. It uses what everybody calls optional arguments for that, which, because it's non-standard, didn't get implemented in the Python standard library and doesn't result in the best of behaviors (https://docs.python.org/3/library/argparse.html#required); trying to group the required arguments together results in them being after the default optional arguments.

• msct_parser shows -variable_name <variable_type> whereas python will display -option_name METAVAR_NAME.

• msct_parser generates display of default option values; argparse doesn't (possibly because these are provided post-construction, ie. you say type=file, default=sys.stdout).

• argparse automatically adds the standard -h,--help arguments and behaves nicely when called with it (ie. you don't expect a program to give a non-null error code when called to get the help.

• argparse automatically creates namespace structures with the variable names inside, so you refer to arguments.qc vs arguments["-qc"]; this makes the code shorter and less error-prone.

• msct_parser features some verification code (eg. handling output folder creation, renaming the option of a .nii file which doesn't exist but whose .nii.gz does); this can be made to work with argparse by using argument type functions.

• good practice for command-line entry points is to not "import the world" before running, because it can take a while. On my laptop with a fast SSD, calling sct_deepseg_sc.py --help takes over 6 seconds.

• on top of command-line completion possibilities, there's a Sphinx module that can generate command-line docs for programs using argparse. Today there may be some command-line docs on SourceForge.

I'd advocate at least new CLIs to switch to argparse, unless the output style is sacred or we want to "improve" msct_parser.

[zougloub]

Illustration of change for a modified sct_deepseg_gm (@perone's care makes this easy) (the option variable names & metavars aren't done), which results in:

$ time sct_deepseg_gm --help
usage: sct_deepseg_gm [-h] -i I [-o O] [-m {large,challenge}] [-v {0,1}]

Spinal Cord Gray Matter (GM) Segmentation using deep dilated convolutions. Reference: CS Perone, E Calabrese, J Cohen-Adad. Spinal cord gray matter segmentation using deep dilated convolutions (2017). arxiv.org/abs/1710.01269

optional arguments:
  -h, --help            show this help message and exit
  -i I                  Image filename to segment (3D volume). Contrast must be similar to T2*-weighted, i.e., WM dark, GM bright and CSF bright (eg. t2s.nii.gz)
  -o O                  Output segmentation file name. If not specified, based on the input file name.
  -m {large,challenge}  Model to use (large or challenge).The model 'large' will be slower but will yield better results. The model 'challenge' was built using data from the following challenge: goo.gl/h4AVar.
  -v {0,1}              Verbose: 0 = no verbosity, 1 = verbose (default).

real    0m0.114s
user    0m0.099s
sys     0m0.013s

diff --git a/scripts/sct_deepseg_gm.py b/scripts/sct_deepseg_gm.py
index 0621c322..986ff718 100755
--- a/scripts/sct_deepseg_gm.py
+++ b/scripts/sct_deepseg_gm.py
@@ -11,6 +11 @@
-import sys
-
-import sct_utils as sct
-from msct_parser import Parser
-
-from spinalcordtoolbox.deepseg_gm import deepseg_gm
+import sys, argparse
@@ -20,21 +15,16 @@ def get_parser():
-    parser = Parser(__file__)
-    parser.usage.set_description('Spinal Cord Gray Matter (GM) Segmentation using deep dilated convolutions. '
-                                 'Reference: CS Perone, E Calabrese, J Cohen-Adad. Spinal cord gray matter segmentation using deep dilated convolutions (2017). arxiv.org/abs/1710.01269')
-
-    parser.add_option(name="-i",
-                      type_value="file",
-                      description="Image filename to segment (3D volume). Contrast must be similar to T2*-weighted, i.e., WM dark, GM bright and CSF bright.",
-                      mandatory=True,
-                      example='t2s.nii.gz')
-
-    parser.add_option(name="-o",
-                      type_value="file_output",
-                      description="Output segmentation file name.",
-                      mandatory=False,
-                      example='sc_gm_seg.nii.gz')
-
-    parser.usage.addSection('\nMISC')
-
-    parser.add_option(name="-m",
-                      type_value='multiple_choice',
-                      description="Model to use (large or challenge)."
+    parser = argparse.ArgumentParser(
+     description='Spinal Cord Gray Matter (GM) Segmentation using deep dilated convolutions. '
+      'Reference: CS Perone, E Calabrese, J Cohen-Adad. Spinal cord gray matter segmentation using deep dilated convolutions (2017). arxiv.org/abs/1710.01269',
+    )
+
+    parser.add_argument("-i",
+                      help="Image filename to segment (3D volume). Contrast must be similar to T2*-weighted, i.e., WM dark, GM bright and CSF bright (eg. t2s.nii.gz)",
+                      required=True,
+                      )
+
+    parser.add_argument("-o",
+                      help="Output segmentation file name. If not specified, based on the input file name.",
+                      )
+
+    parser.add_argument("-m",
+                      help="Model to use (large or challenge)."
@@ -43,3 +33,2 @@ def get_parser():
-                      mandatory=False,
-                      example=['large', 'challenge'],
-                      default_value='large')
+                      choices=('large', 'challenge'),
+                      default='large')
@@ -47,6 +36,6 @@ def get_parser():
-    parser.add_option(name="-v",
-                      type_value='multiple_choice',
-                      description="Verbose: 0 = no verbosity, 1 = verbose.",
-                      mandatory=False,
-                      example=['0', '1'],
-                      default_value='1')
+    parser.add_argument("-v",
+                      help="Verbose: 0 = no verbosity, 1 = verbose (default).",
+                      choices=('0', '1'),
+                      type=int,
+                      default=1,
+                      )
@@ -57 +46,3 @@ def get_parser():
-def run_main():
+def run_main(arguments):
+    import sct_utils as sct
+    from spinalcordtoolbox.deepseg_gm import deepseg_gm
@@ -59,3 +50,3 @@ def run_main():
-    parser = get_parser()
-    arguments = parser.parse(sys.argv[1:])
-    input_filename = arguments["-i"]
+    sct.start_stream_logger()
+
+    input_filename = arguments.i
@@ -64 +55 @@ def run_main():
-        output_filename = arguments["-o"]
+        output_filename = arguments.o
@@ -68,2 +59,2 @@ def run_main():
-    verbose = arguments["-v"]
-    model_name = arguments["-m"]
+    verbose = arguments.v
+    model_name = arguments.m
@@ -72 +63 @@ def run_main():
-                                        model_name, int(verbose))
+                                        model_name, verbose)
@@ -81,2 +72,3 @@ if __name__ == '__main__':
-    sct.start_stream_logger()
-    run_main()
+    parser = get_parser()
+    arguments = parser.parse_args()
+    run_main(arguments)

[jcohenadad]

@zougloub how about planning to move to argparse in 4.0.0? Since we will be breaking some compatibilities anyway, this would be a good opportunity.

[jcohenadad]

I suggest we refactor SCT's CLI functions by chunks of 10-15 functions within single PR, to make the implementation/review/merging workflow easier for the team. For the first PR, we could take care of the following functions (alphabetically sorted):

• [x] sct_analyze_lesion
• [x] sct_analyze_texture
• [x] sct_apply_transfo
• [x] sct_check_dependencies
• [x] sct_compute_ernst_angle
• [x] sct_compute_hausdorff_distance
• [x] sct_compute_mscc
• [x] sct_compute_mtr
• [x] sct_compute_mtsat
• [x] sct_compute_snr
• [x] sct_concat_transfo
• [x] sct_convert
• [x] sct_create_mask
• [x] sct_crop_image

For inspiration, see sct_compute_mtsat, sct_qc

[YangHee-Min]

When using the command sct_compute_mtsat -h, in terminal, we see that even the required arguments are listed as optional. Do we want to leave it like this or do we want to find an alternative (i.e. terminal displays mandatory and optional arguments separately in terminal)?

[jcohenadad]

@YangHee-Min good question: i suggest we leave it like this (users will receive an error message if they don't enter the mandatory arguments anyway)

[YangHee-Min]

@YangHee-Min good question: i suggest we leave it like this (users will receive an error message if they don't enter the mandatory arguments anyway)

When reviewing the script for sct_crop_image, I noticed that this script used different groups of arguments as seen (here) with the different groups including COMMAND LINE RELATED MANDATORY ARGUMENTS, GUI RELATED OPTIONAL ARGUMENTS and COMMAND LINE RELATED OPTIONAL ARGUMENTS. This would mean that for the other scripts, having a mandatory argument group would be possible and I think that displaying the mandatory/optional arguments to the user would clarify how to use the commands.

Do you suggest we simply proceed with the different argument groups for sct_crop_image and leave the other scripts with all the arguments displayed under 'optional arguments' or should we proceed to separate the arguments in different groups (mandatory/optional)?

[jcohenadad]

Can you propose a prototype for compute mtsat where you use grouping?

[YangHee-Min]

Sure! (Here) is an example of sct_compute_mtsat with grouping

[jcohenadad]

Can you pls add in this thread the output of the help?

[YangHee-Min]

You can find the output of the help (here)

[jcohenadad]

i like that! i guess it is not "easily" possible to put mandatory arguments before optional arguments? small comment: i would put a capital letter at the beginning for "Mandatory arguments" (same for Optional).
@zougloub what do you think about this refactoring approach?

[YangHee-Min]

i like that! i guess it is not "easily" possible to put mandatory arguments before optional arguments?

It is possible to do so by removing the default options for the help and adding the help argument to the argument group "Optional arguments" as seen (here) to obtain a help log that looks like (this)

[jcohenadad]

It looks very nice! One last small thing: can you change the "usage: sct_compute_mtsat.py " into "usage: sct_compute_mtsat" when displaying the usage (at the beginning)?

[jcohenadad]

@YangHee-Min could you please start opening a PR so we can comment/review code in it?

[YangHee-Min]

Sounds good. Done in commit ebe82f8 and it prints a help log that looks like (this)

[joshuacwnewton]

Hi @jcohenadad! I've updated the title and description of this issue to better reflect the in-progress conversion.

I've counted 23 remaining uses of msct_parser: 20 in scripts/, 1 in spinalcordtoolbox/gui/, 1 in dev/ and 1 in install/ (see the issue description for the full To-do list). I can split these up into 2 PRs to fit the suggestion in https://github.com/neuropoly/spinalcordtoolbox/issues/1548#issuecomment-498040515.

I've also updated the GitHub project board with issues related to the conversion, e.g. issues with the msct_parser GitHub label, and argparse issues that can't start until the conversion is finished.

If this sounds alright, I'd be happy to start working on the conversion right away. :)

[jcohenadad]

@joshuacwnewton awesome! just fix the CLIs that are exposed to the user. see in setup.py which ones they are

[joshuacwnewton]

Sounds good, @jcohenadad. :)

While converting isct_warpmovie_generator.py, I realized the script itself isn't actually functional, I believe. I've summarized the issues I found in #2818, but for now I'll move onto another script.

This is an internal script, I think? ("isct") But it is still listed in setup.py, so if I understand correctly it's still intended to be used by users and developers alike.

[jcohenadad]

This is an internal script, I think? ("isct") But it is still listed in setup.py, so if I understand correctly it's still intended to be used by users and developers alike.

you understood correctly. it is largely underused thought, so no emergency here 😅

[joshuacwnewton]

I've noticed that the old msct_parser has some special parsing behavior for lists of args:

https://github.com/neuropoly/spinalcordtoolbox/blob/d137a7ef0a6bad2a72f2b5df4edf38922242298c/scripts/msct_parser.py#L179-L197

This doesn't port 1:1 to argparse. I'd say there's two ways to handle this:

1. Switch to using nargs="+" and change to require space-delimiting (instead of using , and :). This was what was done by @YangHee-Min in #2315, e.g. this change here. This is more canonical, but this is also a breaking change, and thus requires updates for users as well as our usage (e.g. in tests, internal scripts). This also might not even work for nested lists which use two delimiters, e.g. x1,y1,z1,val1:x2,y2,z2,val2 for coordinates.
2. Preserve the old behavior using a custom argparse action. Here's a really quick sketch of what that might look like.

# Definition (possibly in spinalcordtoolbox.utils, where `Metavar` and `SmartFormatter` are too
class ListAction(argparse.Action):
    def __init__(self, option_strings, delimiter, subtype *args, **kwargs):
        self.delimiter = delimiter
        self.subtype = subtype
        super(ListAction, self).__init__(option_strings, *args, **kwargs)

    def __call__(self, parser, namespace, values, option_string=None):
        values = # Delimiting behavior goes here
        setattr(namespace, self.dest, values)

# Usage
from spinalcordtoolbox.utils import ListAction
optional.add_argument(
    '-create',
    action=ListAction(delimiter=":", subtype=Coordinate),
)

Making a custom action would allow us to keep the existing tests/usage as-is, which I'd prefer as to minimize the changed lines of code. But, then again, the nargs="+" change was already imposed in the previous conversion PRs. I'll have to see how prevalent this is in the codebase and provide an update accordingly.

[jcohenadad]

yup, i was waiting for this to happen at some point 😉 . My inclination would be to minimize code change and go with the custom action, when that makes sense. For example, the "x1,y1,z1,val1:x2,y2,z2,val2" in sct_label_utils or step=1,key1=valueX:step=2,key1=valueY in sct_register_multimodal are implemented everywhere (in course material, custom scripts for users, etc.), so changing that might be too disturbing. However, in some cases, the change of CLI behaviour could be acceptable.

Please don't hesitate to bring this debate on a case-by-case basis.

[joshuacwnewton]

are implemented _everywhere_ (in course material, custom scripts for users, etc.), so changing that might be too disturbing

Thank you, @jcohenadad! This is very insightful. I'll keep this in mind as I move forward. :smile:

[joshuacwnewton]

Ah, I've realized there's a much cleaner solution than using a custom action. From the argparse docs:

type= can take any callable that takes a single string argument and returns the converted value

This means we can use a factory function to generate typecasting functions for lists on the fly.

[joshuacwnewton]

Just to follow up on your https://github.com/neuropoly/spinalcordtoolbox/issues/1548#issuecomment-667540163, @jcohenadad:

@joshuacwnewton awesome! just fix the CLIs that are exposed to the user. see in setup.py which ones they are

There are still a few remaining non-user-facing usages of msct_parser that I want to ask about before closing this issue:

• dev/sct_nin_convert_dcm2nii.py
• install/compile_external.py: (observation: Python 2 script -- can I assume this is not used anymore?)
• scripts/isct_warpmovie_generator.py (observation: broken, possibly could be moved to dev/ for now until #2828 is addressed?)
• spinalcordtoolbox/gui/cli.py (observation: might make sense to refactor this as scripts/sct_label_manual.py if this would be useful for users. if so, update argparse at the same time.)

With your background, you might have a better understanding of the usage of these scripts than I do: Are any of these used enough to be worth updating?

If not, then perhaps the obsolete code could be dealt with before this issue is closed (e.g. by removing the scripts and removing msct_parser, or moving things to dev/).

[joshuacwnewton]

https://github.com/neuropoly/spinalcordtoolbox/issues/1548#issuecomment-695356992 was brought up in today's meeting, and the response was:

• dev/sct_nin_convert_dcm2nii.py: No longer used.
• install/compile_external.py: No longer used.
• scripts/isct_warpmovie_generator.py: Broken, but a very useful tool, so it is worth fixing. Move to dev.
• spinalcordtoolbox/gui/cli.py: The existence of this was somewhat of a surprise to the team. I think what to do with this is worth discussing further in a dedicated issue, so this is outside the scope of this issue. This is created here: #2904

I'll do a bit of cleanup in one final PR that will close this issue.