Post-SoD refactor: remove pflag, runahead pool #4103

hjoliver · 2021-03-01T01:19:22Z

This is a ~~small~~ SOD follow-up change (no specific Issue, but has been discussed variously)

E.g., partially addresses #3874:

tasks are spawned "on demand" when ready to run according to the graph. This brings runhead limiting into line with xtriggers, task hold, and queues: it's just another way to temporarily hold back tasks that are otherwise ready to run.
... we should get rid of the hidden runahead pool and unify runahead limiting with the other limiting mechanisms:

Make runahead limiting consistent with other forms of limiting (see above)
- Remove the "runahead task pool". Runahead tasks are now held in the main pool, with an is_runahead task indicator (c.f. is_held, is_queued)
- Runahead-limited tasks can now be exposed in the UI, like queued and held tasks, to show the user "this task is ready to run but is being held back by the runahead limit (done for cylc tui on this branch).
A new "hidden pool" is used to hide partially-satisfied tasks (in order to get spawn-on-demand from spawn-on-outputs)
Remove an annoying vestige of the old SOS dependency matching process
- We used to set task_events_mgr.pflag ("processing required flag") whenever a task event occurred that required a new round of dependency matching, then if the flag was set we would iterate through the task pool and do dependency matching and other things.
- Generally tidy up and make clearer the aforementioned "other things" which are now done directly without relying on the old flag
[UPDATE] restores correct force-triggering behaviour in the presence of queues (broken since spawn-on-demand) implementation: triggering an unqueued tasks queues it; triggering a queued task causes it to run.

cylc tui showing queue- and runahead-limited tasks:

Requirements 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).
Already covered by existing tests.
Appropriate change log entry included.
- (mention visible runahead tasks)
(master branch) I have opened a documentation PR at cylc/cylc-doc/pull/XXXX.
- (mention visible runahead tasks, somewhere...)
No dependency changes.

hjoliver · 2021-03-23T12:15:22Z

