Fix suicide trigger & logic #2656
Conversation
Also flatten single item in "(...)" in graph.
|
(Actually, please hold review.) |
| self.remove(itask, 'suicide') | ||
| num_removed += 1 | ||
| if (itask.state.suicide_prerequisites and | ||
| itask.state.suicide_prerequisites_are_all_satisfied()): |
There was a problem hiding this comment.
Use of itask.state.suicide_prerequisites_are_all_satisfied is purposeful. This may cause issues where multiple suicide triggers are used in combination, see https://github.com/cylc/cylc/pull/2523/files#diff-9e759d26a5f403f3e52c71605f48269cR966.
There was a problem hiding this comment.
But I cannot see how you can change it to not break this case - when the prerequisite list implies the & logic.
| showdown:good => good & ! bad & ! ugly | ||
| showdown:bad => bad & ! good & ! ugly | ||
| showdown:ugly => ugly & ! good & ! bad | ||
| showdown:good => good |
There was a problem hiding this comment.
Does this still test for #2523?
There was a problem hiding this comment.
OK. The old logic implies something like showdown:good & showdown:bad => ! ugly. Need to find out what that will do!
There was a problem hiding this comment.
(Obviously, the test has been modified to test the | logic. I'll add another test to do the & logic for outputs from the same task.)
There was a problem hiding this comment.
Looking back at #2523, it would appear that it was the triangle arrangement of tasks which triggered the bug so the new 19-and-suicide.t won't test for this. If might be easier to keep the old test, as the & and | versions should be equivalent the test aught to pass using the same reference file:
{% if AND %}
showdown:good => good & ! bad & ! ugly
showdown:bad => bad & ! good & ! ugly
showdown:ugly => ugly & ! good & ! bad
{% else %}
showdown:bad => bad
showdown:ugly => ugly
showdown:good | showdown:bad => ! ugly
showdown:good | showdown:ugly => ! bad
showdown:bad | showdown:ugly => ! good
{% endif %}
good | bad | ugly => finThere was a problem hiding this comment.
If you re-factor the original graph to make it a bit easier to understand, it would look like this:
showdown:good => good
showdown:bad => bad
showdown:ugly => ugly
showdown:good & showdown:bad => ! ugly
showdown:good & showdown:ugly => ! bad
showdown:bad & showdown:ugly => ! good
It does not work, because showdown's script only send one output message at a time, so the & output logic should never satisfy the suicide triggers.
There was a problem hiding this comment.
Triangle logic no longer tested, probably not necessary.
There was a problem hiding this comment.
I have now modified test 20 to do the original graph, but with new scripts to ensure the correct behaviour for message/output. Note:
showdown:good => good & ! bad & ! ugly
showdown:bad => bad & ! good & ! ugly
showdown:ugly => ugly & ! good & ! bad
# is the same as:
showdown:good => good
showdown:good => ! bad
showdown:good => ! ugly
showdown:bad => bad
showdown:bad => ! good
showdown:bad => ! ugly
showdown:ugly => ugly
showdown:ugly => ! good
showdown:ugly => ! bad
# Rearrange:
showdown:good => good
showdown:bad => bad
showdown:ugly => ugly
showdown:good => ! ugly
showdown:bad => ! ugly
showdown:good => ! bad
showdown:ugly => ! bad
showdown:bad => ! good
showdown:ugly => ! good
# Group by RHS, we have:
showdown:good => good
showdown:bad => bad
showdown:ugly => ugly
showdown:good & showdown:bad => ! ugly
showdown:good & showdown:ugly => ! bad
showdown:bad & showdown:ugly => ! good
|
This should now be good, with enough tests to cover various cases. |
Modify test 20 to look like test 17. Comment to explain the logic. Modify the script message logic to support the correct behaviour.
Also flatten single item in "(...)" in graph. Fix #2655.