From 5af333dfda1bfe3ae6f867151683e8d1eb64f32d Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Tue, 3 Sep 2024 18:12:24 +0100 Subject: [PATCH 1/3] vr: restart the workflow with no changes according to user prompt * Closes #6261 * If there are no changes to reinstall AND the workflow is stopped, prompt the user to see whether they want to restart it anyway. * This makes `cylc vr` more useful as the "I want to restart my workflow" command. * But it also ensures that they are aware if no changes are present as they might have forgotten to press save or run the command on the wrong workflow or whatever. --- changes.d/6354.feat.md | 1 + cylc/flow/scripts/validate_reinstall.py | 48 +++++++--- tests/integration/scripts/__init__.py | 15 +++ tests/integration/scripts/conftest.py | 56 +++++++++++ .../{ => scripts}/test_reinstall.py | 30 +----- .../scripts/test_validate_reinstall.py | 95 +++++++++++++++++++ 6 files changed, 205 insertions(+), 40 deletions(-) create mode 100644 changes.d/6354.feat.md create mode 100644 tests/integration/scripts/__init__.py create mode 100644 tests/integration/scripts/conftest.py rename tests/integration/{ => scripts}/test_reinstall.py (93%) create mode 100644 tests/integration/scripts/test_validate_reinstall.py diff --git a/changes.d/6354.feat.md b/changes.d/6354.feat.md new file mode 100644 index 00000000000..b150a0afb7f --- /dev/null +++ b/changes.d/6354.feat.md @@ -0,0 +1 @@ +Fix an issue that could cause clock-expired tasks to be erroneously retried if "execution retry delays" were configured. diff --git a/cylc/flow/scripts/validate_reinstall.py b/cylc/flow/scripts/validate_reinstall.py index 1385ad20bc1..bb09a727381 100644 --- a/cylc/flow/scripts/validate_reinstall.py +++ b/cylc/flow/scripts/validate_reinstall.py @@ -46,6 +46,8 @@ if TYPE_CHECKING: from optparse import Values +from ansimarkup import parse as cparse + from cylc.flow import LOG from cylc.flow.exceptions import ( ContactFileExists, @@ -74,7 +76,7 @@ from cylc.flow.scripts.reload import ( run as cylc_reload ) -from cylc.flow.terminal import cli_function +from cylc.flow.terminal import cli_function, is_terminal from cylc.flow.workflow_files import detect_old_contact_file CYLC_ROSE_OPTIONS = COP.get_cylc_rose_options() @@ -87,6 +89,10 @@ modify={'cylc-rose': 'validate, install'} ) +_input = input # to enable testing + +NO_CHANGES_STR = 'No changes to reinstall' + def get_option_parser() -> COP: parser = COP( @@ -189,7 +195,7 @@ async def vr_cli( # Force on the against_source option: options.against_source = True - # Run cylc validate + # Run "cylc validate" log_subcommand('validate --against-source', workflow_id) await cylc_validate(parser, options, workflow_id) @@ -197,26 +203,46 @@ async def vr_cli( delattr(options, 'against_source') delattr(options, 'is_validate') + # Run "cylc reinstall" log_subcommand('reinstall', workflow_id) reinstall_ok = await cylc_reinstall( - options, workflow_id, + options, + workflow_id, [], print_reload_tip=False ) if not reinstall_ok: - LOG.warning( - 'No changes to source: No reinstall or' - f' {"reload" if workflow_running else "play"} required.' - ) - return False - - # Run reload if workflow is running or paused: + if ( + not workflow_running + and is_terminal() + and not options.skip_interactive + ): + # there are no changes to install but the workflow isn't running + # => ask the user if they want to restart it anyway + usr = None + while usr not in ['y', 'n']: + LOG.warning(NO_CHANGES_STR) + usr = _input( + cparse('Restart anyway? [y/n]: ') + ).lower() + if usr == 'n': + return False + else: + # the are no changes to install and the workflow is running + # => there is nothing for us to do here + LOG.warning( + f'{NO_CHANGES_STR}: No reinstall or' + f' {"reload" if workflow_running else "play"} required.' + ) + return False + + # Run "cylc reload" (if workflow is running or paused) if workflow_running: log_subcommand('reload', workflow_id) await cylc_reload(options, workflow_id) return True - # run play anyway, to play a stopped workflow: + # Run "cylc play" (if workflow is stopped) else: set_timestamps(LOG, options.log_timestamp) cleanup_sysargv( diff --git a/tests/integration/scripts/__init__.py b/tests/integration/scripts/__init__.py new file mode 100644 index 00000000000..763e078d6a6 --- /dev/null +++ b/tests/integration/scripts/__init__.py @@ -0,0 +1,15 @@ +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) NIWA & British Crown (Met Office) & Contributors. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . diff --git a/tests/integration/scripts/conftest.py b/tests/integration/scripts/conftest.py new file mode 100644 index 00000000000..b76ba422cb6 --- /dev/null +++ b/tests/integration/scripts/conftest.py @@ -0,0 +1,56 @@ +#!/usr/bin/env python3 + +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) NIWA & British Crown (Met Office) & Contributors. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +from types import SimpleNamespace +from uuid import uuid1 + +import pytest + +from cylc.flow.workflow_files import WorkflowFiles + +from ..utils.flow_writer import flow_config_str + + +@pytest.fixture +def one_src(tmp_path, one_conf): + src_dir = tmp_path + (src_dir / 'flow.cylc').write_text(flow_config_str(one_conf)) + (src_dir / 'rose-suite.conf').touch() + return SimpleNamespace(path=src_dir) + + +@pytest.fixture +def one_run(one_src, test_dir, run_dir): + w_run_dir = test_dir / str(uuid1()) + w_run_dir.mkdir() + (w_run_dir / 'flow.cylc').write_text( + (one_src.path / 'flow.cylc').read_text() + ) + (w_run_dir / 'rose-suite.conf').write_text( + (one_src.path / 'rose-suite.conf').read_text() + ) + install_dir = (w_run_dir / WorkflowFiles.Install.DIRNAME) + install_dir.mkdir(parents=True) + (install_dir / WorkflowFiles.Install.SOURCE).symlink_to( + one_src.path, + target_is_directory=True, + ) + return SimpleNamespace( + path=w_run_dir, + id=str(w_run_dir.relative_to(run_dir)), + ) diff --git a/tests/integration/test_reinstall.py b/tests/integration/scripts/test_reinstall.py similarity index 93% rename from tests/integration/test_reinstall.py rename to tests/integration/scripts/test_reinstall.py index b79116828f6..60b0f077f6b 100644 --- a/tests/integration/test_reinstall.py +++ b/tests/integration/scripts/test_reinstall.py @@ -19,8 +19,6 @@ import asyncio from contextlib import asynccontextmanager from pathlib import Path -from types import SimpleNamespace -from uuid import uuid1 import pytest @@ -39,7 +37,7 @@ WorkflowFiles, ) -from .utils.entry_points import EntryPointWrapper +from ..utils.entry_points import EntryPointWrapper ReInstallOptions = Options(reinstall_gop()) @@ -66,32 +64,6 @@ def non_interactive(monkeypatch): ) -@pytest.fixture -def one_src(tmp_path): - src_dir = tmp_path - (src_dir / 'flow.cylc').touch() - (src_dir / 'rose-suite.conf').touch() - return SimpleNamespace(path=src_dir) - - -@pytest.fixture -def one_run(one_src, test_dir, run_dir): - w_run_dir = test_dir / str(uuid1()) - w_run_dir.mkdir() - (w_run_dir / 'flow.cylc').touch() - (w_run_dir / 'rose-suite.conf').touch() - install_dir = (w_run_dir / WorkflowFiles.Install.DIRNAME) - install_dir.mkdir(parents=True) - (install_dir / WorkflowFiles.Install.SOURCE).symlink_to( - one_src.path, - target_is_directory=True, - ) - return SimpleNamespace( - path=w_run_dir, - id=str(w_run_dir.relative_to(run_dir)), - ) - - async def test_rejects_random_workflows(one, one_run): """It should only work with workflows installed by cylc install.""" with pytest.raises(WorkflowFilesError) as exc_ctx: diff --git a/tests/integration/scripts/test_validate_reinstall.py b/tests/integration/scripts/test_validate_reinstall.py new file mode 100644 index 00000000000..5ecbf3edadf --- /dev/null +++ b/tests/integration/scripts/test_validate_reinstall.py @@ -0,0 +1,95 @@ +#!/usr/bin/env python3 + +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) NIWA & British Crown (Met Office) & Contributors. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +from cylc.flow.option_parsers import Options +from cylc.flow.scripts.validate_reinstall import ( + get_option_parser as vr_gop, + vr_cli, +) + +ValidateReinstallOptions = Options(vr_gop()) + + +async def test_prompt_for_running_workflow_with_no_changes( + monkeypatch, + caplog, + capsys, + log_filter, + one_run, + capcall, +): + """It should reinstall and restart the workflow with no changes. + + See: https://github.com/cylc/cylc-flow/issues/6261 + + We hope to get users into the habbit of "cylc vip" to create a new run, + and "cylc vr" to contine an old one (picking up any new changes in the + process). + + This works fine, unless there are no changes to reinstall, in which case + the "cylc vr" command exits (nothing to do). + + The "nothing to reinstall" situation can be interpretted two ways: + 1. Unexpected error, the user expected there to be something to reinstall, + but there wasn't. E.g, they forgot to press save. + 2. Unexpected annoyance, I wanted to restart the workflow, just do it. + + To handle this we explain that there are no changes to reinstall and + prompt the user to see if they want to press save or restart the workflow. + """ + # disable the clean_sysargv logic (this interferes with other tests) + cleanup_sysargv_calls = capcall( + 'cylc.flow.scripts.validate_reinstall.cleanup_sysargv' + ) + + # answer "y" to all prompts + def _input(prompt): + print(prompt) + return 'y' + + monkeypatch.setattr( + 'cylc.flow.scripts.validate_reinstall._input', + _input, + ) + + # make it look like we are running this command in a terminal + monkeypatch.setattr( + 'cylc.flow.scripts.validate_reinstall.is_terminal', + lambda: True + ) + monkeypatch.setattr( + 'cylc.flow.scripts.reinstall.is_terminal', + lambda: True + ) + + # attempt to restart it with "cylc vr" + ret = await vr_cli( + vr_gop(), ValidateReinstallOptions(), one_run.id + ) + # the workflow should reinstall + assert ret + + # the user should have been warned that there were no changes to reinstall + assert log_filter(caplog, contains='No changes to reinstall') + + # they should have been presented with a prompt + # (to which we have hardcoded the response "y") + assert 'Restart anyway?' in capsys.readouterr()[0] + + # the workflow should have restarted + assert len(cleanup_sysargv_calls) == 1 From 2505de933d2c38e680be5b752b214b762df4bd9e Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Wed, 23 Oct 2024 14:59:58 +0100 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com> --- changes.d/6261.feat.md | 1 + changes.d/6354.feat.md | 1 - tests/integration/scripts/test_validate_reinstall.py | 4 ++-- 3 files changed, 3 insertions(+), 3 deletions(-) create mode 100644 changes.d/6261.feat.md delete mode 100644 changes.d/6354.feat.md diff --git a/changes.d/6261.feat.md b/changes.d/6261.feat.md new file mode 100644 index 00000000000..46d135919fd --- /dev/null +++ b/changes.d/6261.feat.md @@ -0,0 +1 @@ +Allow a workflow to be restarted by `cylc vr` even if there are no changes to reinstall. diff --git a/changes.d/6354.feat.md b/changes.d/6354.feat.md deleted file mode 100644 index b150a0afb7f..00000000000 --- a/changes.d/6354.feat.md +++ /dev/null @@ -1 +0,0 @@ -Fix an issue that could cause clock-expired tasks to be erroneously retried if "execution retry delays" were configured. diff --git a/tests/integration/scripts/test_validate_reinstall.py b/tests/integration/scripts/test_validate_reinstall.py index 5ecbf3edadf..e74e28cb6ac 100644 --- a/tests/integration/scripts/test_validate_reinstall.py +++ b/tests/integration/scripts/test_validate_reinstall.py @@ -37,14 +37,14 @@ async def test_prompt_for_running_workflow_with_no_changes( See: https://github.com/cylc/cylc-flow/issues/6261 - We hope to get users into the habbit of "cylc vip" to create a new run, + We hope to get users into the habit of "cylc vip" to create a new run, and "cylc vr" to contine an old one (picking up any new changes in the process). This works fine, unless there are no changes to reinstall, in which case the "cylc vr" command exits (nothing to do). - The "nothing to reinstall" situation can be interpretted two ways: + The "nothing to reinstall" situation can be interpreted two ways: 1. Unexpected error, the user expected there to be something to reinstall, but there wasn't. E.g, they forgot to press save. 2. Unexpected annoyance, I wanted to restart the workflow, just do it. From 397e6560e0e2e69f71f7aa0ff086321671f8bab7 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Fri, 25 Oct 2024 11:42:21 +0100 Subject: [PATCH 3/3] vr: test reinstallation abort --- .../scripts/test_validate_reinstall.py | 69 ++++++++++++++----- 1 file changed, 50 insertions(+), 19 deletions(-) diff --git a/tests/integration/scripts/test_validate_reinstall.py b/tests/integration/scripts/test_validate_reinstall.py index e74e28cb6ac..3a62219dff6 100644 --- a/tests/integration/scripts/test_validate_reinstall.py +++ b/tests/integration/scripts/test_validate_reinstall.py @@ -25,6 +25,33 @@ ValidateReinstallOptions = Options(vr_gop()) +def answer_prompts(monkeypatch, *responses): + """Hardcode responses to "cylc vr" interactive prompts.""" + # make it look like we are running this command in a terminal + monkeypatch.setattr( + 'cylc.flow.scripts.validate_reinstall.is_terminal', + lambda: True + ) + monkeypatch.setattr( + 'cylc.flow.scripts.reinstall.is_terminal', + lambda: True + ) + + # patch user input + count = -1 + + def _input(prompt): + nonlocal count, responses + count += 1 + print(prompt) # send the prompt to stdout for testing + return responses[count] + + monkeypatch.setattr( + 'cylc.flow.scripts.validate_reinstall._input', + _input, + ) + + async def test_prompt_for_running_workflow_with_no_changes( monkeypatch, caplog, @@ -57,25 +84,8 @@ async def test_prompt_for_running_workflow_with_no_changes( 'cylc.flow.scripts.validate_reinstall.cleanup_sysargv' ) - # answer "y" to all prompts - def _input(prompt): - print(prompt) - return 'y' - - monkeypatch.setattr( - 'cylc.flow.scripts.validate_reinstall._input', - _input, - ) - - # make it look like we are running this command in a terminal - monkeypatch.setattr( - 'cylc.flow.scripts.validate_reinstall.is_terminal', - lambda: True - ) - monkeypatch.setattr( - 'cylc.flow.scripts.reinstall.is_terminal', - lambda: True - ) + # answer "y" to prompt + answer_prompts(monkeypatch, 'y') # attempt to restart it with "cylc vr" ret = await vr_cli( @@ -93,3 +103,24 @@ def _input(prompt): # the workflow should have restarted assert len(cleanup_sysargv_calls) == 1 + + +async def test_reinstall_abort( + monkeypatch, + capsys, + log_filter, + one_run, +): + """It should abort reinstallation according to user prompt.""" + # answer 'n' to prompt + answer_prompts(monkeypatch, 'n') + + # attempt to restart it with "cylc vr" + ret = await vr_cli( + vr_gop(), ValidateReinstallOptions(), one_run.id + ) + assert ret is False + + # they should have been presented with a prompt + # (to which we have hardcoded the response "n") + assert 'Restart anyway?' in capsys.readouterr()[0]