Simplify and fix runahead computation. #5893

hjoliver · 2023-12-19T23:48:15Z

Fix bug revealed on the forum: the runahead limit point erroneously advances when the limit is specified as a time interval and future triggers are present.

Taking a closer look at the code, I realized it still contained nasty vestiges of the old "max active cycle points" computation, when we were (or at least thought we were) counting active points, rather than all possible points, beyond the base point. Probably my bad 😬

It's considerably simpler and more efficient on this branch.

[UPDATE] close #5825

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-12-20T13:23:54Z

LGTM.

I think this might actually close #5825.

I've transferred the test from that branch here: hjoliver#36

changes.d/5893.fix

cylc/flow/task_pool.py

tests/integration/test_task_pool.py

hjoliver · 2023-12-21T04:02:17Z

tests/integration/test_task_pool.py

+
+        if compat_mode == 'compat-mode':
+            # Cylc 7 does not count failed tasks in runahead computation.
+            assert int(str(schd.pool.runahead_limit_point)) == 5


Different from your original version of the test @oliver-sanders. Cylc 7 ignores failed tasks when computing the limit, that should include submit-failed.

that should include submit-failed

It would appear that Cylc 7 didn't include submit-failed.

E.G. In this workflow 2/foo will submit-fail:

[scheduling] max active cycle points = 3 cycling mode = integer initial cycle point = 1 [[dependencies]] [[[P1]]] graph = foo [[[R1/2]]] graph = foo[-P1] => foo [runtime] [[foo]] script = """ if [[ $CYLC_TASK_CYCLE_POINT -eq 1 ]]; then cylc broadcast "${CYLC_SUITE_NAME}" -n foo -p 2 -s '[environment]foo=$(if' sleep 5 fi """

In Cylc 7 the workflow stalls because the submit dependence prevents 3/foo from being spawned. If you insert 3/foo, the workflow will stall on the runahead limit:

Whereas on this branch the workflow will run on (so no stall):

that should include submit-failed

It would appear that Cylc 7 didn't include submit-failed.

Notice that I said it should include submit-failed - I didn't actually check that it did. So thanks for checking!

OK I'll revert that to reproduce the (incorrect) Cylc 7 behavior.

hjoliver · 2023-12-21T04:03:50Z

tests/integration/test_task_pool.py

+            assert int(str(schd.pool.runahead_limit_point)) == 4  # no change
+
+        # mark cycle 1 as complete
+        # (via task message so the task gets removed before runahead compute)


The new extra-simplified method assumes completed tasks have been removed already, which doesn't happen if you artificially call state_reset in a test.

.github/workflows/test_fast.yml

tests/integration/test_task_pool.py

wxtim

Review so far...

Wrote simple examples to check that both bugs were fixed.
Read the code.
Checked that the tests covered the two bug cases.

wxtim · 2024-01-04T13:19:18Z

tests/integration/test_task_pool.py

+                },
+            }
+        }
+        point = lambda point: IntegerPoint(str(int(point)))


Flake8:E371 is not happy with this. I'm not sure how important it is in this context, but consider...

PEP8 Reference

Suggested change

point = lambda point: IntegerPoint(str(int(point)))

def point(point): return IntegerPoint(str(int(point)))

Flake8 won't be happy with that inline function either.

IMO it's ok, especially for a test.

I'll leave it as-is then.

Flake8 won't be happy with that inline function either.

You sure? PEP8 clearly shows an example:

# Correct: def f(x): return 2*x

and flake 8 seems OK with it.

> echo 'def point(point): return 42'> foo.py > flake8 foo.py ...

Don't care, tis a test.

cylc/flow/task_pool.py

oliver-sanders · 2024-01-04T11:45:46Z

tests/integration/test_task_pool.py

+
+        if compat_mode == 'compat-mode':
+            # Cylc 7 does not count failed tasks in runahead computation.
+            assert int(str(schd.pool.runahead_limit_point)) == 5


that should include submit-failed

It would appear that Cylc 7 didn't include submit-failed.

