Turn of "against source" mode after validation step of Cylc VR

remove the --against-source option from cylc vr - it should _always_ be true test that Cylc VR unsets the --against-source mode after validation. response to review: delattr add test for validate --mode=live,sim,dummy Apply suggestions from code review Co-authored-by: Ronnie Dutta <[email protected]> Apply suggestions from code review Co-authored-by: Ronnie Dutta <[email protected]> response to review Update tests/functional/cylc-combination-scripts/08-vr-against-src.t Co-authored-by: Ronnie Dutta <[email protected]>
cylc · Jul 23, 2024 · dbcf1cc · dbcf1cc
1 parent 4d6430a
commit dbcf1cc
Show file tree

Hide file tree

Showing 6 changed files with 152 additions and 7 deletions.
diff --git a/changes.d/6213.fix.md b/changes.d/6213.fix.md
@@ -0,0 +1 @@
+Fix bug where the `-S`, `-O` and `-D` options in `cylc vr` would not be applied correctly when restarting a workflow.
diff --git a/cylc/flow/scripts/validate.py b/cylc/flow/scripts/validate.py
@@ -100,7 +100,6 @@
     ),
     VALIDATE_RUN_MODE,
     VALIDATE_ICP_OPTION,
-    VALIDATE_AGAINST_SOURCE_OPTION,
 ]
 
 
@@ -111,9 +110,11 @@ def get_option_parser():
         argdoc=[WORKFLOW_ID_OR_PATH_ARG_DOC],
     )
 
-    validate_options = parser.get_cylc_rose_options() + VALIDATE_OPTIONS
-
-    for option in validate_options:
+    for option in [
+        *parser.get_cylc_rose_options(),
+        *VALIDATE_OPTIONS,
+        VALIDATE_AGAINST_SOURCE_OPTION,
+    ]:
         parser.add_option(*option.args, **option.kwargs)
 
     parser.set_defaults(is_validate=True)

diff --git a/cylc/flow/scripts/validate_reinstall.py b/cylc/flow/scripts/validate_reinstall.py
@@ -62,6 +62,7 @@
 from cylc.flow.scheduler_cli import PLAY_OPTIONS, scheduler_cli
 from cylc.flow.scripts.validate import (
     VALIDATE_OPTIONS,
+    VALIDATE_AGAINST_SOURCE_OPTION,
     run as cylc_validate,
 )
 from cylc.flow.scripts.reinstall import (
@@ -166,11 +167,14 @@ async def vr_cli(parser: COP, options: 'Values', workflow_id: str):
         return 1
 
     # Force on the against_source option:
-    options.against_source = True   # Make validate check against source.
+    options.against_source = True
+
+    # Run cylc validate
     log_subcommand('validate --against-source', workflow_id)
     await cylc_validate(parser, options, workflow_id)
 
-    # Unset is validate after validation.
+    # Unset options that do not apply after validation:
+    delattr(options, 'against_source')
     delattr(options, 'is_validate')
 
     log_subcommand('reinstall', workflow_id)
@@ -198,7 +202,7 @@ async def vr_cli(parser: COP, options: 'Values', workflow_id: str):
             'play',
             unparsed_wid,
             options,
-            compound_script_opts=VR_OPTIONS,
+            compound_script_opts=[*VR_OPTIONS, VALIDATE_AGAINST_SOURCE_OPTION],
             script_opts=(*PLAY_OPTIONS, *parser.get_std_options()),
             source='',  # Intentionally blank
         )

