Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify the help on incorrect positional arg #75

Merged
merged 5 commits into from
Dec 18, 2023

Conversation

jeremyfowers
Copy link
Collaborator

@jeremyfowers jeremyfowers commented Dec 15, 2023

This is a bug fix PR.

Closes #72 by simplifying the error processing code. See #72 for an example of how thev overloaded error message would always look before.

Before: on generic errors, provide help that was relevant to both invalid command and invalid file

Now: on generic errors, let argparse decide an appropriate and specific error message for commands and files, respectively.

Demos

Invalid Command (Close to real command)

(tkml) jefowers@XSJ-JEFOWERS-L1:~/turnkeyml/models/selftest$ turnkey banchmerk linear.py

Error: Unexpected positional argument `turnkey banchmerk`. Did you mean `turnkey benchmark`?

▄██████████████▄▐█▄▄▄▄█▌
██████▌▄▌▄▐▐▌███▌▀▀██▀▀
████▄█▌▄▌▄▐▐▌▀███▄▄█▌
▄▄▄▄▄██████████████


Traceback (most recent call last):
  File "/home/jefowers/miniconda3/envs/tkml/bin/turnkey", line 8, in <module>
    sys.exit(turnkeycli())
  File "/home/jefowers/turnkeyml/src/turnkeyml/cli/cli.py", line 490, in main
    raise exceptions.ArgError(
turnkeyml.common.exceptions.ArgError: Unexpected positional argument `turnkey banchmerk`. Did you mean `turnkey benchmark`?

Invalid Command (Not Close)

(tkml) jefowers@XSJ-JEFOWERS-L1:~/turnkeyml/models/selftest$ turnkey build linear.py
error: argument COMMAND: invalid choice: 'build' (choose from 'benchmark', 'cache', 'models', 'version')
usage: turnkey [-h] COMMAND ...

TurnkeyML benchmarking command line interface

optional arguments:
  -h, --help  show this help message and exit

command:
  COMMAND     Choose one of the following commands:
    benchmark
              Benchmark the performance of one or more models
    cache     Commands for managing the build cache
    models    Commands for managing the models
    version   Print the package version number

Invalid File Input (Explicit benchmark command)

(tkml) jefowers@XSJ-JEFOWERS-L1:~/turnkeyml/models/selftest$ turnkey benchmark lanier.py
Error: ['lanier.py'] do not exist, please verify if the file(s) exists.

▄██████████████▄▐█▄▄▄▄█▌
██████▌▄▌▄▐▐▌███▌▀▀██▀▀
████▄█▌▄▌▄▐▐▌▀███▄▄█▌
▄▄▄▄▄██████████████


Traceback (most recent call last):
  File "/home/jefowers/miniconda3/envs/tkml/bin/turnkey", line 8, in <module>
    sys.exit(turnkeycli())
  File "/home/jefowers/turnkeyml/src/turnkeyml/cli/cli.py", line 500, in main
    args.func(args)
  File "/home/jefowers/turnkeyml/src/turnkeyml/cli/cli.py", line 64, in benchmark_command
    benchmark_files(**api_args)
  File "/home/jefowers/turnkeyml/src/turnkeyml/files_api.py", line 133, in benchmark_files
    raise exceptions.ArgError(
turnkeyml.common.exceptions.ArgError: ['lanier.py'] do not exist, please verify if the file(s) exists.

Invalid File Input (Implied benchmark command)

Same as the prior demo (as it should be)

(tkml) jefowers@XSJ-JEFOWERS-L1:~/turnkeyml/models/selftest$ turnkey lanier.py
Error: ['lanier.py'] do not exist, please verify if the file(s) exists.

▄██████████████▄▐█▄▄▄▄█▌
██████▌▄▌▄▐▐▌███▌▀▀██▀▀
████▄█▌▄▌▄▐▐▌▀███▄▄█▌
▄▄▄▄▄██████████████


Traceback (most recent call last):
  File "/home/jefowers/miniconda3/envs/tkml/bin/turnkey", line 8, in <module>
    sys.exit(turnkeycli())
  File "/home/jefowers/turnkeyml/src/turnkeyml/cli/cli.py", line 500, in main
    args.func(args)
  File "/home/jefowers/turnkeyml/src/turnkeyml/cli/cli.py", line 64, in benchmark_command
    benchmark_files(**api_args)
  File "/home/jefowers/turnkeyml/src/turnkeyml/files_api.py", line 133, in benchmark_files
    raise exceptions.ArgError(
turnkeyml.common.exceptions.ArgError: ['lanier.py'] do not exist, please verify if the file(s) exists.

Invalid File Extension

(tkml) jefowers@XSJ-JEFOWERS-L1:~/turnkeyml/models/selftest$ turnkey benchmark bob.po

Before: would throw a fail whale and a stack trace.

Now: stardard argparse exit:

error: input_files must end with .py, .onnx, or .txt (got 'bob.po')

usage: turnkey benchmark [-h] [--use-slurm | --process-isolation]
                         [--lean-cache] [-d CACHE_DIR]
                         [--labels [LABELS [LABELS ...]]]
                         [--sequence {optimize-fp16,optimize-fp32,onnx-fp32}]
                         [--rebuild {if_needed,always,never}]
                         [--device {nvidia,x86}]
                         [--runtime {ort,trt,torch-eager,torch-compiled}]
                         [--iterations ITERATIONS] [--analyze-only] [-b]
                         [--script-args SCRIPT_ARGS] [--max-depth MAX_DEPTH]
                         [--onnx-opset ONNX_OPSET] [--timeout TIMEOUT]
                         [--rt-args [RT_ARGS [RT_ARGS ...]]]
                         input_files [input_files ...]

@jeremyfowers jeremyfowers added the bug Something isn't working label Dec 15, 2023
@jeremyfowers jeremyfowers self-assigned this Dec 15, 2023
@jeremyfowers jeremyfowers linked an issue Dec 15, 2023 that may be closed by this pull request
Signed-off-by: Jeremy <jeremy.fowers@amd.com>
Signed-off-by: Jeremy <jeremy.fowers@amd.com>
Signed-off-by: Jeremy <jeremy.fowers@amd.com>
@jeremyfowers jeremyfowers force-pushed the 72-confusing-error-message-on-incorrect-command branch from 032398d to 8e7eb59 Compare December 18, 2023 16:53
Signed-off-by: Jeremy <jeremy.fowers@amd.com>
@jeremyfowers jeremyfowers marked this pull request as ready for review December 18, 2023 17:42
Copy link
Collaborator

@ramkrishna2910 ramkrishna2910 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Comments really helped reviewing :)

Copy link
Collaborator

@danielholanda danielholanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and it is looking good, although the stardard argparse exit is still hard to find when such a long help message is printed (assuming this is out of scope for now).

@jeremyfowers jeremyfowers merged commit 21eacec into main Dec 18, 2023
10 checks passed
@jeremyfowers jeremyfowers deleted the 72-confusing-error-message-on-incorrect-command branch December 18, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing error message on incorrect command
3 participants