From 5061e5649dca0bc99a3b73782745db8722c01f1f Mon Sep 17 00:00:00 2001 From: Mohit Ak Date: Wed, 9 Sep 2026 02:10:57 -0400 Subject: [PATCH] Validate job options before writing the job script run() rendered and wrote .sh (and, with --clean, wiped the previous outputs and kicked off a build) before __validate_job_options() had a chance to refuse the invocation, so `--no-mpi` with several ranks, `nodes <= 0`, or a malformed `--email` aborted the run but left a stale job script behind. Move the validation to the top of run(), right after the case is loaded, so an invalid invocation is rejected before any side effect. Add unit tests that drive run() with each rejected option combination and assert that neither build, case.clean(), the job-script generator nor the input-file generator was invoked, plus a control that a valid invocation still reaches generation. Fixes #1511 (part b). --- toolchain/mfc/run/run.py | 9 ++- toolchain/mfc/run/test_run.py | 120 ++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 toolchain/mfc/run/test_run.py diff --git a/toolchain/mfc/run/run.py b/toolchain/mfc/run/run.py index cd6987cd90..0d5d839c8e 100644 --- a/toolchain/mfc/run/run.py +++ b/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: diff --git a/toolchain/mfc/run/test_run.py b/toolchain/mfc/run/test_run.py new file mode 100644 index 0000000000..c177f9f3ec --- /dev/null +++ b/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 ``.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)