Fix prerequisite and output manipulation on state changes. #2600

hjoliver · 2018-03-12T01:03:41Z

Address a problem first revealed in #2561 (no one had tried to use task prerequisites for external purposes until now, I guess).

Principally, at certain task state changes prerequisites were being set to all-met or all-not-met. This was at best unnecessary and at worst wrong: in conditional triggers, or manual triggering, tasks can submit without all prerequisites being met.

On this branch:

prerequisites are only unset on task state reset to 'waiting'. Otherwise they're left alone (e.g. if manually triggered, any un-met prerequisites now remain un-met).
outputs on the other hand are manipulated to stay consistent with state changes (otherwise triggering of downstream tasks would not work).

Also:

disallows manual reset to ready (this state is really a detail of internal implementation - users should use cylc trigger) and held (users should use cylc hold, which properly handles swap states)
See more detailed comments in TaskState.reset_state() and the bin/cylc-reset usage string.

Tests based on #2599 (comment)

hjoliver · 2018-03-12T01:15:32Z

A remaining problem in the same vein: on restart we now load the outputs of each task from the run DB, but we still infer the state of prerequisites from the task state (e.g. if a task with state >= "submitted" must have all prerequisites met... which is not true: it could have conditional prerequisites or have been manually triggered). So - @cylc/core - IMO we need a prerequisites table in the run DB - if agreed, I'll post a new issue for this.

matthewrmshin · 2018-03-12T09:43:56Z

I agree - as long as it does not make it harder to implement #2329 in the future.

hjoliver · 2018-03-12T19:58:16Z

Yeah, perhaps this should wait on #2329 in that case - I'll just add a note to that issue.

TomekTrzeciak · 2018-03-13T11:39:30Z

Isn't it possible to somehow restore prerequisite state based on the task outputs loaded from the DB? I worry that adding prerequisite table in DB would just create more possibilities for prerequisites and actual task states to diverge (more places in the code to keep in sync) and will likely become superfluous once #2329 gets done. Perhaps it's better to focus energy on #2329, which addresses the problem at its source.

sadielbartholomew · 2018-03-13T12:45:43Z

I have a few thoughts (which may be misguided - I've tried to get my head around the matter & related issues but with limited experience I don't claim to fully understand the technicalities):

