Skip to content

Validate workflow and task handler/mail event names #6838

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

Merged
merged 4 commits into from
Jul 22, 2025

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Jul 4, 2025

Closes #2756

The names set in these settings are now validated:

  • [scheduler][events]handler events
  • [scheduler][events]mail events
  • [runtime][X][events]handler events
  • [runtime][X][events]mail events

Any invalid event names will raise an IllegalValueError or WorkflowConfigError.

If there are any outdated Cylc 7 [scheduler][events]handler/mail events they are automatically upgraded.

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).
  • No dependency changes
  • Tests are included (or explain why tests are not needed).
  • Changelog entry included if this is a change that can affect users
  • Amend workflow events docs cylc-doc#840
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@MetRonnie MetRonnie added this to the 8.4.4 milestone Jul 4, 2025
@MetRonnie MetRonnie self-assigned this Jul 4, 2025
@MetRonnie MetRonnie added the bug Something is wrong :( label Jul 4, 2025
@MetRonnie MetRonnie marked this pull request as draft July 4, 2025 16:41
@MetRonnie MetRonnie force-pushed the events-validation branch from b008b4d to 0e42c66 Compare July 7, 2025 12:04
@MetRonnie MetRonnie linked an issue Jul 7, 2025 that may be closed by this pull request
@MetRonnie MetRonnie force-pushed the events-validation branch from 0e42c66 to 2c88a81 Compare July 7, 2025 15:38
@MetRonnie MetRonnie changed the title Validate and upgrade workflow handler events Validate workflow and task handler events Jul 7, 2025
@MetRonnie MetRonnie force-pushed the events-validation branch from 2c88a81 to 4d88c5f Compare July 7, 2025 16:04
@MetRonnie MetRonnie marked this pull request as ready for review July 7, 2025 16:04
@MetRonnie MetRonnie requested review from oliver-sanders and wxtim July 7, 2025 16:04
@MetRonnie MetRonnie force-pushed the events-validation branch from 4d88c5f to 428420e Compare July 16, 2025 11:24
@oliver-sanders
Copy link
Member

oliver-sanders commented Jul 18, 2025

Closes #2756

Wow, 2018 issue!

Maybe a matter for another PR, but I think mail events aren't handled either:

$ cat global.cylc 
[scheduler]
  [[events]]
    mail events = elephant
$ CYLC_CONF_PATH=$PWD cylc config
[scheduler]
    [[events]]
        mail events = elephant

Both [scheduler][events]handler events and [runtime][X][events]handler events names are now validated.

Parsec has an options parameter which allows us to define valid options, e.g:

'method', VDR.V_STRING, 'name',
options=['name', 'address', 'hardwired'],
desc=f'''

This provides validation, but also gets pulled through into the config when documented:

https://cylc.github.io/cylc-doc/stable/html/reference/config/global.html#global.cylc[scheduler][host%20self-identification]method

I don't know if this supports VDR.V_STRING_LIST configs like this one where multiple options can be picked, but it would be great if it did as we would have a single-source-of-truth and all legit values would get automatically pulled through into the docs.


Can you not just use the parsec.upgrader.deprecate logic

I think parsec only offers deprecate and obsolete functions. We don't want to deprecate / obsolete the handler events config, only particular values of it.

The deprecate thinggy accepts a function that can be used to upgrade values from one format to another which is what we want, but I think we would need a new in-situ upgrade function for parsec for this use case.

We could develop this functionality in parsec, it might come in handy again and the closer the functionality is to parsec, the closer it is to the docs. But at the end of the day, meh, whatever works!

@oliver-sanders oliver-sanders modified the milestones: 8.4.4, 8.4.x Jul 18, 2025
@MetRonnie MetRonnie changed the title Validate workflow and task handler events Validate workflow and task handler/mail event names Jul 21, 2025
@MetRonnie
Copy link
Member Author

MetRonnie commented Jul 21, 2025

image

I have moved validation of workflow event names to parsec. However, this is not possible for task event names as any custom output is a valid task event name.

@MetRonnie MetRonnie requested a review from wxtim July 21, 2025 11:51
@MetRonnie MetRonnie force-pushed the events-validation branch from 6934dfb to 0b1057e Compare July 21, 2025 11:59
Moved validation of workflow event names to parsec
EVENT_SUCCEEDED = TASK_OUTPUT_SUCCEEDED
NON_UNIQUE_EVENTS = ('warning', 'critical', 'custom')
STD_EVENTS = [
Copy link
Member

@oliver-sanders oliver-sanders Jul 22, 2025

Choose a reason for hiding this comment

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

Even if we can't validate these at the parsec level, it would be great to document them there.

I think something like this will make list the options in the same format the parsec autodoc would:

With conf('task events', desc = f'''
    :Options: ``{"``, ``".join(STD_EVENTS)}}``, any custom event

    Configure the task event handling system.
''')

Punt to issue if short on time.

Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +2730 to +2749
def _upg_wflow_event_names(self) -> None:
"""Upgrade any Cylc 7 workflow handler/mail events names."""
for setting in ('handler events', 'mail events'):
event_names: Optional[List[str]] = self.cfg['scheduler']['events'][
setting
]
if not event_names:
continue
upgraded: Dict[str, str] = {}
for i, event in enumerate(event_names):
if event in WorkflowEventHandler.EVENTS_DEPRECATED:
event_names[i] = upgraded[event] = (
WorkflowEventHandler.EVENTS_DEPRECATED[event]
)
if upgraded and not cylc.flow.flags.cylc7_back_compat:
LOG.warning(
f"{upgrader.DEPR_MSG}\n"
f" * (8.0.0) [scheduler][events][{setting}] "
+ ', '.join(f'{k} -> {v}' for k, v in upgraded.items())
)
Copy link
Member

Choose a reason for hiding this comment

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

[minor] We could potentially make depr_options a dict of {deprecated_opt: replacement} and handle this in the parsec layer if we thought this functionality would be reused elsewhere.

@oliver-sanders
Copy link
Member

oliver-sanders commented Jul 22, 2025

Looks good, this is a huge improvement in the docs! Thanks for looking into the parsec changes.

@wxtim wxtim merged commit a7988c0 into cylc:8.4.x Jul 22, 2025
28 checks passed
@MetRonnie MetRonnie deleted the events-validation branch July 22, 2025 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

handler events list not validated
3 participants