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

Error-handling for missing or incorrectly specified script arguments #111

Closed
anamileva opened this issue Aug 17, 2019 · 7 comments · May be fixed by #1095
Closed

Error-handling for missing or incorrectly specified script arguments #111

anamileva opened this issue Aug 17, 2019 · 7 comments · May be fixed by #1095
Assignees
Labels
type: enhancement improvements to existing functionality

Comments

@anamileva
Copy link
Member

Add some error-handling for when the user does not specify all required script arguments, as the traceback can be rather unhelpful.

@anamileva anamileva added the type: enhancement improvements to existing functionality label Aug 17, 2019
@anamileva anamileva self-assigned this Aug 17, 2019
@gerritdm
Copy link
Contributor

This definitely applies to the viz plot scripts. Right now the error will propagate to the SQL command and you get an unhelpful sqlite error. We should catch this earlier and give a clear warning to the user that tells what argument is missing where.

@gerritdm
Copy link
Contributor

Sort of related, but I also wonder whether we should replace parse_known_args with parse_args to avoid argument typos going through unnoticed.
https://stackoverflow.com/questions/12818146/python-argparse-ignore-unrecognised-arguments

@gerritdm
Copy link
Contributor

This discussion has some good suggestions on how to do this:
https://stackoverflow.com/questions/24180527/argparse-required-arguments-listed-under-optional-arguments

@anamileva
Copy link
Member Author

anamileva commented Aug 24, 2019

Sort of related, but I also wonder whether we should replace parse_known_args with parse_args to avoid argument typos going through unnoticed.

I considered that, but it's a bit of an issue with run_start_to_end, as the scripts it calls don't share the arguments. We could get around that by passing individual arguments -- not the whole object -- to the series of scripts from run_start_to_end. Full circle back to our discussion from yesterday.

@anamileva anamileva added this to the Public Release milestone Sep 8, 2019
@gerritdm
Copy link
Contributor

gerritdm commented Oct 8, 2019

I've been running into some of this again with the plots.

A few thoughts:

  1. Regarding parse_args vs. parse_known_args: I agree the sub-scripts (run_scenario, process_results, ...) being called in run_end_to_end.py need to use parse_known_args, but everywhere else (run_end_to_end.py, ...) we should be able to use parse_args instead of parse_known_args.
  2. I think we should use the parent parser construct to set up a parser once for common arguments such as database, scenario_id, scenario and scenario_location (rather than repeating all that code in every sub-script). The viz suite has an example of how this is done.
  3. We can make argument requirements/types/etc. a little more explicit when setting up the ArgumentParser objects by also specifying the type, choices (if applicable), a 'required', and more for each argument. Note that the documentation does not recommend using "--" type keywords for required arguments since the "--" flag is generally assumed to denote optional arguments.
  4. Related to (3), I think we should add the default for scenario_location explicitly when setting up the parser, rather than assigning it in gridpath.common_functions.determine_scenario_directory
  5. We should handle the case of non-existing scenarios or scenario_ids better (right now it propagates to the SQL statement in auxiliary.auxiliary.get_scenario_id_and_name(), where you query the scenario table but don't get any matches so the fetchone()[0] statement doesn't work.

@gerritdm
Copy link
Contributor

One easy improvement would be to update get_scenario_id_and_name in auxiliary.auxiliary to throw a meaningful error when there is no match on the scenario name or ID. Right now it tries to fetch the results (which are "None") using .fetchone()[0] and you get the following unhelpful error message:
TypeError: 'NoneType' object is not subscriptable

A simple if query results == None raise IOError("Scenario name not found in database") or something similar would be helpful

gerritdm added a commit that referenced this issue Jan 8, 2020
This way the --help message will be more clear and won't list the
required arguments under optional arguments, which is the default
for argument flags (--arg or -a), whether they are required or not.

Note: to show the required arguments before the optional arguments
we rely on a hacky fixed that accesses and changes a private attributes 
of the parser object (it reverses the list order). 

This addresses some of the issues raised in #111.
@gerritdm
Copy link
Contributor

gerritdm commented Apr 6, 2020

Consolidated in #6. When we address this, we can re-open it and have the PR that address it close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement improvements to existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants