task_pool: fix compute_runahead calculation #5833

oliver-sanders · 2023-11-21T14:01:53Z

Closes reload: reloading shakes out runahead tasks #5825
Cycles were considered active if they contained runahead limited tasks.
This could cause the runahead limit to be bumped forwards whenever the limit calculation was forced to update, e.g. on reload.
This filters out tasks at or beyond the runahead limit and straigntens out the task status checks to match Cylc 7 behaviour in compat mode.

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).
Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
Tests are included (or explain why tests are not needed).
CHANGES.md entry included if this is a change that can affect users
Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

oliver-sanders · 2023-11-21T14:02:46Z

cylc/flow/task_pool.py

@@ -362,20 +362,35 @@ def compute_runahead(self, force=False) -> bool:
            # Find the earliest point with unfinished tasks.
            for point, itasks in sorted(self.get_tasks_by_point().items()):
                if (
-                    points  # got the limit already so this point too


This could never be true as points is populated on a different branch.

No, it's populated on this branch too, within the same loop. So the logic says to automatically append the next points once we have added the first one based on the state checks. (However, I'm not sure why it wasn't just stopping at the first one, since then we just take the min anyway).

Since we're iterating in sorted order the earlier cycles should come first, so a break should make sense?

~~It's using points later in ways which don't cause test failures, so I'll restore the original to avoid breakages.~~

Actually, no, adding this line back breaks the new tests. I'm not sure why we would add cycle points with no active tasks to the points list?

oliver-sanders · 2023-11-21T14:43:32Z

cylc/flow/task_pool.py

-                        not itask.state(
-                            TASK_STATUS_FAILED,
-                            TASK_STATUS_SUCCEEDED,
-                            TASK_STATUS_EXPIRED
-                        )


This counted waiting runahead tasks.

It should be superfluous if not cylc.flow.flags.cylc7_back_compat because is_incomplete should cover it anyway.

wxtim · 2023-11-24T10:01:56Z

tests/integration/README.md

+series using the same pytest runner. This incentivises breaking up larger test
+modules.


Possible alternative

Suggested change

series using the same pytest runner. This incentivises breaking up larger test

modules.

series using the same pytest runner. Consider breaking up

large test modules to allow tests to be run in parallel.

MetRonnie · 2023-11-24T13:14:32Z

Got integration test failures

oliver-sanders · 2023-11-27T10:59:35Z

Got integration test failures

See the OP, caused by the same issue fixed in #5826

MetRonnie · 2023-11-27T17:12:50Z

Is that the correct PR? It doesn't mention closing an issue, and also is against master rather than 8.2.x

hjoliver

I think the new logic is correct, but can we just bail out of the loop once the first point is found? (given the loop is over sorted points, and we only want the min point in the end).

tests/integration/test_task_pool.py

hjoliver · 2023-11-28T05:07:58Z

cylc/flow/task_pool.py

@@ -362,20 +362,35 @@ def compute_runahead(self, force=False) -> bool:
            # Find the earliest point with unfinished tasks.
            for point, itasks in sorted(self.get_tasks_by_point().items()):
                if (
-                    points  # got the limit already so this point too


No, it's populated on this branch too, within the same loop. So the logic says to automatically append the next points once we have added the first one based on the state checks. (However, I'm not sure why it wasn't just stopping at the first one, since then we just take the min anyway).

cylc/flow/task_pool.py

oliver-sanders · 2023-11-28T09:50:20Z

Is that the correct PR? It doesn't mention closing an issue, and also is against master rather than 8.2.x

Yes, it's the right PR, the issue bring fixed was the way the time zone variable was set in the tests. Will have to pull the fix into 8.2.x.

* Closes cylc#5825 * Cycles were considered active if they contained runahead limited tasks. * This could cause the runahead limit to be bumped forwards whenever the limit calculation was forced to update, e.g. on reload. * This filters out tasks at or beyond the runahead limit and straigntens out the task status checks to match Cylc 7 behaviour in compat mode.

* Setting the TZ env var within a Python session doesn't work as intended. * We'll add a TZ offset in CI in the near future.

oliver-sanders · 2023-11-28T10:52:26Z

pytest.ini

    # a weird timezone to check that tests aren't assuming the local timezone
-    TZ=XXX-09:35
+    # Note: this doesn't work properly with pytest-xdist
+    # TZ=XXX-09:35


This prevents the new test from failing.

Setting the TZ variable in this way doesn't work as intended.

A solution will arrive in #5826

oliver-sanders · 2023-11-28T11:21:17Z

tests/f/runahead all pass, but a bunch of others fail, back to draft.

oliver-sanders · 2023-12-20T14:41:36Z

Hopefully superseded by #5893

oliver-sanders added the bug Something is wrong :( label Nov 21, 2023

oliver-sanders added this to the cylc-8.2.4 milestone Nov 21, 2023

oliver-sanders self-assigned this Nov 21, 2023

oliver-sanders commented Nov 21, 2023

View reviewed changes

oliver-sanders changed the base branch from master to 8.2.x November 21, 2023 14:50

oliver-sanders closed this Nov 21, 2023

oliver-sanders reopened this Nov 21, 2023

oliver-sanders marked this pull request as draft November 21, 2023 15:48

oliver-sanders force-pushed the compute_runahead branch from 40db21c to 299ec97 Compare November 22, 2023 15:35

oliver-sanders requested review from wxtim and MetRonnie November 22, 2023 15:37

oliver-sanders marked this pull request as ready for review November 22, 2023 15:37

wxtim reviewed Nov 24, 2023

View reviewed changes

wxtim approved these changes Nov 24, 2023

View reviewed changes

hjoliver reviewed Nov 28, 2023

View reviewed changes

oliver-sanders added 3 commits November 28, 2023 10:50

tests/i: add note about module-scoped fixtures

e0cc4cb

tests: redact TZ environment variable

94a749c

* Setting the TZ env var within a Python session doesn't work as intended. * We'll add a TZ offset in CI in the near future.

oliver-sanders force-pushed the compute_runahead branch from b332dda to 94a749c Compare November 28, 2023 10:51

oliver-sanders commented Nov 28, 2023

View reviewed changes

oliver-sanders requested a review from hjoliver November 28, 2023 10:52

MetRonnie assigned oliver-sanders and unassigned oliver-sanders Nov 28, 2023

oliver-sanders marked this pull request as draft November 28, 2023 11:21

oliver-sanders added the superseded label Jan 2, 2024

oliver-sanders removed this from the cylc-8.2.4 milestone Jan 2, 2024

oliver-sanders closed this Jan 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

task_pool: fix compute_runahead calculation #5833

task_pool: fix compute_runahead calculation #5833

oliver-sanders commented Nov 21, 2023 •

edited

Loading

oliver-sanders Nov 21, 2023

hjoliver Nov 28, 2023 •

edited

Loading

oliver-sanders Nov 28, 2023

oliver-sanders Nov 28, 2023 •

edited

Loading

oliver-sanders Nov 21, 2023

wxtim Nov 24, 2023 •

edited

Loading

MetRonnie commented Nov 24, 2023

oliver-sanders commented Nov 27, 2023

MetRonnie commented Nov 27, 2023

hjoliver left a comment

hjoliver Nov 28, 2023 •

edited

Loading

oliver-sanders commented Nov 28, 2023 •

edited

Loading

oliver-sanders Nov 28, 2023

oliver-sanders commented Nov 28, 2023

oliver-sanders commented Dec 20, 2023

		series using the same pytest runner. This incentivises breaking up larger test
		modules.

task_pool: fix compute_runahead calculation #5833

task_pool: fix compute_runahead calculation #5833

Conversation

oliver-sanders commented Nov 21, 2023 • edited Loading

oliver-sanders Nov 21, 2023

Choose a reason for hiding this comment

hjoliver Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

oliver-sanders Nov 28, 2023

Choose a reason for hiding this comment

oliver-sanders Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

oliver-sanders Nov 21, 2023

Choose a reason for hiding this comment

wxtim Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

MetRonnie commented Nov 24, 2023

oliver-sanders commented Nov 27, 2023

MetRonnie commented Nov 27, 2023

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

oliver-sanders commented Nov 28, 2023 • edited Loading

oliver-sanders Nov 28, 2023

Choose a reason for hiding this comment

oliver-sanders commented Nov 28, 2023

oliver-sanders commented Dec 20, 2023

oliver-sanders commented Nov 21, 2023 •

edited

Loading

hjoliver Nov 28, 2023 •

edited

Loading

oliver-sanders Nov 28, 2023 •

edited

Loading

wxtim Nov 24, 2023 •

edited

Loading

hjoliver Nov 28, 2023 •

edited

Loading

oliver-sanders commented Nov 28, 2023 •

edited

Loading