E.G. In this workflow 2/foo will submit-fail:

[scheduling] max active cycle points = 3 cycling mode = integer initial cycle point = 1 [[dependencies]] [[[P1]]] graph = foo [[[R1/2]]] graph = foo[-P1] => foo [runtime] [[foo]] script = """ if [[ $CYLC_TASK_CYCLE_POINT -eq 1 ]]; then cylc broadcast "${CYLC_SUITE_NAME}" -n foo -p 2 -s '[environment]foo=$(if' sleep 5 fi """

In Cylc 7 the workflow stalls because the submit dependence prevents 3/foo from being spawned. If you insert 3/foo, the workflow will stall on the runahead limit:

Whereas on this branch the workflow will run on (so no stall):

oliver-sanders · 2024-01-04T17:24:02Z

tests/integration/test_task_pool.py

+                },
+            }
+        }
+        point = lambda point: IntegerPoint(str(int(point)))


Flake8 won't be happy with that inline function either.

IMO it's ok, especially for a test.

* Closes cylc#5825

Co-authored-by: Oliver Sanders <[email protected]>

In back-compat mode the cycle point time zone is assumed to be local, whereas in normal mode it is assumed to be UTC. There was contamination of the point parse caching where the time zone would carry over from tests of back-compat vs normal mode

* We were using the pytest-env plugin to run the tests in a non-UTC time zone. * The pytest-env plugin doesn't work with pytest-xdist so this was being ignored. * Also due to the way TZ support works in Python, changing the env var whilst Python is running may or may not result in changes.

Co-authored-by: Ronnie Dutta <[email protected]>

Add compat mode and not compat mode versions of the future triggers bug test.

changing task statuses in compat mode.

hjoliver · 2024-01-08T03:39:25Z

@wxtim - did you intend to push the commit "wip" 238a790 to this branch?

It isn't obviously related, and it's causing a couple of test failures.

[UPDATE] I've rebased the branch, and pushed a revert commit for your "wip" one to avoid losing your work. If not needed can rebase again to remove both.

This reverts commit 520ee29.

wxtim · 2024-01-08T08:43:46Z

@wxtim - did you intend to push the commit "wip" 238a790 to this branch?

No, that's a mistake. I think I've been stung by the behavour of gh rather than git. gh set up a local branch tracking your branch rather than a copy at github/wxtim which I'd have PR'd.

WIP is investigatory work on another ticket and you definately don't want it.

hjoliver added the bug label Dec 19, 2023

hjoliver added this to the cylc-8.2.4 milestone Dec 19, 2023

hjoliver self-assigned this Dec 19, 2023

hjoliver force-pushed the fix-runahead-compute branch from 6164367 to 5e7f4d4 Compare December 19, 2023 23:53

oliver-sanders reviewed Dec 20, 2023

View reviewed changes

changes.d/5893.fix Outdated Show resolved Hide resolved

oliver-sanders reviewed Dec 20, 2023

View reviewed changes

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

oliver-sanders reviewed Dec 20, 2023

View reviewed changes

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

oliver-sanders mentioned this pull request Dec 20, 2023

task_pool: fix compute_runahead calculation #5833

Closed

8 tasks

MetRonnie self-requested a review December 20, 2023 16:41

hjoliver commented Dec 21, 2023

View reviewed changes

tests/integration/test_task_pool.py Outdated Show resolved Hide resolved

hjoliver commented Dec 21, 2023

View reviewed changes

MetRonnie reviewed Dec 21, 2023

View reviewed changes

.github/workflows/test_fast.yml Outdated Show resolved Hide resolved

MetRonnie reviewed Dec 21, 2023

View reviewed changes

tests/integration/test_task_pool.py Outdated Show resolved Hide resolved

tests/integration/test_task_pool.py Outdated Show resolved Hide resolved

wxtim self-requested a review January 4, 2024 10:40

wxtim reviewed Jan 4, 2024

View reviewed changes

oliver-sanders reviewed Jan 4, 2024

View reviewed changes