(Tests passing, after some rebase nightmares; I'll update the description and un-Draft this tomorrow/Wednesday)

hjoliver · 2021-04-20T04:52:45Z

(rebased)

hjoliver · 2021-04-27T05:34:11Z

Temporarily converted to Draft, pending fix of post-rebase test fails.

dwsutherland

Looks good 👍

Will test after the rebase, so will review my approval after a play.

cylc/flow/scheduler.py

cylc/flow/task_pool.py

hjoliver · 2021-05-14T05:15:00Z

(feedback addressed, except for one thing to follow up on, and conflicts).

hjoliver · 2021-05-27T04:50:58Z

(There are no conflicts on this branch right now so long as you don't select "Rebase and merge".)

MetRonnie

9/29 files viewed. Seems to be a couple of unresolved point from earlier

cylc/flow/broadcast_mgr.py

cylc/flow/scheduler.py

cylc/flow/task_pool.py

oliver-sanders

There are some quick changes that will reduce the impact of task iteration, have dumped them in a PR:

hjoliver#16 (review)

oliver-sanders · 2021-05-27T10:33:09Z

cylc/flow/task_pool.py

-        if released:
-            LOG.debug(
-                "Queue released:\n"
-                + '\n'.join(f"* {r.identity}" for r in released)
-            )


Was logging to "debug" as a bullet-point list, now logging each task individually to "info"?

This involves less iteration, since we're already iterating over the list for the preceding lines. Do you have a strong preference either for debug vs info on this? (It's debatable IMO).

oliver-sanders · 2021-05-27T10:34:42Z

cylc/flow/task_pool.py

        """
+        self.release_runahead_tasks()


Unexpected side-effect?

Sorry, I don't follow?

This is here in case there are runahead-limited tasks that could be released (because they are now below the runahead limit) at the time of stall check, in which case, we're not stalled. Unfortunately I don't recall why I thought that was necessary at this point. ... I'll go back and see if I can see why.

cylc/flow/task_proxy.py

oliver-sanders · 2021-05-27T11:32:31Z

cylc/flow/scheduler.py

+                    if x.state(TASK_STATUS_WAITING)
+                    and not x.state.is_queued


These two filters are applied to these three iterations of the task pool so there is scope to reduce the amount of iteration here without making structural changes to the pool.

Haven't put this in my PR incase there's some reason not to, tasks changing state between each iteration? High memory watermark?

No reason that I can think of. There shouldn't be state changes between each iteration. I've extensively refactored this bit along the lines you've suggested. See what you think.

task iteration improvements

hjoliver · 2021-05-28T01:14:05Z

cylc/flow/scheduler.py

@@ -672,8 +666,7 @@ def _load_pool_from_point(self):
            parent_points = tdef.get_parent_points(point)
            if not parent_points or all(
                    x < self.config.start_point for x in parent_points):
-                initial_tasks.append(TaskID.get(tdef.name, point))


(Not used anywhere).

hjoliver · 2021-05-28T06:48:16Z

Apologies reviewers! In addition to my several embarrassing crimes again good iterative form (hopefully I would have found the worst of those myself if I had reviewed my own code properly!) ... there were some TODOs indicating unfinished work on this branch, which I had neglected to come back to. I have now done that, plus addressed all review comments, plus this:

restored correct force-triggering behaviour in the presence of queues (broken since spawn-on-demand) implementation: triggering an unqueued task should queue it; triggering a queued task should cause it to run.

Hopefully good to go now 🤞

It seems we must not have tests for force-triggering with queues - punting that to #4234 as I've run out of time today.

MetRonnie · 2021-05-28T11:53:07Z

cylc/flow/task_pool.py

+        for itask in (
+            itask
+            for cycle in self.main_pool.values()
+            for itask in cycle.values()
+            if itask.state.is_runahead
+            if itask.state(
+                TASK_STATUS_FAILED,
+                TASK_STATUS_SUCCEEDED,
+                TASK_STATUS_EXPIRED
+            )
+            or itask.is_manual_submit
+        ):


This is not very readable in my opinion. Don't know if you want to address it now or in a later follow-up

I used to agree, although I've pretty much got my eye in for these constructs now. In any case, you can blame @oliver-sanders ' side-PR for this one! Oliver, why exactly do you prefer this kind of thing over nested loops with conditional blocks in them? I doubt that comprehensions are any more efficient unless they only involve basic types.

(However, the variable cycle in this one is badly named, I'll fix that).

I used to agree

Ditto, however, once I got used to comprehensions I changed my mind (a short stint of Erlang is an effective conversion, there is no alternative!), I think they are more readable and the filtering is much clearer (especially than the if <cond>: continue pattern (negative logic etc).

cylc/flow/task_pool.py

hjoliver · 2021-06-01T03:45:43Z

Three approvals should do it!

hjoliver added this to the cylc-8.0b0 milestone Mar 1, 2021

hjoliver self-assigned this Mar 1, 2021

hjoliver force-pushed the pflag-refactor branch from cfaaeab to b1ceb8c Compare March 1, 2021 05:12

MetRonnie mentioned this pull request Mar 1, 2021

cylc pause #4076

Merged

7 tasks

hjoliver mentioned this pull request Mar 18, 2021

triggering tasks beyond the scheduler window #4131

Closed

hjoliver force-pushed the pflag-refactor branch 2 times, most recently from d0f978e to e45eacc Compare March 23, 2021 02:12

hjoliver changed the title ~~Post-SoD refactor of task queuing.~~ Post-SoD refactor: remove pflag, runahead pool Mar 23, 2021

hjoliver modified the milestones: cylc-8.0b0, cylc-8.0b1 Mar 23, 2021

hjoliver added the sod-follow-up label Mar 23, 2021

hjoliver marked this pull request as ready for review March 23, 2021 22:00

hjoliver force-pushed the pflag-refactor branch from 5781f3d to bb366ef Compare March 24, 2021 05:03

hjoliver modified the milestones: cylc-8.0b1, cylc-8.0b2 Apr 15, 2021

hjoliver force-pushed the pflag-refactor branch from bb366ef to f234234 Compare April 20, 2021 04:52

hjoliver added 8 commits April 27, 2021 17:22

Post SOD refactor of task queueing etc.

84e360f

Fix imports.

c748603

Fix hold-release.

c0c605f

tweaks

3912eaf

more tweaks

b7fb0af

Update docstrings, type hints.

f570b0d

Follow-up cylc#4139

c0ab9fa

Final tidy.

a4e7b04

hjoliver force-pushed the pflag-refactor branch from f234234 to a4e7b04 Compare April 27, 2021 05:33

hjoliver marked this pull request as draft April 27, 2021 05:33

hjoliver marked this pull request as ready for review April 28, 2021 04:44

dwsutherland approved these changes May 4, 2021

View reviewed changes

cylc/flow/scheduler.py Outdated Show resolved Hide resolved

cylc/flow/task_pool.py Outdated Show resolved Hide resolved

cylc/flow/task_pool.py Show resolved Hide resolved

hjoliver added 2 commits May 27, 2021 13:39

Merge branch 'master' into pflag-refactor

40f135f

Lint fix.

5a9ddfe

MetRonnie reviewed May 27, 2021

View reviewed changes

task iteration improvements

c292114

oliver-sanders reviewed May 27, 2021

View reviewed changes

Merge pull request #16 from oliver-sanders/pflag

c85155b

task iteration improvements

hjoliver commented May 28, 2021

View reviewed changes

Address (and tweak) code review.

e25f037

hjoliver force-pushed the pflag-refactor branch 4 times, most recently from 0f42c2b to 8281420 Compare May 28, 2021 04:40

Ext triggering main loop tidy.

44e6d0b

hjoliver force-pushed the pflag-refactor branch from 8281420 to 44e6d0b Compare May 28, 2021 04:44

hjoliver added 2 commits May 28, 2021 17:43

Simplify runahead comp a bit.

dbc0190

Restore correct forced triggering.

fea2604

hjoliver force-pushed the pflag-refactor branch from fffc316 to fea2604 Compare May 28, 2021 06:41

Update change log.

7ce2f11

hjoliver mentioned this pull request May 28, 2021

Tests needed for forced triggering with queues #4234

Open

MetRonnie reviewed May 28, 2021

View reviewed changes

MetRonnie approved these changes May 28, 2021

View reviewed changes

Final tidy

00ebe7c

hjoliver merged commit bdb51e2 into cylc:master Jun 1, 2021

hjoliver deleted the pflag-refactor branch June 1, 2021 03:45

hjoliver mentioned this pull request Jun 3, 2021

Allow cylc hold to hold future tasks that haven't spawned yet #4238

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Post-SoD refactor: remove pflag, runahead pool #4103

Post-SoD refactor: remove pflag, runahead pool #4103

hjoliver commented Mar 1, 2021 •

edited

Loading

hjoliver commented Mar 23, 2021

hjoliver commented Apr 20, 2021

hjoliver commented Apr 27, 2021

dwsutherland left a comment •

edited

Loading

hjoliver commented May 14, 2021

hjoliver commented May 27, 2021

MetRonnie left a comment

oliver-sanders left a comment

oliver-sanders May 27, 2021

hjoliver May 27, 2021

oliver-sanders May 27, 2021

hjoliver May 27, 2021

oliver-sanders May 27, 2021 •

edited

Loading

hjoliver May 28, 2021

hjoliver May 28, 2021

hjoliver commented May 28, 2021 •

edited

Loading

MetRonnie May 28, 2021

hjoliver Jun 1, 2021 •

edited

Loading

hjoliver Jun 1, 2021

oliver-sanders Jun 2, 2021 •

edited

Loading

hjoliver commented Jun 1, 2021

Post-SoD refactor: remove pflag, runahead pool #4103

Post-SoD refactor: remove pflag, runahead pool #4103

Conversation

hjoliver commented Mar 1, 2021 • edited Loading

hjoliver commented Mar 23, 2021

hjoliver commented Apr 20, 2021

hjoliver commented Apr 27, 2021

dwsutherland left a comment • edited Loading

Choose a reason for hiding this comment

hjoliver commented May 14, 2021

hjoliver commented May 27, 2021

MetRonnie left a comment

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjoliver commented May 28, 2021 • edited Loading

Choose a reason for hiding this comment

hjoliver Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

hjoliver commented Jun 1, 2021

hjoliver commented Mar 1, 2021 •

edited

Loading

dwsutherland left a comment •

edited

Loading

oliver-sanders May 27, 2021 •

edited

Loading

hjoliver commented May 28, 2021 •

edited

Loading

hjoliver Jun 1, 2021 •

edited

Loading

oliver-sanders Jun 2, 2021 •

edited

Loading