Use OrderedDict for queues to make them FIFO

[TomekTrzeciak]

Partial fix for #1196.

[hjoliver]

@matthewrmshin - this change seems sensible (and satisfyingly simple!) - is at good for review now as far as you're concerned (@TomekTrzeciak is one of your colleagues, no?)

[matthewrmshin]

Yes and yes.

[hjoliver]

This change doesn't have the desired effect! Here's a test suite that contrives to adds tasks proc_1..5 to the 'q1' queue in order, before any are released to run:

[cylc]
    [[parameters]]
        n = 1..5
        m = 1..1
[scheduling]
    [[queues]]
        [[[q1]]]
            limit = 1
            members = proc<n>, hold<m>
    [[dependencies]]
        graph = """
          delay<n> => proc<n>
          hold<m>
          """
[runtime]
    [[delay<n>]]
        script = sleep $(( CYLC_TASK_PARAM_n * 2 ))
    [[proc<n>]]
        script = sleep 4
    [[hold<m>]]
        script = sleep 12

On running it, proc<n> are queued in order 1, 2, 3, 4, 5, but are released to run in order 4, 5, 3, 1, 2.

[hjoliver]

I suspect you thought (naturally enough) that self.queues[qname] in task_pool.py holds all task proxies that are currently queued in queue 'qname'. However, it doesn't - it holds all current task proxies that are members of queue 'qname'. The particular members of the queue that are actually "queued" have to be identified by their task status ( which would be ... "queued").

This is - I think - an almost minimal implementation to achieve the current random release behaviour. To get ordered/prioritized release, we'll need a slightly more sophisticated queue model (perhaps involving actual "queue" data structures!).

[hjoliver]

(given the small size of this change - unless my analysis is way off base - might as well close the PR and revisit later with a new queue implementation)

[TomekTrzeciak]

I believe I fixed the issue in my latest commit (proc<n> in your example runs in the same order as it it queued). The fix comes down to popping the task from the queue dict and immediately reinserting it back into it, effectively moving it to the back of the queue. The rest is just a bit of code refactoring. Not sure about performance implications of this change, though (depends on how expensive is key insertion/deletion in OrderedDict compared to the dictionary traversal).

[hjoliver]

Nice, good job. I wonder if it might be more elegant to separate the (now) two functionalities of self.queues, and use a deque for the FIFO bit?

[TomekTrzeciak]

I had a similar thought about using deque, but that would require more substantial redesign of the queues.

I've added two more commits: one to fix loss of queue ordering on reload (queues were reinitialised as regular dicts) and the second one to make runahead pool into FIFO too, so that tasks are migrated to queues is the spawn order (I think this might matter for queues without limits, like the default one, but I haven't test it).

[hjoliver]

Nice. This is certainly an improvement. It even does the right thing if I turn the example above into a cycling suite:

➤ cylc cat-log tomek | egrep 'proc_n.*submitted at'
2018-03-09T09:36:12+13 INFO - [proc_n1.1] -(current:ready) submitted at 2018-03-09T09:36:11+13
2018-03-09T09:36:19+13 INFO - [proc_n2.1] -(current:ready) submitted at 2018-03-09T09:36:18+13
2018-03-09T09:36:25+13 INFO - [proc_n3.1] -(current:ready) submitted at 2018-03-09T09:36:24+13
2018-03-09T09:36:31+13 INFO - [proc_n4.1] -(current:ready) submitted at 2018-03-09T09:36:30+13
2018-03-09T09:36:37+13 INFO - [proc_n5.1] -(current:ready) submitted at 2018-03-09T09:36:36+13
2018-03-09T09:36:57+13 INFO - [proc_n1.2] -(current:ready) submitted at 2018-03-09T09:36:56+13
2018-03-09T09:37:03+13 INFO - [proc_n2.2] -(current:ready) submitted at 2018-03-09T09:37:02+13
2018-03-09T09:37:10+13 INFO - [proc_n3.2] -(current:ready) submitted at 2018-03-09T09:37:09+13
2018-03-09T09:37:16+13 INFO - [proc_n4.2] -(current:ready) submitted at 2018-03-09T09:37:15+13
2018-03-09T09:37:22+13 INFO - [proc_n5.2] -(current:ready) submitted at 2018-03-09T09:37:21+13
2018-03-09T09:37:28+13 INFO - [proc_n1.3] -(current:ready) submitted at 2018-03-09T09:37:27+13
2018-03-09T09:37:34+13 INFO - [proc_n2.3] -(current:ready) submitted at 2018-03-09T09:37:33+13
2018-03-09T09:37:40+13 INFO - [proc_n3.3] -(current:ready) submitted at 2018-03-09T09:37:39+13
2018-03-09T09:37:46+13 INFO - [proc_n4.3] -(current:ready) submitted at 2018-03-09T09:37:45+13
2018-03-09T09:37:52+13 INFO - [proc_n5.3] -(current:ready) submitted at 2018-03-09T09:37:51+13

[hjoliver]

@TomekTrzeciak - to finish this off, could you:

• briefly document the new FIFO behaviour of queues in the user guide
• make a test based on the example above (or similar) to ensure that we retain FIFO queues in future
  (thanks)

[TomekTrzeciak]

Done. I found and fixed one more bug in there (and fixed a typo in the docs).

[hjoliver]

looks good now