vr: restart the workflow with no changes according to user prompt

oliver-sanders · oliver-sanders · commit 8021ce9852ae · 2024-09-04T10:09:28.000+01:00
* 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.
diff --git a/changes.d/6354.feat.md 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
@@ -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,34 +195,54 @@ 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)
 
     # Unset options that do not apply after validation:
     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('<bold>Restart anyway?</bold> [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
@@ -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 <http://www.gnu.org/licenses/>.
diff --git a/tests/integration/scripts/conftest.py 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 <http://www.gnu.org/licenses/>.
+
+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/scripts/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
@@ -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 <http://www.gnu.org/licenses/>.
+
+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

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Fix an issue that could cause clock-expired tasks to be erroneously retried if "execution retry delays" were configured.`