Validate task messages & task outputs separately #3820
Conversation
16e1803 to
4ea61fd
Compare
| elif ':' in message_str: | ||
| valid, err_msg = TaskMessageValidator.validate(message_str) | ||
| if not valid: | ||
| raise UserInputError( | ||
| f'Invalid task message "{message_str}" - {err_msg}') |
There was a problem hiding this comment.
The trouble with this is, it doesn't fail until the cylc message ... command inside [runtime][<task>]script is finally run. Not sure if there is a better way; trying to parse the contents of the script setting seems overly complicated
There was a problem hiding this comment.
Ooh, yeah, hadn't thought of that. If messages were separate config items, that would be easier, but that train left the station long ago.
There was a problem hiding this comment.
Actually, maybe this is OK. It results in job, not scheduler, failure. So it can be fixed on the fly (fix task def, reload and retrigger).
There was a problem hiding this comment.
On that basis I'm OK with this; but we'll see what @oliver-sanders thinks.
There was a problem hiding this comment.
I don't think we could really hope to do anything better with the current system.
In cylc-message, allow colons only if one is present at the end of the first word (i.e. severity level)
Not needed as each class already has doctests
4ea61fd to
3de4bb3
Compare
|
Not sure why tests timed out Re-running... |
|
...Re-timed-out Usually happens when a test is waiting for some form of signal but where no appropriate timeout was set. It's more than likely that this validation has flushed out an error in a test suite? The test should hang locally too, you should be able to see which one it is when run locally: $ CHUNK=1/4 etc/bin/run-functional-tests tests/f |
|
I should have expected that, considering that task message validation failure will stall the workflow (Cylc workflow, not GH Actions workflow...) |
| Task outputs are validated by | ||
| :py:class:`cylc.flow.unicode_rules.TaskOutputValidator`. | ||
|
|
||
| .. autoclass:: cylc.flow.unicode_rules.TaskOutputValidator |
There was a problem hiding this comment.
I think your cylc-doc PR also contains the line .. autoclass:: cylc.flow.unicode_rules.TaskOutputValidator.
I think that will attempt to document it twice?
There was a problem hiding this comment.
In that PR it's .. autoclass:: cylc.flow.unicode_rules.TaskMessageValidator
They're quite similarly named; maybe TaskMessageValidator should be renamed CylcMessageValidator?
There was a problem hiding this comment.
Ah, ok, these classes aren't user facing so don't need to worry too much about nomenclature.
| elif ':' in message_str: | ||
| valid, err_msg = TaskMessageValidator.validate(message_str) | ||
| if not valid: | ||
| raise UserInputError( | ||
| f'Invalid task message "{message_str}" - {err_msg}') |
There was a problem hiding this comment.
I don't think we could really hope to do anything better with the current system.
|
Tests passing. |
|
I'll leave the merge to you so you can decide where you want to put the |
Follow on from #3788, these changes close #3428
I didn't quite get it right in #3788. There are two different requirements for task messages and task outputs/message triggers.
cylc message -- "${CYLC_SUITE_NAME}" "${CYLC_TASK_JOB}" "WARNING: Uh-oh"):[runtime][my_task][outputs]foo = "Uh-oh"):Note: Task messages can be used for purposes other than for message triggering, so they can have colons after
SEVERITY:. However, colons cannot be used in task messaging if indeed they are used for triggering, as suite/flow validation will fail if the task outputs contain colons.Requirements check-list
CONTRIBUTING.mdand added my name as a Code Contributor.