Implement scheduler command method arg validation. #6064
Conversation
hjoliver
force-pushed
the
cmd-arg-validation
branch
from
74e7f30
to
a6269bc
Compare
hjoliver
force-pushed
the
cmd-arg-validation
branch
from
a6269bc
to
1b8c7d2
Compare
| """Decorate scheduler commands with a callable .validate attribute. | ||
|
|
||
| """ | ||
| # TODO: properly handle "Callable has no attribute validate"? |
There was a problem hiding this comment.
Probs easier just to remove the type hint.
There was a problem hiding this comment.
I don't think these validators have been hooked into the CLI correctly, invalid cylc set commands seem to be getting through?
$ cylc set foo//1/a --pre=all,2/foo:baz
~osanders/foo: command ef0628db-eb45-490f-9b1e-1d28259e600a queued
# log/scheduler/log
INFO - Command "set" actioned. ID=ef0628db-eb45-490f-9b1e-1d28259e600a
| return func | ||
|
|
||
|
|
||
| def validate_flow_opts(flows: List[str], flow_wait: bool) -> None: |
There was a problem hiding this comment.
Because the validate function is looking for validators in this module with the prefix validate_ these utility functions (e.g. validate_flow_opts) are easily muddled with the command validators which use them (e.g. validate_set). It would be a good to prise these apart to avoid confusion and the possibility of accidental bypassing of validation. Suggestions:
- Prefix the utility functions with an underscore to mark them as private to this module (makes sense if they have no valid use outside).
- Move them into their own module(s) (if they have valid use outside of this module).
- Hardwire the validation function in the
@validatedecorator to avoid theglobals()lookup entirely e.g.@validate(validators.set)rather than@validate.
| from cylc.flow.exceptions import InputError | ||
|
|
||
|
|
||
| def print_response(multi_results): |
There was a problem hiding this comment.
Suggest making the reporter an option to call_multi_async so that this can be called in place of _report_multi so that this is happening within the "call_multi" framework rather than outside of it. I.E. this should replace _report_multi rather than running alongside it. Also a good idea as this would remove the top-level loop for multi_result in multi_results allowing for asynchronous reporting of outcomes for different workflows in the future.
Outline of future ambitions for reference: #4778 (actually quite easy, but low priority)
| @@ -2146,6 +2147,7 @@ def command_force_trigger_tasks( | |||
| return self.pool.force_trigger_tasks( | |||
| tasks, flow, flow_wait, flow_descr) | |||
|
|
|||
| @command_validation.validate | |||
There was a problem hiding this comment.
(in relation to earlier comment)
E.G, this could be:
@validate(command_validation.set)
def command_set(| # Validation failure | ||
| raise InputError(response[1]) | ||
| else: | ||
| print(f"{wf_id}: command {response[1]} queued") |
There was a problem hiding this comment.
Should we be logging the command ID at the info level?
This internally generated string isn't really user facing, it's not useful information to them.
| print(f"{wf_id}: command {response[1]} queued") | |
| print(f"{wf_id}: command queued") | |
| LOG.debug(f'command-id: {response[1]}') |
| >>> validate_flow_opts(["1", "2"], True) | ||
|
|
||
| Bad: | ||
| >>> validate_flow_opts(["none", "1"], False) |
There was a problem hiding this comment.
Validation seems to work for cylc trigger but not cylc set?
$ cylc set foo//1/a --flow=none,1
~osanders/foo: command e1b50751-da49-4588-9fe3-91e3ad2e7bbd queued
$ cylc trigger foo//1/a --flow=none,1
InputError: Flow values must be an integer, or 'all', 'new', or 'none'
| cylc.flow.exceptions.InputError: ... | ||
|
|
||
| # Error: "all" must be used alone: | ||
| >>> validate_prereqs(["all", "2/foo:baz"]) |
There was a problem hiding this comment.
Doesn't seem to be hooked into cylc set:
$ cylc set foo//1/a --pre=all,2/foo:baz
~osanders/foo: command 5b016bdf-7e26-4073-81b1-dda1c23c9e7c queued
|
Possibly now superseded by #6112 |
|
If we're happy with the approach this can be superseded by #6112 |
hjoliver
added
superseded
and removed
could be better
Close #6013
Validate scheduler command method args before queuing the command, and print mutation response to the terminal:
Only applied to
cylc seton this PR - do follow-up work for other commands.Check List
CONTRIBUTING.mdand added my name as a Code Contributor.setup.cfg(andconda-environment.ymlif present).CHANGES.mdentry included if this is a change that can affect users?.?.xbranch.