Parser issue with default value while using bash help

[NathanMolinier]

Description

To Improve code clarity and usage, some default options were configured within the parser based on other parser values, example:

https://github.com/spinalcordtoolbox/disc-labeling-benchmark/blob/d172ad3d9d33c56fcffc73c2ca68f4766435f675/src/bcm/run/extract_disc_cords.py#L105-L106

Where parser.parse_args().datapath is used in the default option of --out-txt-file.

However, even if this type of use is possible, a problem is visible when using the bash help

python src/bcm/run/extract_disc_cords.py -h

Output:

usage: extract_disc_cords.py [-h] [--datapath <folder>] -c CONTRAST

Extract discs coords from benchmark methods

optional arguments:
  -h, --help            show this help message and exit
  --datapath <folder>   Path to data folder generated using bcm/utils/gather_data.py Example: ~/<your_dataset>/vertebral_data (Required)
  -c CONTRAST, --contrast CONTRAST
                        MRI contrast: choices=["t1", "t2"] (Required)

Thus not showing other arguments after this use of a complex default

Solutions

Parser defaults need therefore to be simple or defined after all the arguments have been added.

[joshuacwnewton]

Here is what currently happens, line-by line:

https://github.com/spinalcordtoolbox/disc-labeling-benchmark/blob/d172ad3d9d33c56fcffc73c2ca68f4766435f675/src/bcm/run/extract_disc_cords.py#L96-L106

1. We run python src/bcm/run/extract_disc_cords.py -h
2. The parser is created with default argument -h (L97)
3. Two arguments (--datapath, --contrast) are added to the parser (L101, L103)
4. We attempt to add a third argument, --out-text-file, which calls parser.parse_args() as part of the default value.
   • parser.parse_args() is equivalent to saying parser.parse_args(args=None)
   • When args=None, argparse will use sys.argv[1:]
   • Meaning, argparse will fetch the -h that we specified on the command line!! And so, the parser exits the program at the end of -h, while we were in the middle of adding the third argument.

To avoid exiting early when running -h, we could potentially try this:

    # Make sure we remove `-h` and `--help` before we try to use `parse_args` within the `default` value
    argv_no_h = [arg for arg in sys.argv[1:] if arg not in ['-h', '--help']]

    # Then, when we're inside `parser.add_argument`, we make sure to specify `parser.parse_args(argv_no_h)`
    parser.add_argument('-txt', '--out-txt-file', default=os.path.abspath(os.path.join('results/files', f'{os.path.basename(os.path.normpath(parser.parse_args(argv_no_h).datapath))}_{CONTRAST[parser.parse_args(argv_no_h).contrast][0]}_hg{parser.parse_args(argv_no_h).ndiscs}_discs_coords.txt')),
                        type=str, metavar='N',help='Generated txt file path (default="results/files/(datapath_basename)_(test_CONTRAST)_hg(nb_class_hourglass)_discs_coords.txt")')

This will fix the "exiting early" problem, but it will reveal a new issue:

(venv_bcm) joshua@tadpole:~/repos/disc-labeling-benchmark$ python src/bcm/run/extract_disc_cords.py -h
usage: extract_disc_cords.py [-h] [--datapath <folder>] -c CONTRAST
extract_disc_cords.py: error: the following arguments are required: -c/--contrast

--contrast is a mandatory argument. So, as it is currently written, we can't just run -h by itself, because -h is dependent on parser.parse_args(), which is in turn dependent on -c.

---

Overall, I think that the best thing to do is remove the calls to parser.parser_args while you're in the middle of defining your arguments. This will allow you to call -h without needing to also call -c.

So, for example, if we move the values for default= outside of the parser, we get:

# Before
init_txt_file(parser.parse_args())

# After
arguments = parser.parse_args()
if arguments.out_txt_file is None:
    arguments.out_txt_file = os.path.abspath(os.path.join('results/files', f'{os.path.basename(os.path.normpath(arguments.datapath))}_{CONTRAST[arguments.contrast][0]}_hg{arguments.ndiscs}_discs_coords.txt'))
if arguments.skeleton_dir is None:
    arguments.skeleton_dir = os.path.join(parser.parse_args().datapath, 'skeletons')
init_txt_file(arguments)

And -h works just fine now. :)

[NathanMolinier]

Thank you @joshuacwnewton for your time and your comment ! I thought about using something similar after the parser definition but for me it is also making the script harder to debug when default values are defined after. However, I believe this is the only solution to have a working help.