post_configure: ensure asyncio tasks created by plugins are awaited #5868

oliver-sanders · 2023-12-06T15:26:43Z

This is one of a triplet of PRs which closes cylc/cylc-rose#274

Await any background tasks started by a plugin before continuing. See reinstall + fileinstall: async clash cylc-rose#274
Centralise the plugin loading/running/reporting logic.
Fix the installation test for back-compat-mode.

Reviewers, please check:

It does what it says on the tin.
(re)Install logging works as expected.
Errors in file installation are reported as expected.

Testing:

This diff will make file installation take longer:

diff --git a/metomi/rose/config_processors/fileinstall.py b/metomi/rose/config_processors/fileinstall.py
index 937ed746..9797c042 100644
--- a/metomi/rose/config_processors/fileinstall.py
+++ b/metomi/rose/config_processors/fileinstall.py
@@ -399,6 +399,8 @@ class ConfigProcessorForFile(ConfigProcessorBase):
 
     async def process_job(self, job, conf_tree, loc_dao, work_dir):
         """Process a job, helper for "process"."""
+        import asyncio
+        await asyncio.sleep(1)
         for key, method in [
             (Loc.A_INSTALL, self._target_install),
             (Loc.A_SOURCE, self._source_pull),

This diff will disable the new guard:

diff --git a/cylc/flow/async_util.py b/cylc/flow/async_util.py
index 478c41dc29..14ddc26a80 100644
--- a/cylc/flow/async_util.py
+++ b/cylc/flow/async_util.py
@@ -519,8 +519,8 @@ async def async_block():
         # any tasks two() started will have been awaited by now
     """
     # make a list of all tasks running before we enter the context manager
-    tasks_before = asyncio.all_tasks()
+    # tasks_before = asyncio.all_tasks()
     # run the user code
     yield
     # await any new tasks
-    await asyncio.gather(*(asyncio.all_tasks() - tasks_before))
+    # await asyncio.gather(*(asyncio.all_tasks() - tasks_before))

Try installing a workflow with cylc install --debug, it will now log the time taken by each plugin.

With the async_block it should take >1s (due to the sleep) without the async_block (i.e. with the above diff applied) it should take <1s because the sleep will not be awaited.

CI:

Note that the cylc-rose and metomi-rose tests will fail until both the cylc-flow and metomi-rose branches are merged.

I.E. please run the tests manually due review.

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 no - unreleased issue
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-06T15:32:16Z

cylc/flow/scripts/install.py

-                entry_point.name,
-                exc
-            ) from None
+    async with async_block():


This ensures that any async tasks started by the plugins are awaited before we move on.

Rose fileinstallation will create async tasks but cannot await them (because it's called synchronously) so we have to do it here, otherwise cylc vip would start running the workflow before local file installation has finished.

oliver-sanders · 2023-12-06T15:35:25Z

tests/integration/test_install.py

@@ -97,8 +97,8 @@ def src_run_dirs(
    return tmp_src_path, tmp_run_path


-def test_install_scan_no_ping(
-    src_run_dirs: Callable,


Note, mypy cannot validate type hints for fixtures so they can be completely wrong as in this case.

oliver-sanders · 2023-12-06T15:37:18Z

tests/integration/test_install.py

-        @staticmethod
-        def raiser(*_, **__):
-            import cylc.flow.flags
-            if cylc.flow.flags.cylc7_back_compat:


This test was intended to check that the back-compat flag was set correctly, however, it achieved this by monkeypatching the back-compat flag (i.e. manually setting it to what it expected it to be).

oliver-sanders · 2023-12-06T15:52:43Z

tests/functional/cylc-combination-scripts/05-vr-fail-is-running.t

@@ -54,7 +55,7 @@ run_fail "${TEST_NAME_BASE}-runs" cylc vr "${WORKFLOW_NAME}"
 grep_ok "on elephantshrew." "${TEST_NAME_BASE}-runs.stderr"

 # Clean Up:
-sed -i "s@CYLC_WORKFLOW_HOST=elephantshrew@CYLC_WORKFLOW_HOST=$HOSTNAME@" "${CONTACTFILE}"


Not sure this sed was working (workflow will eventually time out and the test will pass), switched to restoring the original contact file.

oliver-sanders · 2024-01-26T10:37:50Z

cylc/flow/async_util.py

+
+
+@asynccontextmanager
+async def async_block():


Crude but effective way to allow us to call async -> sync -> async.

This makes a list of async tasks before the plugin is called in order to await any new tasks created by the plugin.

This means that plugins can go async like so:

def plugin(opts): asyncio.create_task(run(opts)) # the plugin will return now, but Cylc will await the "run" for us async def run(opts): ...

I think it's a reasonable solution.

Sadly, directly converting the plugin interfaces to async isn't an easy option due to the use of plugins within parsec. Making the plugin async would make loading the global config async which would make global config value retrieval (e.g. the line below) async:

glbl_cfg().get(['install', 'symlink dirs'])

Which would basically require the entire cylc-flow codebase to be async and prohibit config caching at the module level.

cylc/flow/plugins.py

oliver-sanders · 2024-01-26T10:43:46Z

cylc/flow/plugins.py

+                async with _async_block():
+                    plugin_result = meth(*args, **kwargs)


This ensures that any async tasks started by the plugins are awaited before we move on.

Rose fileinstallation will create async tasks but cannot await them (because it's called synchronously) so we have to do it here, otherwise cylc vip would start running the workflow before local file installation has finished.

cylc/flow/async_util.py

cylc/flow/scripts/install.py

tests/integration/test_reinstall.py

wxtim · 2024-02-05T11:22:48Z

Read code
Tried out locally
Assured myself that test failures were unrelated to this change:
- Rose stem failure caused by a local change
- MyPy Failure (metomi/rose) caused by MyPy becoming annoying around Python 3.7.
- Test Tutorials.

cylc/flow/plugins.py

MetRonnie · 2024-02-05T15:25:29Z

tests/integration/test_reinstall.py

+    monkeypatch.setattr(
+        'cylc.flow.plugins._async_block',
+        async_block,
+    )
+
+    # this is what it would have done without async block
+    (one_src.path / 'b').touch()  # give it something else to install
+    my_install_plugin.clear()
+    assert my_install_plugin == []
+    await reinstall_cli(opts=ReInstallOptions(), workflow_id=one_run.id)
+    # the absence of "end" means that the task was not awaited
+    assert my_install_plugin == ['start', 'return']


Not sure I understand why this bit is being tested?

tldr; the second part of this test confirms that the first part is a valid test.

It's easy to write a test for this sort of thing that passes, even if the related functionality doesn't actually work.

To make the that this functionality does work, I wrote two tests:

One checks that there would be a problem if we didn't use the fix.

The second checks that the fix solves the problem.

By writing them together using the same approach you reduce the likelyhood of the test being broken and producing false negatives in the future.

MetRonnie · 2024-02-09T10:49:54Z

Unfortunately functional tests / py-3-latest and test-tutorial-workflow / test (3) hang at some point

oliver-sanders · 2024-02-09T12:02:07Z

Lucky catch, this PR appears to have broken daemonization, which is a major weak spot in our testing (due to its slippery nature). I have no idea why just that one test fails, but you can easily replicate the issue by configuring run hosts.

The cause isn't obvious, even uninstalling cylc-rose and commenting out the async_block code doesn't change it.

[edit] Daemonization is fine via cylc play, it's just the cylc vip script that has issues.

[edit] Can't replicate this any more 😕

* Await any background tasks started by a plugin before continuing. * Addresses cylc/cylc-rose#274 * Centralise the plugin loading/running/reporting logic. * Fix the installation test for back-compat-mode.

wxtim

Read changes
Run tests locally
Tried to break various components of this

* The "cylc vip" command combines three individual coroutines, usually run in their own event loops. * For unknown reasons, the cylc.flow.network.scan.scan coroutine pipe, called as part of the the "install" coroutine, interacts with the daemonization routine called as part of the "play" coroutine. * Keeping these event loops separate avoids the conflict.

oliver-sanders · 2024-02-21T16:14:42Z

I think I have a solution to the hanging problem, see d173c2a for details.

The issue could be replicated in the simplest way:

# [scheduler][run hosts]available=
$ cylc vip

Before d173c2a, the scheduler would startup, but the cylc vip command would not exit.

MetRonnie

I'll test this out tomorrow

MetRonnie · 2024-02-21T17:05:50Z

cylc/flow/scripts/validate_install_play.py

+    # NOTE: We call each of the stages in its own event loop because there is a
+    # strange interaction whereby the cylc.flow.network.scan coroutine pipe can
+    # cause sys.exit to hang on the original process post-daemonization


MetRonnie

Tested

This was referenced Dec 6, 2023

fileinstall: remove asyncio logic cylc/cylc-rose#276

Merged

fileinstall: run coros in the background if loop already running metomi/rose#2744

Merged

oliver-sanders commented Dec 6, 2023

View reviewed changes

oliver-sanders force-pushed the cylc-rose-274 branch from 65a6ec4 to 12f0881 Compare December 6, 2023 15:50

oliver-sanders commented Dec 6, 2023

View reviewed changes

oliver-sanders self-assigned this Dec 6, 2023

oliver-sanders added this to the cylc-8.3.0 milestone Dec 6, 2023

oliver-sanders force-pushed the cylc-rose-274 branch from 12f0881 to 9a8317f Compare December 6, 2023 16:24

oliver-sanders added the non-cylc bug This is a bug, but not in Cylc label Dec 11, 2023

oliver-sanders force-pushed the cylc-rose-274 branch from e1fd499 to dff3b63 Compare January 26, 2024 09:53

oliver-sanders commented Jan 26, 2024

View reviewed changes

cylc/flow/plugins.py Show resolved Hide resolved

oliver-sanders commented Jan 26, 2024

View reviewed changes

oliver-sanders force-pushed the cylc-rose-274 branch 2 times, most recently from 9c45021 to b4c7e44 Compare January 26, 2024 10:56

oliver-sanders marked this pull request as ready for review January 26, 2024 10:56

oliver-sanders requested a review from wxtim January 26, 2024 10:56

wxtim approved these changes Jan 26, 2024

View reviewed changes

cylc/flow/async_util.py Show resolved Hide resolved

wxtim self-requested a review January 26, 2024 13:25

wxtim reviewed Jan 26, 2024

View reviewed changes

cylc/flow/scripts/install.py Show resolved Hide resolved

wxtim requested changes Jan 26, 2024

View reviewed changes

tests/integration/test_reinstall.py Show resolved Hide resolved

wxtim reviewed Jan 26, 2024

View reviewed changes

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

oliver-sanders requested a review from wxtim February 5, 2024 10:28

wxtim approved these changes Feb 5, 2024

View reviewed changes

oliver-sanders requested a review from MetRonnie February 5, 2024 11:38

MetRonnie reviewed Feb 5, 2024

View reviewed changes

oliver-sanders force-pushed the cylc-rose-274 branch from 953f92d to 99985d9 Compare February 7, 2024 16:53

oliver-sanders requested a review from MetRonnie February 7, 2024 16:53

oliver-sanders mentioned this pull request Feb 14, 2024

vr: plugin options not respected when workflow is running #5968

Closed

post_configure: ensure asyncio tasks created by plugins are awaited

e51537e

* Await any background tasks started by a plugin before continuing. * Addresses cylc/cylc-rose#274 * Centralise the plugin loading/running/reporting logic. * Fix the installation test for back-compat-mode.

oliver-sanders force-pushed the cylc-rose-274 branch from 99985d9 to e51537e Compare February 20, 2024 13:15

wxtim approved these changes Feb 21, 2024

View reviewed changes

MetRonnie reviewed Feb 21, 2024

View reviewed changes

wxtim self-requested a review February 22, 2024 09:41

wxtim approved these changes Feb 22, 2024

View reviewed changes

MetRonnie approved these changes Feb 22, 2024

View reviewed changes

MetRonnie merged commit 4ac6f3d into cylc:master Feb 22, 2024
27 checks passed

oliver-sanders deleted the cylc-rose-274 branch February 23, 2024 09:25

wxtim mentioned this pull request Feb 23, 2024

Clear Template Variables from scheduler options before reload #5996

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

post_configure: ensure asyncio tasks created by plugins are awaited #5868

post_configure: ensure asyncio tasks created by plugins are awaited #5868

oliver-sanders commented Dec 6, 2023 •

edited by MetRonnie

Loading

oliver-sanders Dec 6, 2023

oliver-sanders Dec 6, 2023

oliver-sanders Dec 6, 2023

oliver-sanders Dec 6, 2023

oliver-sanders Jan 26, 2024

oliver-sanders Jan 26, 2024

wxtim commented Feb 5, 2024

MetRonnie Feb 5, 2024

oliver-sanders Feb 5, 2024

MetRonnie commented Feb 9, 2024

oliver-sanders commented Feb 9, 2024 •

edited

Loading

wxtim left a comment

oliver-sanders commented Feb 21, 2024 •

edited

Loading

MetRonnie left a comment

MetRonnie Feb 21, 2024

MetRonnie left a comment

		async with _async_block():
		plugin_result = meth(args, *kwargs)

post_configure: ensure asyncio tasks created by plugins are awaited #5868

post_configure: ensure asyncio tasks created by plugins are awaited #5868

Conversation

oliver-sanders commented Dec 6, 2023 • edited by MetRonnie Loading

Reviewers, please check:

Testing:

CI:

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

Choose a reason for hiding this comment

wxtim commented Feb 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MetRonnie commented Feb 9, 2024

oliver-sanders commented Feb 9, 2024 • edited Loading

wxtim left a comment

Choose a reason for hiding this comment

oliver-sanders commented Feb 21, 2024 • edited Loading

MetRonnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

oliver-sanders commented Dec 6, 2023 •

edited by MetRonnie

Loading

oliver-sanders commented Feb 9, 2024 •

edited

Loading

oliver-sanders commented Feb 21, 2024 •

edited

Loading