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

Implement scheduler command method arg validation. #6064

Closed
wants to merge 1 commit into from

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Apr 11, 2024

Close #6013

Validate scheduler command method args before queuing the command, and print mutation response to the terminal:

  • error if method arg validation fails
  • or print command ID if command validated and submitted

Only applied to cylc set on this PR - do follow-up work for other commands.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@hjoliver hjoliver added the could be better Not exactly a bug, but not ideal. label Apr 11, 2024
@hjoliver hjoliver added this to the cylc-8.3.0 milestone Apr 11, 2024
@hjoliver hjoliver self-assigned this Apr 11, 2024
@ColemanTom
Copy link
Contributor

Close #6064

I think you mean #6013 ? 6064 is this pull request.

@hjoliver
Copy link
Member Author

Close #6064

I think you mean #6013 ? 6064 is this pull request.

Yikes, thanks, good spotting ... corrected. A pull request that closes itself might cause GitHub to self-destruct 🤯

"""Decorate scheduler commands with a callable .validate attribute.

"""
# TODO: properly handle "Callable has no attribute validate"?
Copy link
Member

Choose a reason for hiding this comment

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

Probs easier just to remove the type hint.

cylc/flow/network/multi.py Show resolved Hide resolved
@hjoliver hjoliver requested a review from dwsutherland May 2, 2024 19:29
@hjoliver hjoliver marked this pull request as ready for review May 2, 2024 19:30
Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

LGTM - WRT tests, perhaps could use something similar to the graphql tests for integration and direct for unit (?)

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

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:
Copy link
Member

Choose a reason for hiding this comment

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

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 @validate decorator to avoid the globals() lookup entirely e.g. @validate(validators.set) rather than @validate.

from cylc.flow.exceptions import InputError


def print_response(multi_results):
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

(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")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Suggested change
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)
Copy link
Member

Choose a reason for hiding this comment

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

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"])
Copy link
Member

Choose a reason for hiding this comment

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

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

@hjoliver
Copy link
Member Author

Possibly now superseded by #6112

@oliver-sanders
Copy link
Member

If we're happy with the approach this can be superseded by #6112

@hjoliver hjoliver closed this Jun 13, 2024
@hjoliver hjoliver added superseded and removed could be better Not exactly a bug, but not ideal. labels Jun 13, 2024
@hjoliver hjoliver removed this from the 8.3.0 milestone Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scheduler command arg validation
4 participants