hjoliver and others added 11 commits January 8, 2024 16:23

Simplify and fix runahead computation.

0ad5078

New runhead integration test.

04c0280

reload: test reloading doesn't nudge the runahead limit

0ca9dda

* Closes cylc#5825

Apply suggestions from code review [skip ci]

c5d9d56

Co-authored-by: Oliver Sanders <[email protected]>

Further simplification of runahead compute.

21078b1

Apply suggestions from code review

08fc17a

Co-authored-by: Ronnie Dutta <[email protected]>

Add both runahead formats to existing tests for compute runahead.

a439635

Add compat mode and not compat mode versions of the future triggers bug test.

Wrote a test to check for changes of runahead limit based on

8106408

changing task statuses in compat mode.

wip

520ee29

hjoliver force-pushed the fix-runahead-compute branch from 238a790 to d9c3ba3 Compare January 8, 2024 03:47

hjoliver added 2 commits January 8, 2024 17:15

Tweak compat-mode runahead.

6cdab75

Revert "wip"

9941fad

This reverts commit 520ee29.

hjoliver force-pushed the fix-runahead-compute branch from d9c3ba3 to 9941fad Compare January 8, 2024 04:16

oliver-sanders approved these changes Jan 8, 2024

View reviewed changes

wxtim approved these changes Jan 9, 2024

View reviewed changes

wxtim merged commit b64fcd6 into cylc:8.2.x Jan 9, 2024
32 of 36 checks passed

hjoliver deleted the fix-runahead-compute branch January 9, 2024 11:16

hjoliver mentioned this pull request Jan 9, 2024

reload: reloading shakes out runahead tasks #5825

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify and fix runahead computation. #5893

Simplify and fix runahead computation. #5893

hjoliver commented Dec 19, 2023 •

edited

Loading

oliver-sanders commented Dec 20, 2023 •

edited

Loading

hjoliver Dec 21, 2023 •

edited

Loading

oliver-sanders Jan 4, 2024

hjoliver Jan 8, 2024

hjoliver Dec 21, 2023

wxtim left a comment

wxtim Jan 4, 2024 •

edited

Loading

oliver-sanders Jan 4, 2024

hjoliver Jan 8, 2024

wxtim Jan 8, 2024 •

edited

Loading

oliver-sanders Jan 4, 2024

oliver-sanders Jan 4, 2024

hjoliver commented Jan 8, 2024 •

edited

Loading

wxtim commented Jan 8, 2024 •

edited

Loading

	point = lambda point: IntegerPoint(str(int(point)))
	def point(point): return IntegerPoint(str(int(point)))

Simplify and fix runahead computation. #5893

Simplify and fix runahead computation. #5893

Conversation

hjoliver commented Dec 19, 2023 • edited Loading

oliver-sanders commented Dec 20, 2023 • edited Loading

hjoliver Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

oliver-sanders Jan 4, 2024

Choose a reason for hiding this comment

hjoliver Jan 8, 2024

Choose a reason for hiding this comment

hjoliver Dec 21, 2023

Choose a reason for hiding this comment

wxtim left a comment

Choose a reason for hiding this comment

wxtim Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

oliver-sanders Jan 4, 2024

Choose a reason for hiding this comment

hjoliver Jan 8, 2024

Choose a reason for hiding this comment

wxtim Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

oliver-sanders Jan 4, 2024

Choose a reason for hiding this comment

oliver-sanders Jan 4, 2024

Choose a reason for hiding this comment

hjoliver commented Jan 8, 2024 • edited Loading

wxtim commented Jan 8, 2024 • edited Loading

hjoliver commented Dec 19, 2023 •

edited

Loading

oliver-sanders commented Dec 20, 2023 •

edited

Loading

hjoliver Dec 21, 2023 •

edited

Loading

wxtim Jan 4, 2024 •

edited

Loading

wxtim Jan 8, 2024 •

edited

Loading

hjoliver commented Jan 8, 2024 •

edited

Loading

wxtim commented Jan 8, 2024 •

edited

Loading