Are manual & conditional triggers the only cases whereby tasks can be submitted without having their prerequisites satisfied? If they are, considering the general aim to keep things as 'light-weight' as possible, might it be best to only record & use prerequisites for tasks triggered as such instead of every task? We could treat tasks triggered in those ways as special cases in which a prerequisites table would need to be consulted, but otherwise know that the prerequisite state is safe to infer from the task state. Then if a user (as in Add information about prerequisites to task environment #2555) wanted to know about prerequisites that triggered specific tasks, we could add the data relevant to those tasks (only)? It seems unnecessary to record all prerequisite data when there are only distinct cases it is needed (unless it constitutes progression towards other broader/future aims).
Have all of the possible complexities been considered for a prerequisites table in the run DB? From having skimmed through the user guide, there are some dependency features which are supported that may (?) introduce complications, e.g:

future triggering & triggering off tasks yet to exist & off tasks from other suites: how to account for such prerequisite entries;
suicide triggering: lost data from 'removal' of tasks?

If I've evidently completely misunderstood something please just say; at least I can get clarification :)

hjoliver · 2018-03-14T01:13:32Z

[deleted two of my own comments after some rethinking]

hjoliver · 2018-03-14T01:24:15Z

@TomekTrzeciak -

Isn't it possible to somehow restore prerequisite state based on the task outputs loaded from the DB?

That should be possible in principle, but I'm not sure how easy it would be, and conditional prerequisites completed after a task triggered would muddy the waters somewhat (c.f. your stated goal of wanting to know which prerequisites actually caused the task to trigger). Best to revisit after #2329 I think.

hjoliver · 2018-03-14T01:30:31Z

@sadielbartholomew -

that's quite a good idea, but it's not just triggering that's the problem. There's also manual state reset - e.g. you can force a task to "succeeded" even though it never ran at all. Again, we should rethink all this in light of Improve interaction between task state, output and prerequisite #2329
and 3. these are probably not a problem, because all such prerequisites in the end reduce to a simple string with an associated boolean satisfied/not-satisfied status.

hjoliver · 2018-03-14T01:35:25Z

The current conversation on persistence or restoration of prerequisite state after a restart is kind of off-topic for this PR - we should continue if necessary under #2329.

Under the current implementation it is still worth finishing this off this PR though, to make #2561 viable.

hjoliver · 2018-03-15T09:03:26Z

[updated PR description]

hjoliver · 2018-03-15T22:55:56Z

@TomekTrzeciak - I tried to assign you as a second reviewer on this, since it's closely tied to your PR, however I think I need to invite you to be a member of the "cylc" group to do that (just done).

hjoliver · 2018-03-15T23:06:52Z

lib/cylc/task_events_mgr.py

+                if output not in [TASK_OUTPUT_EXPIRED,
+                                  TASK_OUTPUT_SUBMIT_FAILED,
+                                  TASK_OUTPUT_FAILED]:
+                    msg += "\n  " + output


Note on this branch tasks retain these "alternate standard outputs" permanently (but normally in the non-completed state). They were being added and removed on the fly, which was messy, just to avoid this log message on normal successful task completion.

This probably adds a tiny amount to the memory footprint, but it is the right thing to do IMO.

TomekTrzeciak · 2018-03-19T10:00:59Z

lib/cylc/task_pool.py

+                if status in [TASK_STATUS_FAILED,
+                              TASK_STATUS_SUBMIT_FAILED]:
+                    # TODO - HUH? SUBMIT_FAILED? WHAT ABOUT SUCCEEDED?
+                    itask.set_event_time('finished',


This looks suspicious, indeed. Needs some more explanation if this is correct.

Should probably include all states in cylc.task_state.TASK_STATUSES_FINAL.

(oops, looks like I forgot to come back to my own reminder there!)

Fixed. Turns out set_event_time merely updates the state summary times shown in the GUI - nothing to do with "events" as such. So I've changed the method name, and only update the "finished" time for succeeded and failed tasks.

TomekTrzeciak · 2018-03-19T10:19:10Z

lib/cylc/task_state.py

@@ -398,7 +390,7 @@ def _set_state(self, status):
            message += " (%s)" % self.hold_swap
        LOG.debug(message, itask=self.identity)

-    def is_greater_than(self, status):
+    def is_gt(self, status):


Why status_leq and status_geq are module functions, while this is a method?

Well, the latter concerns where self.status (of a task state object) lies in the ordered list of status strings, whereas the former concerns the relative position of two bare status strings. Some minor refactoring could probably get rid of one or the other, but I don't think it particularly matters.

hjoliver · 2018-03-22T01:12:53Z

(branch rebased)

matthewrmshin · 2018-04-11T13:27:48Z

@sadielbartholomew please sanity check this one.

sadielbartholomew · 2018-04-13T17:16:11Z

Sanity test in progress. Not related to the essence of this PR, but while conducting testing it emerged that I have a significant amount of 'bad' suites sitting in my 'cylc-run' directory (the Rose Bush migration PR being to blame) as per the comments in the two tests/registration .t files, so I have tried to string together some logic to bypass the issue. See branched PR.

sadielbartholomew

Looks good to me: sensible approach to the issue & the implementation seems sound. Please consider my referenced but essentially off-topic PR (which could be taken separately to this one if more convenient).

…r-bad-dirs Pre-empt local 'registration' test failure by 'Errno 2'

hjoliver · 2018-04-13T23:49:52Z

Merging (two approvals, and @sadielbartholomew's side-PR only affects a couple of tests).

hjoliver added this to the soon milestone Mar 12, 2018

hjoliver self-assigned this Mar 12, 2018

hjoliver mentioned this pull request Mar 12, 2018

add dependencies variable to job script #2561

Merged

hjoliver mentioned this pull request Mar 12, 2018

Improve interaction between task state, output and prerequisite #2329

Open

hjoliver force-pushed the task-state-reset-fix branch 2 times, most recently from 3311ea8 to a530b2c Compare March 15, 2018 08:58

hjoliver changed the title ~~Minimal prerequisite and output manipulation on state reset.~~ Fix prerequisite and output manipulation on state changs. Mar 15, 2018

hjoliver force-pushed the task-state-reset-fix branch 3 times, most recently from f63362f to 3260103 Compare March 15, 2018 20:52

hjoliver changed the title ~~Fix prerequisite and output manipulation on state changs.~~ Fix prerequisite and output manipulation on state changes. Mar 15, 2018

hjoliver requested a review from matthewrmshin March 15, 2018 22:42

hjoliver added the bug Something is wrong :( label Mar 15, 2018

hjoliver modified the milestones: soon, next release Mar 15, 2018

hjoliver commented Mar 15, 2018

View reviewed changes

TomekTrzeciak reviewed Mar 19, 2018

View reviewed changes

hjoliver added 2 commits March 22, 2018 14:11

Fix prerequisite and output manipulation on task state changes.

696ac85

Forced reset arg not needed.

1031d02

hjoliver added 2 commits March 22, 2018 14:11

Fixed tests.

3ecfacf

Clarify summary time updates.

35bdbe5

hjoliver force-pushed the task-state-reset-fix branch from af49813 to 35bdbe5 Compare March 22, 2018 01:11

matthewrmshin approved these changes Mar 29, 2018

View reviewed changes

matthewrmshin requested a review from sadielbartholomew April 11, 2018 13:26

hjoliver mentioned this pull request Apr 12, 2018

Kill job on task state reset from submitted or running #2621

Closed

Replace 'registration' test warnings with logic to bypass issue

fe9facd

sadielbartholomew mentioned this pull request Apr 13, 2018

Pre-empt local 'registration' test failure by 'Errno 2' hjoliver/cylc-flow#4

Merged

sadielbartholomew approved these changes Apr 13, 2018

View reviewed changes

Merge pull request #4 from sadielbartholomew/registration-tests-filte…

0d4bce8

…r-bad-dirs Pre-empt local 'registration' test failure by 'Errno 2'

hjoliver merged commit 09ca7df into cylc:master Apr 13, 2018

hjoliver deleted the task-state-reset-fix branch April 13, 2018 23:50

hjoliver mentioned this pull request Apr 14, 2018

cat-log: remote cylc sub-command #2503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix prerequisite and output manipulation on state changes. #2600

Fix prerequisite and output manipulation on state changes. #2600

hjoliver commented Mar 12, 2018 •

edited

Loading

hjoliver commented Mar 12, 2018

matthewrmshin commented Mar 12, 2018

hjoliver commented Mar 12, 2018

TomekTrzeciak commented Mar 13, 2018

sadielbartholomew commented Mar 13, 2018

hjoliver commented Mar 14, 2018 •

edited

Loading

hjoliver commented Mar 14, 2018

hjoliver commented Mar 14, 2018 •

edited

Loading

hjoliver commented Mar 14, 2018

hjoliver commented Mar 15, 2018

hjoliver commented Mar 15, 2018 •

edited

Loading

hjoliver Mar 15, 2018

hjoliver Mar 15, 2018

TomekTrzeciak Mar 19, 2018

matthewrmshin Mar 19, 2018

hjoliver Mar 19, 2018

hjoliver Mar 20, 2018

TomekTrzeciak Mar 19, 2018

hjoliver Mar 20, 2018 •

edited

Loading

hjoliver commented Mar 22, 2018

matthewrmshin commented Apr 11, 2018

sadielbartholomew commented Apr 13, 2018 •

edited

Loading

sadielbartholomew left a comment

hjoliver commented Apr 13, 2018

Fix prerequisite and output manipulation on state changes. #2600

Fix prerequisite and output manipulation on state changes. #2600

Conversation

hjoliver commented Mar 12, 2018 • edited Loading

hjoliver commented Mar 12, 2018

matthewrmshin commented Mar 12, 2018

hjoliver commented Mar 12, 2018

TomekTrzeciak commented Mar 13, 2018

sadielbartholomew commented Mar 13, 2018

hjoliver commented Mar 14, 2018 • edited Loading

hjoliver commented Mar 14, 2018

hjoliver commented Mar 14, 2018 • edited Loading

hjoliver commented Mar 14, 2018

hjoliver commented Mar 15, 2018

hjoliver commented Mar 15, 2018 • edited Loading

hjoliver Mar 15, 2018

Choose a reason for hiding this comment

hjoliver Mar 15, 2018

Choose a reason for hiding this comment

TomekTrzeciak Mar 19, 2018

Choose a reason for hiding this comment

matthewrmshin Mar 19, 2018

Choose a reason for hiding this comment

hjoliver Mar 19, 2018

Choose a reason for hiding this comment

hjoliver Mar 20, 2018

Choose a reason for hiding this comment

TomekTrzeciak Mar 19, 2018

Choose a reason for hiding this comment

hjoliver Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

hjoliver commented Mar 22, 2018

matthewrmshin commented Apr 11, 2018

sadielbartholomew commented Apr 13, 2018 • edited Loading

sadielbartholomew left a comment

Choose a reason for hiding this comment

hjoliver commented Apr 13, 2018

hjoliver commented Mar 12, 2018 •

edited

Loading

hjoliver commented Mar 14, 2018 •

edited

Loading

hjoliver commented Mar 14, 2018 •

edited

Loading

hjoliver commented Mar 15, 2018 •

edited

Loading

hjoliver Mar 20, 2018 •

edited

Loading

sadielbartholomew commented Apr 13, 2018 •

edited

Loading