Deepreg: Argparse broken for train and predict scripts due to incorrect argument in adding --gpu_allow_growth flag.

Created on 21 Jun 2020  Â·  2Comments  Â·  Source: DeepRegNet/DeepReg

Issue description

  1. The argparse implementation for the flag ‘--gpu_allow_growth ‘ in deepreg/train and deepreg/predict returns an error. The expected behaviour is described in the README where gpu_allow_growth should be an optional flag which instructs TF whether or not to allocate whole GPU memory or not. Calling the command line applications returns an error indicating one of the arguments is not valid. a) Need to fix this error and b) reflect the outlined functionality in the README at #52 for argparse where --gpu_allow_growth is a flag with no input.
  2. Additionally, the docs for argparse commands have been updated in the readme at #52, it would be nice to update the docs in the command line interfaces such that calling --help is actually helpful/informative to the end user.

 Type of issue

Please delete options that are not relevant.

  • [x] Bug report
  • [ ] New feature request
  • [x] Documentation update
  • [ ] Test request
  • [ ] Linting request

For bug report or feature request, steps to reproduce the issue, including (but not limited to):

  1. OS: MacOS High Sierra 10.13.6
  2. Environment to reproduce: pip install .e in an empty venv like instructed in README.
  3. What commands run / sample script used: call command line applications like predict --help or train --help.

What's the expected result?

  1. Calling predict/train --help should print a list of arguments, descriptions of the arguments and a description of the command line application.
  2. We get informative docs when we call --help such that users can easily navigate CLI.

    What's the actual result?

  3. Calling the above commands result in: TypeError: __init__() got an unexpected keyword argument 'show_default' meaning that show_default isn't an argument we can pass to the argparse function..

  4. The arg descriptions do not match the updated README ones at #52 and as such could be confusing to end user.

    Additional details / screenshot

Should be a relatively easy fix by removing the line "show_default = True" in the argument --gpu_allow_growth for both train and predict scripts (line 145 in train, line 232 in predict).

bug docs good first issue

All 2 comments

Currently, both of the below arguments have the same 2nd option string. How would we want to best resolve this? Once we decide, all else is fixed, and I will PR changes (along with an update to the readme).

--config_path | -c , string, path to the configuraton file in training, .yaml extension.
--ckpt_path | -c , string, path to the checkpointed model, .ckpt extension.

Modify to: (?)
--config_path | -c
--ckpt_path | -ck or -k

Additionally, spotted more 'show_default' flags... so not only in --gpu_allow_growth but also:

  • --log_dir, --ckpt_path, --mode, --batch_size, --sample_label...
Was this page helpful?
0 / 5 - 0 ratings

Related issues

mathpluscode picture mathpluscode  Â·  3Comments

mathpluscode picture mathpluscode  Â·  3Comments

NMontanaBrown picture NMontanaBrown  Â·  5Comments

s-sd picture s-sd  Â·  5Comments

mathpluscode picture mathpluscode  Â·  6Comments