From 64f35f4f00d06e213b5770af57d9263d7be5a23b Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Tue, 21 Nov 2023 13:10:46 +0000 Subject: [PATCH 1/3] task_pool: fix compute_runahead calculation * Closes https://github.com/cylc/cylc-flow/issues/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. --- cylc/flow/task_pool.py | 49 ++++++++++------ tests/integration/test_task_pool.py | 89 +++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 17 deletions(-) diff --git a/cylc/flow/task_pool.py b/cylc/flow/task_pool.py index 6ddc62d39f2..e964d2bfedb 100644 --- a/cylc/flow/task_pool.py +++ b/cylc/flow/task_pool.py @@ -51,13 +51,13 @@ from cylc.flow.task_state import ( TASK_STATUSES_ACTIVE, TASK_STATUSES_FINAL, - TASK_STATUS_WAITING, TASK_STATUS_EXPIRED, + TASK_STATUS_FAILED, TASK_STATUS_PREPARING, - TASK_STATUS_SUBMITTED, TASK_STATUS_RUNNING, + TASK_STATUS_SUBMITTED, TASK_STATUS_SUCCEEDED, - TASK_STATUS_FAILED, + TASK_STATUS_WAITING, TASK_OUTPUT_EXPIRED, TASK_OUTPUT_FAILED, TASK_OUTPUT_SUCCEEDED, @@ -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 - or any( - not itask.state( - TASK_STATUS_FAILED, - TASK_STATUS_SUCCEEDED, - TASK_STATUS_EXPIRED - ) - or ( - # For Cylc 7 back-compat, ignore incomplete tasks. - # (Success is required in back-compat mode, so - # failedtasks end up as incomplete; and Cylc 7 - # ignores failed tasks in computing the limit). - itask.state.outputs.is_incomplete() - and not cylc.flow.flags.cylc7_back_compat + any( + # filter out runahead tasks + itask.state(is_runahead=False) + and ( + # waiting tasks + # * tasks with partially satisfied prereqs + # * tasks with incomplete xtriggers + # * held tasks + itask.state(TASK_STATUS_WAITING) + + # unfinished tasks (back-compat mode) + # * Cylc 7 runahead logic considered a cycle to be + # active if it had "unfinished" tasks + or ( + cylc.flow.flags.cylc7_back_compat + and not itask.state( + TASK_STATUS_FAILED, + TASK_STATUS_SUCCEEDED, + TASK_STATUS_EXPIRED, + ) + ) + + # incomplete tasks (optional outputs) + # * tasks with one or more required outputs + # incomplete + or ( + not cylc.flow.flags.cylc7_back_compat + and itask.state.outputs.is_incomplete() + ) ) for itask in itasks ) diff --git a/tests/integration/test_task_pool.py b/tests/integration/test_task_pool.py index 97526088660..e96f21da28b 100644 --- a/tests/integration/test_task_pool.py +++ b/tests/integration/test_task_pool.py @@ -1246,3 +1246,92 @@ async def test_detect_incomplete_tasks( assert log_filter(log, contains=f"[{itask}] did not complete required outputs:") # the task should not have been removed assert itask in schd.pool.get_tasks() + + +@pytest.mark.parametrize('compat_mode', ['compat-mode', 'normal-mode']) +@pytest.mark.parametrize('cycling_mode', ['integer', 'datetime']) +async def test_compute_runahead( + cycling_mode, + compat_mode, + flow, + scheduler, + start, + monkeypatch, +): + """Test the calculation of the runahead limit. + + This test ensures that: + * Runahead tasks are excluded from computations + see https://github.com/cylc/cylc-flow/issues/5825 + * Tasks are initiated with the correct is_runahead status on statup. + * Behaviour is the same in compat/regular modes. + * Behaviour is the same for integer/datetime cycling modes. + + """ + if cycling_mode == 'integer': + config = { + 'scheduler': { + 'allow implicit tasks': 'True', + }, + 'scheduling': { + 'initial cycle point': '1', + 'cycling mode': 'integer', + 'runahead limit': 'P3', + 'graph': { + 'P1': 'a' + }, + } + } + point = lambda point: IntegerPoint(str(int(point))) + else: + config = { + 'scheduler': { + 'allow implicit tasks': 'True', + 'cycle point format': 'CCYY', + }, + 'scheduling': { + 'initial cycle point': '0001', + 'runahead limit': 'P3Y', + 'graph': { + 'P1Y': 'a' + }, + } + } + point = ISO8601Point + + monkeypatch.setattr( + 'cylc.flow.flags.cylc7_back_compat', + compat_mode == 'compat-mode', + ) + + id_ = flow(config) + schd = scheduler(id_) + async with start(schd): + schd.pool.compute_runahead(force=True) + assert int(str(schd.pool.runahead_limit_point)) == 4 + + # ensure task states are initiated with is_runahead status + assert schd.pool.get_task(point('0001'), 'a').state(is_runahead=False) + assert schd.pool.get_task(point('0005'), 'a').state(is_runahead=True) + + # mark the first three cycles as running + for cycle in range(1, 4): + schd.pool.get_task(point(f'{cycle:04}'), 'a').state.reset( + TASK_STATUS_RUNNING + ) + schd.pool.compute_runahead(force=True) + assert int(str(schd.pool.runahead_limit_point)) == 4 # no change + + # mark cycle 1 as incomplete (but finished) + schd.pool.get_task(point('0001'), 'a').state.reset( + TASK_STATUS_SUBMIT_FAILED + ) + schd.pool.compute_runahead(force=True) + assert int(str(schd.pool.runahead_limit_point)) == 4 # no change + + # mark cycle 1 as complete + schd.pool.get_task(point('0001'), 'a').state.reset( + TASK_STATUS_SUCCEEDED + ) + schd.pool.compute_runahead(force=True) + assert int(str(schd.pool.runahead_limit_point)) == 5 # +1 From e0cc4cb31547cabb268244d4542895a3d5dd66bc Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Wed, 22 Nov 2023 15:34:43 +0000 Subject: [PATCH 2/3] tests/i: add note about module-scoped fixtures --- tests/integration/README.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/integration/README.md b/tests/integration/README.md index 71bd6bfb922..e7246a53ca6 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -64,3 +64,21 @@ These methods both shut down the workflow / clean up after themselves. It is necessary to shut down workflows correctly to clean up resorces and running tasks. + +## Module Scoped Fixtures + +There's a reasonable overhead to some text fixtures, especially the ones which +involve writing files to disk or starting Cylc schedulers. + +To make tests run faster you can use module-scoped fixtures, these are test +fixtures which are created once, then reused for all tests in the module. + +You'll find a bunch of module-scoped fixtues prefixed with `mod_`, e.g. +`mod_start` is the module-scoped version of `start`. When using module-scoped +fixtures, ensure that tests do not modify the fixture object as this will enable +tests to interact. + +In order to get speedup from module-scoped fixtures when running with +pytest-xdist, we configure pytest-xdist to run all of the tests in a module in +series using the same pytest runner. This incentivises breaking up larger test +modules. From 94a749c1160f0006944185a4d237e8bc381daedf Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Tue, 28 Nov 2023 10:48:40 +0000 Subject: [PATCH 3/3] tests: redact TZ environment variable * 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. --- pytest.ini | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pytest.ini b/pytest.ini index 81df3785cec..f45af77f6e5 100644 --- a/pytest.ini +++ b/pytest.ini @@ -34,11 +34,12 @@ testpaths = tests/integration/ env = # 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 doctest_optionflags = NORMALIZE_WHITESPACE IGNORE_EXCEPTION_DETAIL ELLIPSIS asyncio_mode = auto markers= - linkcheck: Test links \ No newline at end of file + linkcheck: Test links