diff --git a/autohands/build_util.py b/autohands/build_util.py index b8ea25c..60f49f4 100644 --- a/autohands/build_util.py +++ b/autohands/build_util.py @@ -2,9 +2,11 @@ import logging import os import re +import shutil import signal import subprocess import sys +import tempfile import time import traceback from pathlib import Path @@ -356,11 +358,86 @@ def is_clean_skip_exit(output: str) -> bool: return bool(tail) and _SKIP_EXIT_RE.match(tail[-1]) is not None -def execute_notebook(f, report=None, env=None): +def regenerate_notebook(nb_path, scripts_dir) -> Path: + """ + Regenerate one notebook from its source ``.py`` into a temp dir. + + The recovery for a *stale* notebook: the script moved on but the committed + ``.ipynb`` was never refreshed by ``generate.py``. Whole-workspace + regeneration stays ``generate.py``'s job — this regenerates only the single + notebook in front of the runner, so the recovery is cheap. + + The regenerated copy lives in a temp dir; the committed ``notebooks/`` tree + is never touched, so a smoke run leaves the worktree clean. + + Parameters + ---------- + nb_path + The notebook that failed, e.g. ``notebooks/imaging/model_fit.ipynb``. + scripts_dir + The directory holding the source scripts, e.g. ``/scripts``. + The source is looked up at the notebook's path relative to its own + ``notebooks/`` root, with a ``.py`` suffix. + + Returns + ------- + The path to the regenerated notebook. + + Raises + ------ + FileNotFoundError + If no source script exists — nothing to regenerate from. + """ + nb_path = Path(nb_path) + scripts_dir = Path(scripts_dir) + script_path = scripts_dir / Path(nb_path.name).with_suffix(".py") + if not script_path.exists(): + raise FileNotFoundError(f"No source script at {script_path}") + + tmp_dir = Path(tempfile.mkdtemp(prefix="smoke_regen_")) + tmp_script = tmp_dir / script_path.name + shutil.copy(script_path, tmp_script) + old_cwd = os.getcwd() + try: + os.chdir(tmp_dir) + return Path(py_to_notebook(tmp_script)) + finally: + os.chdir(old_cwd) + + +def _run_notebook_once(f, report=None, env=None, recorded=None, write_back=True): + """ + Run one notebook once and classify the outcome. + + Returns ``"passed"``, ``"failed"`` or ``"timeout"`` so the caller can decide + whether a retry is warranted; results are still appended to ``report`` here, + under ``recorded`` (which may differ from ``f`` when a regenerated temp copy + is standing in for the committed notebook). + """ print(f"Running <{f}> at {datetime.datetime.now().isoformat()}") timeout_secs = timeout_for(env) + recorded = str(recorded if recorded is not None else f) + + run_target = Path(f) + scratch_dir = None + if not write_back: + # run_notebook.py writes the executed notebook back in place, so point + # it at a throwaway copy to leave the committed tree clean. The kernel's + # cwd is pinned to the repo root either way, so relative dataset/ and + # output/ paths resolve identically. + scratch_dir = Path(tempfile.mkdtemp(prefix="smoke_nb_")) + run_target = scratch_dir / Path(f).name + shutil.copyfile(f, run_target) + + try: + return _classify_notebook_run(run_target, recorded, report, env, timeout_secs) + finally: + if scratch_dir is not None: + shutil.rmtree(scratch_dir, ignore_errors=True) + +def _classify_notebook_run(run_target, recorded, report, env, timeout_secs): start = time.time() try: # stderr is always captured so a clean `sys.exit(0)` skip guard can be @@ -372,11 +449,16 @@ def execute_notebook(f, report=None, env=None): # from the repo root. nbconvert has no CLI flag for the kernel cwd, so # the runner sets resources['metadata']['path'] via the Python API. # Still a subprocess, so isolation/timeout/env are unchanged. + # + # It is invoked through sys.executable, NOT a bare `jupyter` binary, so + # a missing notebook toolchain surfaces as an ordinary non-zero exit + # (one FAIL, run continues) rather than a FileNotFoundError escaping and + # aborting the whole run with no summary line. run_capped( [ sys.executable, str(Path(__file__).parent / "run_notebook.py"), - str(f), + str(run_target), str(Path.cwd()), ], check=True, @@ -398,12 +480,12 @@ def execute_notebook(f, report=None, env=None): if captured: message = f"{message}\n{captured}" report.results.append(ScriptResult( - file=str(f), + file=recorded, status=Status.TIMEOUT, duration_seconds=duration, error_message=message, )) - return + return "timeout" logging.exception(e) sys.exit(1) except subprocess.CalledProcessError as e: @@ -417,26 +499,26 @@ def execute_notebook(f, report=None, env=None): from result_collector import ScriptResult, Status print(f" PASS (skipped via sys.exit(0), {duration:.1f}s)") report.results.append(ScriptResult( - file=str(f), + file=recorded, status=Status.PASSED, duration_seconds=duration, error_message="sys.exit(0) skip guard (ignored)", )) else: print(f" PASS (skipped via sys.exit(0), {duration:.1f}s)") - return + return "passed" if "InversionException" in traceback.format_exc(): if report is not None: from result_collector import ScriptResult, Status print(f" PASS (InversionException, {duration:.1f}s)") report.results.append(ScriptResult( - file=str(f), + file=recorded, status=Status.PASSED, duration_seconds=duration, error_message="InversionException (ignored)", )) - return + return "passed" if report is not None: from result_collector import ScriptResult, Status @@ -444,13 +526,13 @@ def execute_notebook(f, report=None, env=None): last_line = stderr.strip().splitlines()[-1] if stderr.strip() else str(e) print(f" FAIL ({duration:.1f}s) {last_line}") report.results.append(ScriptResult( - file=str(f), + file=recorded, status=Status.FAILED, duration_seconds=duration, error_message=str(e), traceback=stderr, )) - return + return "failed" # stderr is captured now (see the subprocess call above), so echo it # before dying or the failure would be silent. if captured: @@ -463,10 +545,71 @@ def execute_notebook(f, report=None, env=None): from result_collector import ScriptResult, Status print(f" PASS ({duration:.1f}s)") report.results.append(ScriptResult( - file=str(f), + file=recorded, status=Status.PASSED, duration_seconds=duration, )) + return "passed" + + +def execute_notebook(f, report=None, env=None, write_back=True, + retry_from_scripts=None, report_as=None): + """ + Execute one notebook as a subprocess, with the kernel cwd at the repo root. + + ``write_back`` controls whether the notebook keeps its executed outputs. + True (the default) is the generation/release contract: ``run_notebook.py`` + writes back in place, matching ``nbconvert --output ``, so a + partially-executed notebook keeps its outputs and generated notebooks are + committed with them. False executes a throwaway copy and leaves the + committed tree untouched — what a **PR smoke gate** needs, since dirtying + ``notebooks/`` on every run would have the gate mutate the tree it is + testing. + + ``retry_from_scripts``, set to a workspace's ``scripts`` directory, turns a + genuine failure into one regenerate-and-retry: the notebook is rebuilt from + its source ``.py`` (:func:`regenerate_notebook`) and run once more. This + catches a *stale* notebook — the script moved on but the committed + ``.ipynb`` was never refreshed by ``generate.py``. Deliberately narrow: + + * a TIMEOUT is never retried — it would burn a second full cap to reach the + same result, doubling the slowest entry's cost; + * a clean skip-guard exit is already a PASS and never reaches the retry; + * the retry's verdict REPLACES the first attempt's, so one notebook + contributes exactly one result. + + ``report_as`` is the path recorded in the report, defaulting to ``f``; the + retry passes the original notebook so a temp path never leaks into results. + + Returns ``"passed"``, ``"failed"`` or ``"timeout"``. + """ + recorded = report_as if report_as is not None else f + mark = len(report.results) if report is not None else None + + status = _run_notebook_once( + f, report=report, env=env, recorded=recorded, write_back=write_back + ) + if status != "failed" or retry_from_scripts is None: + return status + + print(" notebook failed; regenerating from source script and retrying...") + try: + regenerated = regenerate_notebook(f, retry_from_scripts) + except Exception as exc: + # No source script, or generation itself failed. The first attempt's + # FAIL stands — the recovery was unavailable, not the notebook fixed. + print(f" [regenerate_notebook] {exc}") + return status + + if report is not None: + # Drop the first attempt so the retry's verdict is the only one recorded. + del report.results[mark:] + # The regenerated notebook is already a throwaway in /tmp, so writing its + # outputs back costs nothing and touches no committed file. + return _run_notebook_once( + regenerated, report=report, env=env, recorded=recorded, write_back=True + ) + def execute_notebooks_in_folder( @@ -476,15 +619,35 @@ def execute_notebooks_in_folder( report=None, skip_reasons=None, env_config=None, + files=None, + write_back=True, + retry_from_scripts=None, ): + """ + Run the notebooks under ``directory``, honouring ``no_run_list``. + + ``files`` selects the discovery model, mirroring + :func:`execute_scripts_in_folder`: omitted, notebooks are discovered + recursively (opt-out coverage); supplied by :func:`files_from_list`, exactly + those entries run in that order (opt-in). ``no_run_list`` applies either way + and wins over the list. + + ``write_back`` and ``retry_from_scripts`` are passed through to + :func:`execute_notebook`; a PR smoke gate wants ``write_back=False`` (leave + the committed tree clean) plus the workspace's ``scripts`` directory for the + stale-notebook recovery. + """ # Infrastructure files — always skip, never report infra_skip = ["__init__", "README"] no_run_list.extend(infra_skip) - files = list((Path.cwd() / directory).rglob("*.ipynb")) - print(f"Found {len(files)} notebooks") + if files is None: + files = sorted((Path.cwd() / directory).rglob("*.ipynb")) + print(f"Found {len(files)} notebooks") + else: + print(f"Running {len(files)} listed notebooks") - for file in sorted(files): + for file in files: if file.stem in infra_skip: continue if visualise_dict is not None: @@ -497,6 +660,9 @@ def execute_notebooks_in_folder( ): continue if should_skip(file, no_run_list): + # Before the existence check, for the same reason as the script + # runner: an excluded notebook that has also been deleted is still + # excluded, and a failure would contradict the exclusion. if report is not None: from result_collector import ScriptResult, Status reason = _find_skip_reason(file, no_run_list, skip_reasons or {}) @@ -505,10 +671,28 @@ def execute_notebooks_in_folder( status=Status.SKIPPED, skip_reason=reason, )) + elif not file.exists(): + # Only reachable from an allowlist. One FAIL, run continues. + print(f" {file} ... FAIL (listed but not found)") + if report is not None: + from result_collector import ScriptResult, Status + report.results.append(ScriptResult( + file=str(file), + status=Status.FAILED, + error_message=f"Listed in the notebook list but not found at {file}", + )) + else: + sys.exit(1) else: from env_config import build_env_for_script env = build_env_for_script(file, env_config) - execute_notebook(file, report=report, env=env) + execute_notebook( + file, + report=report, + env=env, + write_back=write_back, + retry_from_scripts=retry_from_scripts, + ) def execute_script(f, report=None, env=None, extra_args=None): @@ -619,19 +803,93 @@ def find_scripts_in_folder(directory: str) -> List[Path]: ) -def execute_scripts_in_folder(directory, no_run_list=None, report=None, skip_reasons=None, env_config=None): +def files_from_list(directory: str, list_path) -> List[Path]: + """ + Resolve an explicit allowlist of scripts to run, in the allowlist's own order. + + The opt-in counterpart to :func:`find_scripts_in_folder`. Blank lines and + ``#`` comments are ignored; every other line is a path relative to + ``directory``. Duplicated entries are collapsed to their first occurrence so + a repeated line does not run the script twice. + + Order is the file's order, deliberately — NOT ``find_scripts_in_folder``'s + simulator-first sort. An allowlist is hand-maintained, so its sequence is + the author's statement of what must run before what, and re-sorting it would + silently reorder a suite whose entries depend on an earlier one's output. + + Entries are returned whether or not they exist on disk; a missing one is + reported per-entry by the caller rather than aborting the run. + + Parameters + ---------- + directory + The directory the allowlist's entries are relative to. + list_path + The allowlist file (e.g. a workspace's ``smoke_tests.txt``). + + Returns + ------- + A list of paths to the scripts, in allowlist order. + + Raises + ------ + FileNotFoundError + If the allowlist file itself is missing. That is a configuration error, + not a script failure: silently running nothing would be a vacuously + green gate. + """ + list_path = Path(list_path) + if not list_path.exists(): + raise FileNotFoundError(f"no script list at {list_path}") + + root = Path.cwd() / directory + files: List[Path] = [] + seen = set() + for line in list_path.read_text().splitlines(): + line = line.strip() + if not line or line.startswith("#"): + continue + if line in seen: + continue + seen.add(line) + files.append(root / line) + return files + + +def execute_scripts_in_folder(directory, no_run_list=None, report=None, skip_reasons=None, env_config=None, files=None): + """ + Run the scripts under ``directory``, honouring ``no_run_list``. + + ``files`` selects the discovery model. Omitted (the default), scripts are + discovered recursively under ``directory`` — coverage is opt-out, and + excluding one is an entry in ``no_run.yaml``. Supplied (from + :func:`files_from_list`), exactly those entries run, in that order — + coverage is opt-in. + + ``no_run_list`` applies either way. An allowlisted script that is also + ``no_run``-listed is SKIPPED with its documented reason: an explicit + exclusion is the more specific statement of intent, and letting an + allowlist override it would resurrect a script that was deliberately + turned off. + """ no_run_list = no_run_list or [] # Infrastructure files — always skip, never report infra_skip = ["__init__", "README"] no_run_list.extend(infra_skip) - files = find_scripts_in_folder(directory) - print(f"Found {len(files)} scripts") + if files is None: + files = find_scripts_in_folder(directory) + print(f"Found {len(files)} scripts") + else: + print(f"Running {len(files)} listed scripts") for file in files: if file.stem in infra_skip: continue if should_skip(file, no_run_list): + # Checked BEFORE existence: an excluded script that has also been + # deleted is still excluded, and reporting it as a failure would + # contradict the exclusion. if report is not None: from result_collector import ScriptResult, Status reason = _find_skip_reason(file, no_run_list, skip_reasons or {}) @@ -640,6 +898,21 @@ def execute_scripts_in_folder(directory, no_run_list=None, report=None, skip_rea status=Status.SKIPPED, skip_reason=reason, )) + elif not file.exists(): + # Only reachable from an allowlist — discovery cannot yield a + # missing path. Report it and carry on: the runner's contract is to + # continue through failures, and a stale allowlist entry must not + # cost coverage of every entry after it. + print(f" {file} ... FAIL (listed but not found)") + if report is not None: + from result_collector import ScriptResult, Status + report.results.append(ScriptResult( + file=str(file), + status=Status.FAILED, + error_message=f"Listed in the script list but not found at {file}", + )) + else: + sys.exit(1) else: from env_config import build_env_for_script, args_for_script env = build_env_for_script(file, env_config) diff --git a/autohands/run.py b/autohands/run.py index 3130b35..9320607 100644 --- a/autohands/run.py +++ b/autohands/run.py @@ -28,6 +28,37 @@ default=None, help="Path to profile_smoke.yaml for per-script environment configuration", ) +parser.add_argument( + "--list", + dest="list_file", + type=str, + default=None, + help=( + "Path to a notebook list (e.g. a workspace's smoke_notebooks.txt). Given, " + "only the listed notebooks run, in the list's order (opt-in coverage); " + "omitted, notebooks are discovered recursively under DIRECTORY (opt-out " + "coverage). no_run.yaml applies either way." + ), +) +parser.add_argument( + "--no-write-back", + action="store_true", + help=( + "Execute a throwaway copy so the committed notebooks are left untouched. " + "What a PR smoke gate wants; the release/generation pipeline does not " + "pass it, because there the executed outputs are the product." + ), +) +parser.add_argument( + "--retry-from", + type=str, + default=None, + help=( + "Scripts directory (e.g. 'scripts'). On a genuine failure the notebook " + "is regenerated from its source .py and retried ONCE, recovering a stale " + "notebook whose script moved on. Timeouts are never retried." + ), +) args = parser.parse_args() @@ -95,6 +126,16 @@ from env_config import load_env_config env_config = load_env_config(env_config_path) + files = None + if args.list_file: + try: + files = build_util.files_from_list(directory, args.list_file) + except FileNotFoundError as e: + # A missing list is a configuration error, not a notebook failure. + # Running nothing and exiting 0 would be a vacuously green gate. + print(f"ERROR: {e}", file=sys.stderr) + sys.exit(1) + build_util.execute_notebooks_in_folder( no_run_list=no_run_list, visualise_dict=visualise_dict, @@ -102,6 +143,9 @@ report=report, skip_reasons=skip_reasons, env_config=env_config, + files=files, + write_back=not args.no_write_back, + retry_from_scripts=args.retry_from, ) if report is not None: diff --git a/autohands/run_python.py b/autohands/run_python.py index dfaab1d..10d3edb 100644 --- a/autohands/run_python.py +++ b/autohands/run_python.py @@ -22,6 +22,18 @@ default=None, help="Path to profile_smoke.yaml for per-script environment configuration", ) +parser.add_argument( + "--list", + dest="list_file", + type=str, + default=None, + help=( + "Path to a script list (e.g. a workspace's smoke_tests.txt). Given, only " + "the listed scripts run, in the list's order (opt-in coverage); omitted, " + "scripts are discovered recursively under DIRECTORY (opt-out coverage). " + "no_run.yaml applies either way." + ), +) args = parser.parse_args() @@ -73,12 +85,23 @@ from env_config import load_env_config env_config = load_env_config(env_config_path) + files = None + if args.list_file: + try: + files = build_util.files_from_list(directory, args.list_file) + except FileNotFoundError as e: + # A missing list is a configuration error, not a script failure. + # Running nothing and exiting 0 would be a vacuously green gate. + print(f"ERROR: {e}", file=sys.stderr) + sys.exit(1) + build_util.execute_scripts_in_folder( no_run_list=no_run_list, directory=directory, report=report, skip_reasons=skip_reasons, env_config=env_config, + files=files, ) if report is not None: diff --git a/bin/autohands b/bin/autohands index 635dd23..a2a2ec4 100755 --- a/bin/autohands +++ b/bin/autohands @@ -271,18 +271,31 @@ cmd_regenerate_navigator() { help_run() { cat <<'EOF' autohands run [--visualise] [--report-dir DIR] [--env-config FILE] + [--list FILE] [--no-write-back] [--retry-from DIR] Execute Jupyter notebooks in for , skipping files listed in the workspace's config/build/no_run.yaml (required). Arguments: - project Project name (e.g. autofit, autogalaxy, autolens) - directory Subdirectory of notebooks/ to execute - --visualise Only run notebooks listed in the workspace's - config/build/visualise_notebooks.yaml (selects nothing - if the workspace has no such file) - --report-dir Directory to write structured JSON results to - --env-config Path to env_vars.yaml for per-script environment config + project Project name (e.g. autofit, autogalaxy, autolens) + directory Subdirectory of notebooks/ to execute + --visualise Only run notebooks listed in the workspace's + config/build/visualise_notebooks.yaml (selects nothing + if the workspace has no such file) + --report-dir Directory to write structured JSON results to + --env-config Path to env_vars.yaml for per-script environment config + --list Notebook list (e.g. a workspace's smoke_notebooks.txt). + Given, only the listed notebooks run, in the list's order + (opt-in coverage); omitted, notebooks are discovered + recursively (opt-out). no_run.yaml applies either way, and + wins over the list. + --no-write-back Execute a throwaway copy, leaving the committed notebooks + untouched. What a PR smoke gate wants; the release pipeline + omits it, because there the executed outputs ARE the product. + --retry-from Scripts directory. On a genuine failure the notebook is + regenerated from its source .py and retried ONCE, recovering + a stale notebook whose script moved on. Timeouts are never + retried; a clean skip-guard exit is already a PASS. Run from the workspace root. EOF @@ -294,6 +307,7 @@ cmd_run() { help_run_python() { cat <<'EOF' autohands run_python [--report-dir DIR] [--env-config FILE] + [--list FILE] Execute Python scripts in for , skipping files listed in config/build/no_run.yaml. @@ -303,6 +317,10 @@ Arguments: directory Subdirectory of scripts/ to execute --report-dir Directory to write structured JSON results to --env-config Path to env_vars.yaml for per-script environment config + --list Script list (e.g. a workspace's smoke_tests.txt). Given, only + the listed scripts run, in the list's order (opt-in coverage); + omitted, scripts are discovered recursively under + (opt-out coverage). no_run.yaml applies either way, and wins. Run from the workspace root. EOF diff --git a/docs/internals.md b/docs/internals.md index 6261bc6..eb5741b 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -114,8 +114,8 @@ This workspace is often imported from `/mnt/c/...` and Codex may not be able to All scripts in `autohands/` are run from within a checked-out workspace directory (not from this repo root). They rely on `PYTHONPATH` including the PyAutoHands directory. -- **`run_python.py `** — Executes Python scripts in a workspace folder, skipping files listed in `config/no_run.yaml` -- **`run.py [--visualise]`** — Executes Jupyter notebooks in a workspace folder, skipping files in `config/no_run.yaml` +- **`run_python.py [--report-dir DIR] [--env-config FILE] [--list FILE]`** — Executes Python scripts in a workspace folder, skipping files listed in `config/no_run.yaml`. Coverage is opt-out by default (recursive discovery); `--list` switches it to opt-in, running exactly the entries of a script list such as a workspace's `smoke_tests.txt`, in that list's own order. `no_run.yaml` applies either way and wins over the list, so an explicitly excluded script stays excluded. A missing list file is an error, never an empty run. +- **`run.py [--visualise] [--report-dir DIR] [--env-config FILE] [--list FILE] [--no-write-back] [--retry-from DIR]`** — Executes Jupyter notebooks in a workspace folder, skipping files in `config/no_run.yaml`. `--list` switches coverage from opt-out discovery to opt-in (a workspace's `smoke_notebooks.txt`, in that list's order); `no_run.yaml` still wins over the list. `--no-write-back` executes a throwaway copy so the committed notebooks stay clean (a PR gate wants this; the release pipeline does not, since there the outputs are the product). `--retry-from ` regenerates a failing notebook from its source `.py` and retries ONCE — the stale-notebook recovery; timeouts are never retried. - **`generate.py `** — Converts Python scripts in `scripts/` to `.ipynb` notebooks in `notebooks/`, run from within the workspace root - **`generate_markdown.py [--only ] [--optimize-only]`** — Renders the curated scripts listed in the workspace's `config/build/markdown_examples.yaml` to **executed** markdown pages with output images under `markdown/`, plus an index, committed so examples are readable on GitHub. Manual / at-release only, never per-commit; refuses `PYAUTO_TEST_MODE` (truncated searches make wrong images — model-fit reruns instead resume from the completed `output/` cache); never renders `features/` scripts; restores tracked files a script modifies (e.g. simulators rewriting `dataset/`). Extracted figures are optimized on the way out, but only for the render in progress — `--optimize-only` renders nothing and puts already-committed `markdown/**/_files/` PNGs through the same optimizer, for pages built before it shipped. Rules and rationale in the module docstring. - **`script_matrix.py [project2 ...]`** — Outputs a JSON matrix of `{name, directory}` pairs for GitHub Actions matrix strategy @@ -166,11 +166,9 @@ would require regenerating and committing notebooks across every workspace. **Propagation.** Each workspace repo carries its own `.github/scripts/run_smoke.py` (no template sync — PyAutoHeart's reusable -`smoke-tests.yml` deliberately leaves the runner in the workspace), and that -copy has its own `execute_notebook` used by the PR smoke gate. It already -imports from PyAutoHands (`env_config`, `build_util.py_to_notebook`, with -`PyAutoHands/autohands` on `PYTHONPATH`), so adoption is a two-line change per -workspace: +`smoke-tests.yml` deliberately leaves the runner in the workspace). The three +notebook-capable copies each had their own `execute_notebook`, so the skip guard +had to be adopted per repo — a two-line change, applied 2026-07-25: ```python from build_util import is_clean_skip_exit @@ -180,15 +178,57 @@ if rc != 0 and is_clean_skip_exit(output): rc = 0 ``` -Workspaces carrying a `run_smoke.py` copy: `autofit_workspace`, -`autogalaxy_workspace`, `autolens_workspace`, `autofit_workspace_test`, -`autogalaxy_workspace_test`, `autolens_workspace_test`, -`autocti_workspace_test`, `HowToGalaxy`, `HowToLens` (nine copies, five distinct -revisions — they have drifted). Only the three **user-facing workspaces** -(`autofit_workspace`, `autogalaxy_workspace`, `autolens_workspace`) have a -notebook execution leg, so only those three needed the edit — applied -2026-07-25. The HowTo and `*_test` runners are scripts-only (no -`smoke_notebooks.txt`, no nbconvert path) and require nothing. +### The `run_smoke.py` copies + +**Ten copies in three variants** (measured 2026-08-24 across every repo's +`main`). They are three structurally different programs, not revisions of one: + +| Variant | Repos | Lines | Notebook leg | Coverage | +|---|---|---|---|---| +| **workspace** | `autofit_workspace`, `autogalaxy_workspace`, `autolens_workspace` | 356 | yes | opt-in (`smoke_tests.txt` + `smoke_notebooks.txt`) | +| **workspace_test** | `autofit_workspace_test`, `autogalaxy_workspace_test`, `autolens_workspace_test`, `autocti_workspace_test` | 198 | no | opt-in (`smoke_tests.txt`) | +| **HowTo** | `HowToLens`, `HowToGalaxy`, `HowToFit` | 75 | no | opt-out (`no_run.yaml`) | + +There is **no live drift inside any variant**: the three workspace copies are +byte-identical, the four `workspace_test` copies differ only in two docstring +lines, and the three HowTo copies differ only in their `PROJECT` constant. That +state was reached by three manual N-repo sweeps, not by a sync mechanism — +#185 (env resolution collapsed onto `env_config`), #226/#227 (`timeout_for` and +`kill_group` promoted here, so all ten honour `BUILD_SCRIPT_TIMEOUT` and the +process-group kill), and the jupyter-guard fix. Only the skip guard is +notebook-specific: `is_clean_skip_exit` belongs in exactly the three +notebook-capable copies, since the other seven never shell out to `jupyter`. + +**Direction of travel: delegation.** The HowTo tier is the target shape — 75 +lines of `PROJECT` plus a `subprocess.run` into `run_python.py` — and it needed +none of those three sweeps, precisely because it holds no logic. The obstacle +was never behaviour but *discovery model*: `run_python.py` was opt-out only, +while the other two variants are opt-in allowlists. `--list ` closes that +gap for the script leg, so the `workspace_test` variant can collapse to a HowTo- +shaped delegator. The notebook leg followed in the same change: +`run.py` gained the matching `--list`, plus `--no-write-back` and +`--retry-from`, so the `workspace` variant can collapse too. + +Two of the three notebook behaviours that variant held needed real promotion, +and one dissolved on inspection: + +- **`--no-write-back` (promoted).** `run_notebook.py` writes executed outputs + back in place — right for generation, where the outputs are the product, but + wrong for a PR gate, which must not dirty the tree it is testing. The flag + executes a throwaway copy instead. The kernel cwd is pinned to the repo root + either way, so this supersedes the workspace copy's staged-copy-at-root trick + rather than porting it. +- **`--retry-from` (promoted).** One regenerate-from-source retry recovers a + *stale* notebook whose script moved on. Deliberately narrow: a TIMEOUT is + never retried (a second full cap for the same answer), a clean skip-guard exit + is already a PASS and never reaches it, and the retry's verdict REPLACES the + first attempt's so one notebook contributes one result. +- **`JUPYTER_MISSING_RC` (not needed).** That guard exists because the workspace + copy shelled out to a bare `jupyter` binary, so an absent toolchain raised + `FileNotFoundError` out of `main()` and aborted the run with no summary line. + `execute_notebook` invokes `sys.executable run_notebook.py`, which always + exists, so a missing toolchain is an ordinary non-zero exit — one FAIL, run + continues. The failure mode is structurally absent here. ### Google Colab architecture diff --git a/tests/test_notebook_delegation.py b/tests/test_notebook_delegation.py new file mode 100644 index 0000000..9ec2c23 --- /dev/null +++ b/tests/test_notebook_delegation.py @@ -0,0 +1,219 @@ +""" +Cover the notebook-leg behaviours promoted out of the workspace `run_smoke.py`. + +These are the behaviours the vendored 356-line workspace runner held that +`build_util` did not, and that a collapsed delegator must not lose: + +* the committed `notebooks/` tree stays clean when a smoke gate runs it; +* a genuine failure gets ONE regenerate-from-source retry (stale-notebook + recovery), and the retry's verdict replaces the first attempt's; +* a TIMEOUT is never retried; +* a clean skip-guard exit is a PASS and never reaches the retry. +""" + +import sys +from pathlib import Path + +import pytest + +PROJECT_ROOT = Path(__file__).parent.parent +AUTOHANDS_DIR = PROJECT_ROOT / "autohands" +sys.path.insert(0, str(AUTOHANDS_DIR)) + +import build_util # noqa: E402 +from build_util import execute_notebook # noqa: E402 +from result_collector import RunReport, Status # noqa: E402 + +nbformat = pytest.importorskip("nbformat") +pytest.importorskip("nbconvert") + + +def _write_notebook(path: Path, source: str) -> Path: + nb = nbformat.v4.new_notebook() + nb.cells = [nbformat.v4.new_code_cell(source)] + path.parent.mkdir(parents=True, exist_ok=True) + nbformat.write(nb, path) + return path + + +@pytest.fixture +def workspace(tmp_path, monkeypatch): + (tmp_path / "notebooks").mkdir() + (tmp_path / "scripts").mkdir() + monkeypatch.chdir(tmp_path) + return tmp_path + + +def _report(): + return RunReport(project="p", directory="notebooks", run_type="notebook") + + +class TestWriteBack: + """`write_back` decides whether the committed notebook is modified.""" + + def test_write_back_false_leaves_the_notebook_untouched(self, workspace): + """ + A PR smoke gate must not dirty the tree it is testing. + + run_notebook.py writes executed outputs back in place, which is right + for generation but wrong for a gate — an unchanged worktree after a + smoke run is the invariant the workspace copy protected by executing a + throwaway copy. + """ + nb = _write_notebook(workspace / "notebooks" / "n.ipynb", "x = 1 + 1") + before = nb.read_bytes() + + status = execute_notebook(nb, report=_report(), write_back=False) + + assert status == "passed" + assert nb.read_bytes() == before + + def test_write_back_true_records_outputs(self, workspace): + """The generation/release contract is unchanged: outputs are kept.""" + nb = _write_notebook(workspace / "notebooks" / "n.ipynb", "print('hi')") + before = nb.read_bytes() + + status = execute_notebook(nb, report=_report(), write_back=True) + + assert status == "passed" + assert nb.read_bytes() != before + + +class TestRegenerateAndRetry: + """One retry from the source script, and only where it can help.""" + + def test_stale_notebook_is_regenerated_and_passes(self, workspace): + """ + The recovery case: the script moved on, the committed notebook did not. + + The notebook fails; its source script is healthy; regenerating and + retrying turns the entry green with exactly one recorded result. + """ + _write_notebook(workspace / "notebooks" / "n.ipynb", "raise RuntimeError('stale')") + (workspace / "scripts" / "n.py").write_text("x = 1 + 1\n") + report = _report() + + status = execute_notebook( + workspace / "notebooks" / "n.ipynb", + report=report, + write_back=False, + retry_from_scripts=workspace / "scripts", + ) + + assert status == "passed" + # The retry REPLACES the first attempt — one notebook, one result. + assert len(report.results) == 1 + assert report.results[0].status == Status.PASSED + # A temp path never leaks into the report. + assert report.results[0].file.endswith("notebooks/n.ipynb") + + def test_genuinely_broken_notebook_still_fails_after_the_retry(self, workspace): + """A real bug survives regeneration, and stays one FAIL.""" + _write_notebook(workspace / "notebooks" / "n.ipynb", "raise RuntimeError('boom')") + (workspace / "scripts" / "n.py").write_text("raise RuntimeError('boom')\n") + report = _report() + + status = execute_notebook( + workspace / "notebooks" / "n.ipynb", + report=report, + write_back=False, + retry_from_scripts=workspace / "scripts", + ) + + assert status == "failed" + assert len(report.results) == 1 + assert report.results[0].status == Status.FAILED + + def test_missing_source_script_leaves_the_first_failure_standing(self, workspace): + """ + No source to regenerate from: the recovery was unavailable, not the + notebook fixed. The original FAIL must stand rather than vanish. + """ + _write_notebook(workspace / "notebooks" / "n.ipynb", "raise RuntimeError('boom')") + report = _report() + + status = execute_notebook( + workspace / "notebooks" / "n.ipynb", + report=report, + write_back=False, + retry_from_scripts=workspace / "scripts", + ) + + assert status == "failed" + assert len(report.results) == 1 + assert report.results[0].status == Status.FAILED + + def test_no_retry_when_not_asked(self, workspace): + """Without retry_from_scripts the behaviour is exactly as before.""" + _write_notebook(workspace / "notebooks" / "n.ipynb", "raise RuntimeError('boom')") + (workspace / "scripts" / "n.py").write_text("x = 1\n") + report = _report() + + status = execute_notebook( + workspace / "notebooks" / "n.ipynb", report=report, write_back=False + ) + + assert status == "failed" + + +class TestRetryIsNarrow: + """A retry is only ever spent where it can change the answer.""" + + def test_timeout_is_never_retried(self, workspace, monkeypatch): + """ + Retrying a timeout burns a second full cap for the same result, + doubling the slowest entry's cost. + """ + _write_notebook(workspace / "notebooks" / "n.ipynb", "x = 1") + (workspace / "scripts" / "n.py").write_text("x = 1\n") + monkeypatch.setenv("BUILD_SCRIPT_TIMEOUT", "0") + + calls = [] + real = build_util._run_notebook_once + + def counting(*args, **kwargs): + calls.append(args[0]) + return "timeout" + + monkeypatch.setattr(build_util, "_run_notebook_once", counting) + status = execute_notebook( + workspace / "notebooks" / "n.ipynb", + report=_report(), + write_back=False, + retry_from_scripts=workspace / "scripts", + ) + + assert status == "timeout" + assert len(calls) == 1, "a timeout must not trigger the regenerate-and-retry" + assert real is not None # the real implementation is still importable + + def test_clean_skip_exit_passes_without_a_retry(self, workspace, monkeypatch): + """ + The optional-dependency skip guard is already a PASS, so it must never + reach the retry path — regenerating it would be pure waste. + """ + _write_notebook( + workspace / "notebooks" / "n.ipynb", + "import sys\nprint('skipping')\nsys.exit(0)", + ) + (workspace / "scripts" / "n.py").write_text("x = 1\n") + report = _report() + + calls = [] + real = build_util._run_notebook_once + + def counting(*args, **kwargs): + calls.append(args[0]) + return real(*args, **kwargs) + + monkeypatch.setattr(build_util, "_run_notebook_once", counting) + status = execute_notebook( + workspace / "notebooks" / "n.ipynb", + report=report, + write_back=False, + retry_from_scripts=workspace / "scripts", + ) + + assert status == "passed" + assert len(calls) == 1 + assert report.results[0].status == Status.PASSED diff --git a/tests/test_script_list.py b/tests/test_script_list.py new file mode 100644 index 0000000..400744a --- /dev/null +++ b/tests/test_script_list.py @@ -0,0 +1,179 @@ +""" +Cover the opt-in allowlist mode of the shared script runner. + +`--list` exists so an opt-in workspace (one with a hand-maintained +`smoke_tests.txt`) can use the same runner as the opt-out HowTo repos, instead +of vendoring its own copy of the loop. These tests pin the four behaviours the +vendored copies relied on, so a collapsed workspace runner cannot lose them. +""" + +import sys +from pathlib import Path + +import pytest + +PROJECT_ROOT = Path(__file__).parent.parent +AUTOHANDS_DIR = PROJECT_ROOT / "autohands" +sys.path.insert(0, str(AUTOHANDS_DIR)) + +from build_util import execute_scripts_in_folder, files_from_list # noqa: E402 +from result_collector import RunReport, Status # noqa: E402 + + +@pytest.fixture +def workspace(tmp_path, monkeypatch): + """A minimal workspace: scripts/ plus a place to write a script list.""" + scripts = tmp_path / "scripts" + (scripts / "sub_directory").mkdir(parents=True) + for rel in ("top_level.py", "a.py", "sub_directory/sub.py"): + (scripts / rel).write_text("") + (scripts / "simulators").mkdir() + (scripts / "simulators" / "simulator_script.py").write_text("") + monkeypatch.chdir(tmp_path) + return tmp_path + + +def _rel(paths, root): + return [str(Path(p).relative_to(root / "scripts")) for p in paths] + + +def test_allowlist_is_honoured(workspace): + """Only listed scripts come back — the three unlisted ones do not.""" + listing = workspace / "smoke_tests.txt" + listing.write_text("top_level.py\nsub_directory/sub.py\n") + + files = files_from_list("scripts", listing) + + assert _rel(files, workspace) == ["top_level.py", "sub_directory/sub.py"] + + +def test_allowlist_order_is_the_files_order(workspace): + """ + The list's sequence survives, rather than being re-sorted. + + `find_scripts_in_folder` deliberately sorts simulators first; an allowlist + must NOT, because a hand-maintained suite may depend on an earlier entry's + output. Listing a simulator last therefore keeps it last. + """ + listing = workspace / "smoke_tests.txt" + listing.write_text("top_level.py\na.py\nsimulators/simulator_script.py\n") + + files = files_from_list("scripts", listing) + + assert _rel(files, workspace) == [ + "top_level.py", + "a.py", + "simulators/simulator_script.py", + ] + + +def test_blank_lines_comments_and_duplicates_are_dropped(workspace): + listing = workspace / "smoke_tests.txt" + listing.write_text( + "# a comment\n" + "\n" + "top_level.py\n" + " \n" + " a.py \n" + "# another comment\n" + "top_level.py\n" # duplicate: must not run twice + ) + + files = files_from_list("scripts", listing) + + assert _rel(files, workspace) == ["top_level.py", "a.py"] + + +def test_missing_list_file_is_an_error_not_an_empty_run(workspace): + """ + A missing list must not resolve to "nothing to run". + + Returning an empty list would let the caller exit 0 having tested nothing — + the vacuously-green-gate failure mode. + """ + with pytest.raises(FileNotFoundError, match="no script list at"): + files_from_list("scripts", workspace / "does_not_exist.txt") + + +def test_no_run_wins_over_the_allowlist(workspace, capsys): + """ + An allowlisted script that is also no_run-listed is SKIPPED, with its reason. + + The explicit exclusion is the more specific statement of intent; letting the + allowlist override it would resurrect a script someone deliberately turned off. + """ + listing = workspace / "smoke_tests.txt" + listing.write_text("top_level.py\na.py\n") + report = RunReport(project="p", directory="scripts", run_type="script") + + execute_scripts_in_folder( + directory="scripts", + no_run_list=["a"], + report=report, + skip_reasons={"a": "deliberately off"}, + files=files_from_list("scripts", listing), + ) + + by_name = {Path(r.file).name: r for r in report.results} + assert by_name["a.py"].status == Status.SKIPPED + assert by_name["a.py"].skip_reason == "deliberately off" + + +def test_listed_but_missing_entry_fails_without_stopping_the_run(workspace): + """ + A stale allowlist entry is one FAIL, not an abort. + + The runner's contract is to continue through failures; aborting would cost + coverage of every entry after the stale one and print no summary. + """ + listing = workspace / "smoke_tests.txt" + listing.write_text("gone.py\ntop_level.py\n") + report = RunReport(project="p", directory="scripts", run_type="script") + + execute_scripts_in_folder( + directory="scripts", + no_run_list=[], + report=report, + files=files_from_list("scripts", listing), + ) + + by_name = {Path(r.file).name: r for r in report.results} + assert by_name["gone.py"].status == Status.FAILED + assert "not found" in by_name["gone.py"].error_message + # The entry AFTER the stale one still ran. + assert "top_level.py" in by_name + + +def test_absent_flag_leaves_discovery_untouched(workspace): + """The opt-out path is unchanged when no list is passed.""" + report = RunReport(project="p", directory="scripts", run_type="script") + execute_scripts_in_folder(directory="scripts", no_run_list=[], report=report) + + assert len(report.results) == 4 + # Discovery's simulator-first ordering still applies here. + assert Path(report.results[0].file).name == "simulator_script.py" + + +def test_no_run_wins_even_when_the_listed_file_is_missing(workspace): + """ + An excluded script that has also been deleted is SKIPPED, not FAILED. + + Pins the check order: reporting a failure for a script someone deliberately + turned off would contradict the exclusion, and would redden a gate over a + file nobody intends to run. + """ + listing = workspace / "smoke_tests.txt" + listing.write_text("deleted_and_excluded.py\ntop_level.py\n") + report = RunReport(project="p", directory="scripts", run_type="script") + + execute_scripts_in_folder( + directory="scripts", + no_run_list=["deleted_and_excluded"], + report=report, + skip_reasons={"deleted_and_excluded": "retired"}, + files=files_from_list("scripts", listing), + ) + + by_name = {Path(r.file).name: r for r in report.results} + assert by_name["deleted_and_excluded.py"].status == Status.SKIPPED + assert by_name["deleted_and_excluded.py"].skip_reason == "retired"