Conversation
jameshcorbett
left a comment
jameshcorbett
left a comment
There was a problem hiding this comment.
A few nitpicks and questions below.
|
|
||
|
|
||
| ``` | ||
| usage: psij-consol [-h] [-v] [--debug] {validate,run} ... |
There was a problem hiding this comment.
I'm not a huge fan of this name but I can't think of anything better, and we can always change it later I guess.
scripts/psij-consol.py
Outdated
| validate_parser = subparser.add_parser("validate", help='validate JobSpec file') | ||
| validate_parser.add_argument("file", help="JobSpec file") | ||
| execute_parser = subparser.add_parser("run", help='execute JobSpec file') | ||
| execute_parser.add_argument("file", help="JobSpec file") |
There was a problem hiding this comment.
I wonder if it should accept multiple files? The tricky part would then be how that interacts with -n.
There was a problem hiding this comment.
We can always make this change later though.
scripts/psij-consol.py
Outdated
| execute_parser = subparser.add_parser("run", help='execute JobSpec file') | ||
| execute_parser.add_argument("file", help="JobSpec file") | ||
| execute_parser.add_argument("-j", | ||
| "--job-executor", |
There was a problem hiding this comment.
It looks like the executor needs to be specified so I think this should be a mandatory argument and not an --optional.
There was a problem hiding this comment.
I see you added required=True, but what I meant (although I didn't explain it well, sorry) was to make it a mandatory positional argument, so that usage would look like psij-consol run slurm spec.json rather than psij-consol run spec.json -j slurm.
There was a problem hiding this comment.
Thanks for the clarification. Your suggestion makes the cli more intuitive.
scripts/psij-consol.py
Outdated
|
|
||
| print("Initializing job executor") | ||
| jex = psij.JobExecutor.get_instance(args.executor) | ||
| if not (jex and isinstance(jex, JobExecutor)): |
There was a problem hiding this comment.
Are these checks necessary? Shouldn't the loading mechanism throw a detailed exception in case of failure?
There was a problem hiding this comment.
It probably does. Changing to try - except.
jameshcorbett
left a comment
jameshcorbett
left a comment
There was a problem hiding this comment.
I would still like to see this comment addressed but it's minor. Otherwise looks good.
|
It would be nice to have some basic tests, but that can always be added in another PR. |
2 participants
closes #103