Validate job options before writing the job script

toolchain/mfc/run/run.py

@@ -158,6 +158,14 @@ def __execute_job_script(qsystem: queues.QueueSystem):
 
 def run(targets=None, case=None):
     targets = get_targets(list(REQUIRED_TARGETS) + (targets or ARG("targets")))
+
+    # Reject invalid job options before anything has side effects: loading the
+    # case executes case.py, and build, --clean and the job script/input file
+    # generation all touch the build tree or the case directory. A run that is
+    # going to be refused should not leave a stale job script (or a wiped
+    # output directory) behind.
+    __validate_job_options()
+
     case = case or input.load(ARG("input"), ARG("--"))
 
     build(targets)
@@ -197,7 +205,6 @@ def run(targets=None, case=None):
             cons.print(f"  [dim]MPI: {ARG('nodes')} nodes × {ARG('tasks_per_node')} tasks/node = {ARG('nodes') * ARG('tasks_per_node')} total ranks[/dim]")
 
     __generate_job_script(targets, case)
-    __validate_job_options()
     __generate_input_files(targets, case)
 
     if verbosity >= 2:

toolchain/mfc/run/test_run.py

@@ -0,0 +1,120 @@
+"""Tests for run.run() option validation ordering (issue #1511).
+
+An invalid invocation -- ``--no-mpi`` with more than one rank, ``nodes <= 0``,
+a malformed ``--email`` -- must be rejected before run() touches the case
+directory. Previously the job script was rendered and written to disk first,
+so a rejected run left a stale ``<name>.sh`` behind (and, with ``--clean``, had
+already wiped the previous run's outputs).
+"""
+
+import types
+import unittest
+from contextlib import ExitStack
+from unittest.mock import Mock, patch
+
+from .. import state
+from ..common import MFCException
+from . import run as run_mod
+
+
+def _fake_target(name):
+    return types.SimpleNamespace(name=name)
+
+
+def _fake_case(clean):
+    return types.SimpleNamespace(params={}, clean=clean)
+
+
+class _StateSandbox(unittest.TestCase):
+    def setUp(self):
+        self._saved_gARG = dict(state.gARG)
+        state.gARG.clear()
+        state.gARG.update(
+            {
+                "name": "MFC",
+                "input": "case.py",
+                "targets": ["simulation"],
+                "targets_explicit": True,
+                "engine": "interactive",
+                "mpi": True,
+                "nodes": 1,
+                "tasks_per_node": 1,
+                "email": "",
+                "verbose": 0,
+                "clean": True,
+                "archive": None,
+                "dry_run": True,
+                "output_summary": None,
+            }
+        )
+
+    def tearDown(self):
+        state.gARG.clear()
+        state.gARG.update(self._saved_gARG)
+
+    def _patched_run(self):
+        """Patch every side-effecting collaborator of run() with a Mock.
+
+        Module-level dunder names are not mangled, but attribute access from
+        inside a class body would be, so the generators are swapped through
+        ``__dict__`` (same trick as test_archive.py).
+        """
+        stack = ExitStack()
+        mocks = {
+            "build": stack.enter_context(patch.object(run_mod, "build")),
+            "generate_job_script": Mock(),
+            "generate_input_files": Mock(),
+            "clean": Mock(),
+        }
+        stack.enter_context(patch.object(run_mod, "get_targets", side_effect=lambda names: [_fake_target(n) for n in names]))
+        stack.enter_context(
+            patch.dict(
+                run_mod.__dict__,
+                {
+                    "__generate_job_script": mocks["generate_job_script"],
+                    "__generate_input_files": mocks["generate_input_files"],
+                },
+            )
+        )
+        return stack, mocks
+
+
+class TestInvalidOptionsAreRejectedBeforeSideEffects(_StateSandbox):
+    def _assert_rejected_cleanly(self):
+        stack, mocks = self._patched_run()
+        with stack:
+            with self.assertRaises(MFCException):
+                run_mod.run(targets=["simulation"], case=_fake_case(mocks["clean"]))
+
+            self.assertFalse(mocks["generate_job_script"].called, "job script was written before option validation")
+            self.assertFalse(mocks["generate_input_files"].called, "input files were written before option validation")
+            self.assertFalse(mocks["clean"].called, "case was cleaned before option validation")
+            self.assertFalse(mocks["build"].called, "build ran before option validation")
+
+    def test_no_mpi_with_multiple_ranks(self):
+        state.gARG.update({"mpi": False, "tasks_per_node": 4})
+        self._assert_rejected_cleanly()
+
+    def test_non_positive_nodes(self):
+        state.gARG.update({"nodes": 0})
+        self._assert_rejected_cleanly()
+
+    def test_non_positive_tasks_per_node(self):
+        state.gARG.update({"tasks_per_node": 0})
+        self._assert_rejected_cleanly()
+
+    def test_malformed_email(self):
+        state.gARG.update({"email": "not-an-address"})
+        self._assert_rejected_cleanly()
+
+
+class TestValidOptionsStillRun(_StateSandbox):
+    def test_valid_options_reach_generation(self):
+        stack, mocks = self._patched_run()
+        with stack:
+            run_mod.run(targets=["simulation"], case=_fake_case(mocks["clean"]))
+
+            self.assertTrue(mocks["build"].called)
+            self.assertTrue(mocks["clean"].called)
+            self.assertTrue(mocks["generate_job_script"].called)
+            self.assertTrue(mocks["generate_input_files"].called)