diff --git a/tests/functional/cylc-combination-scripts/08-vr-against-src.t b/tests/functional/cylc-combination-scripts/08-vr-against-src.t
@@ -0,0 +1,95 @@
+#!/usr/bin/env bash
+# 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/>.
+
+#------------------------------------------------------------------------------
+# Test `cylc vr` (Validate Reinstall restart)
+# Tests that VR doesn't modify the source directory for Cylc play.
+# See https://github.com/cylc/cylc-flow/issues/6209
+
+. "$(dirname "$0")/test_header"
+set_test_number 9
+
+# Setup (Run VIP, check that the play step fails in the correct way):
+WORKFLOW_NAME="cylctb-x$(< /dev/urandom tr -dc _A-Z-a-z-0-9 | head -c6)"
+cp "${TEST_SOURCE_DIR}/vr_workflow_fail_on_play/flow.cylc" .
+
+# Run Cylc VIP
+run_fail "setup (vip)" \
+    cylc vip --debug \
+    --workflow-name "${WORKFLOW_NAME}" \
+    --no-run-name
+validate1="$(grep -Po "WARNING - vip:(.*)" "setup (vip).stderr" | awk -F ':' '{print $2}')"
+play1="$(grep -Po "WARNING - play:(.*)" "setup (vip).stderr" | awk -F ':' '{print $2}')"
+
+# Change the workflow to make the reinstall happen:
+echo "" >> flow.cylc
+
+# Run Cylc VR:
+TEST_NAME="${TEST_NAME_BASE}"
+run_fail "${TEST_NAME}" cylc vr "${WORKFLOW_NAME}"
+validate2="$(grep -Po "WARNING - vr:(.*)" "${TEST_NAME_BASE}.stderr" | awk -F ':' '{print $2}')"
+play2="$(grep -Po "WARNING - play:(.*)" "${TEST_NAME_BASE}.stderr" | awk -F ':' '{print $2}')"
+
+# Test that the correct source directory is openened at different
+# stages of Cylc VIP & VR
+TEST_NAME="outputs-created"
+if [[ -n $validate1 && -n $validate2 && -n $play1 && -n $play2 ]]; then
+    ok "${TEST_NAME}"
+else
+    fail "${TEST_NAME}"
+fi
+
+
+TEST_NAME="vip validate and play operate on different folders"
+if [[ $validate1 != "${play1}" ]]; then
+    ok "${TEST_NAME}"
+else
+    fail "${TEST_NAME}"
+fi
+
+TEST_NAME="vr & vip validate operate on the same folder"
+if [[ $validate1 == "${validate2}" ]]; then
+    ok "${TEST_NAME}"
+else
+    fail "${TEST_NAME}"
+fi
+
+TEST_NAME="vr validate and play operate on different folders"
+if [[ $validate2 != "${play2}" ]]; then
+    ok "${TEST_NAME}"
+else
+    fail "${TEST_NAME}"
+fi
+
+TEST_NAME="vip play loads from a cylc-run subdir"
+if [[ "${play2}" =~ cylc-run ]]; then
+    ok "${TEST_NAME}"
+else
+    fail "${TEST_NAME}"
+fi
+
+TEST_NAME="vr play loads from a cylc-run subdir"
+if [[ "${play2}" =~ cylc-run ]]; then
+    ok "${TEST_NAME}"
+else
+    fail "${TEST_NAME}"
+fi
+
+# Clean Up:
+run_ok "${TEST_NAME_BASE}-stop cylc stop ${WORKFLOW_NAME} --now --now"
+purge
+exit 0
diff --git a/tests/functional/cylc-combination-scripts/vr_workflow_fail_on_play/flow.cylc b/tests/functional/cylc-combination-scripts/vr_workflow_fail_on_play/flow.cylc
@@ -0,0 +1,19 @@
+#!jinja2
+{% from "sys" import argv %}
+{% from "cylc.flow" import LOG %}
+{% from "pathlib" import Path %}
+{% if argv[1] == "play" %}
+    this = should cause cylc play to fail
+{% endif %}
+
+{% set SPATH = Path.cwd().__str__() %}
+{% do LOG.warning(argv[1] + ":" + SPATH) %}
+
+
+[scheduling]
+    initial cycle point = 1500
+    [[graph]]
+        P1Y = foo
+
+[runtime]
+    [[foo]]
diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py
@@ -23,6 +23,7 @@
 from cylc.flow.cfgspec.glbl_cfg import glbl_cfg
 from cylc.flow.cfgspec.globalcfg import GlobalConfig
 from cylc.flow.exceptions import (
+    PointParsingError,
     ServiceFileError,
     WorkflowConfigError,
     XtriggerConfigError,
@@ -593,3 +594,27 @@ def _inner(*args, **kwargs):
 
     # the cache should have been updated by the reload
     assert get_platforms(glbl_cfg()) == {'localhost', 'foo', 'bar'}
+
+
+def test_validate_run_mode(flow: Fixture, validate: Fixture):
+    """Test that Cylc validate will only check simulation mode settings
+    if validate --mode simulation or dummy.
+
+    Discovered in:
+    https://github.com/cylc/cylc-flow/pull/6213#issuecomment-2225365825
+    """
+    wid = flow({
+        'scheduling': {'graph': {'R1': 'mytask'}},
+        'runtime': {'mytask': {'simulation': {'fail cycle points': 'alll'}}}
+    })
+
+    # It's fine with run mode live
+    validate(wid)
+
+    # It fails with run mode simulation:
+    with pytest.raises(PointParsingError, match='Incompatible value'):
+        validate(wid, run_mode='simulation')
+
+    # It fails with run mode dummy:
+    with pytest.raises(PointParsingError, match='Incompatible value'):
+        validate(wid, run_mode='dummy')