From 7d5d357c578e08a09e22d1171e86226f3d8e65f1 Mon Sep 17 00:00:00 2001 From: Jammy2211 Date: Tue, 28 Jul 2026 13:46:30 +0100 Subject: [PATCH] fix: run notebooks with the kernel cwd pinned to the workspace root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `jupyter nbconvert --execute` starts the kernel in the notebook's own directory, not the directory nbconvert was launched from. The workspaces document the opposite ("scripts are run from the repository root so relative paths to dataset/ and output/ resolve") and their auto-simulate guards shell out to root-relative simulator paths, so every auto-simulating notebook failed with exit 2 ("can't open file") — the largest contributor to the red run_notebooks shards. nbconvert exposes no CLI flag for the kernel's working directory; the only knob is resources['metadata']['path'], which nbclient turns into the kernel cwd, and that is reachable only from the Python API. So execute_notebook now subprocesses a new autohands/run_notebook.py which sets it. The subprocess boundary is kept deliberately: process isolation, BUILD_SCRIPT_TIMEOUT and the per-notebook environment are all unchanged — only the kernel's cwd moves. run_notebook.py leaves CellExecutionError uncaught on purpose. Python's own traceback then reproduces the exact shape build_util.is_clean_skip_exit parses (a CellExecutionError marker whose last line is `SystemExit: 0`) to tell an intentional optional-dependency skip from a real failure. Verified: sys.exit(0) -> True, sys.exit(1) -> False, ValueError -> False. Two regression tests lock in the kernel cwd and the root-relative-subprocess case. Full suite green (236). Closes #204 Co-Authored-By: Claude Opus 5 --- autohands/build_util.py | 13 ++++- autohands/run_notebook.py | 71 +++++++++++++++++++++++++ tests/test_run_notebook_cwd.py | 97 ++++++++++++++++++++++++++++++++++ 3 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 autohands/run_notebook.py create mode 100644 tests/test_run_notebook_cwd.py diff --git a/autohands/build_util.py b/autohands/build_util.py index 4c718d0b..e652758c 100644 --- a/autohands/build_util.py +++ b/autohands/build_util.py @@ -225,8 +225,19 @@ def execute_notebook(f, report=None, env=None): # stderr is always captured so a clean `sys.exit(0)` skip guard can be # told apart from a genuine cell failure (is_clean_skip_exit); stdout # keeps streaming live unless the report collector wants it. + # Run via run_notebook.py rather than `jupyter nbconvert --execute`: + # nbconvert starts the kernel in the notebook's own directory, but the + # workspaces document (and their auto-simulate guards require) execution + # 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. subprocess.run( - ["jupyter", "nbconvert", "--to", "notebook", "--execute", "--output", f, f], + [ + sys.executable, + str(Path(__file__).parent / "run_notebook.py"), + str(f), + str(Path.cwd()), + ], check=True, timeout=TIMEOUT_SECS, stdout=subprocess.PIPE if report is not None else None, diff --git a/autohands/run_notebook.py b/autohands/run_notebook.py new file mode 100644 index 00000000..96d1e09c --- /dev/null +++ b/autohands/run_notebook.py @@ -0,0 +1,71 @@ +""" +Execute one notebook with the kernel's working directory pinned to the +workspace root. + +Why this exists +--------------- +``jupyter nbconvert --execute`` starts the kernel in the **notebook's own +directory**, not in the directory nbconvert was launched from. The workspaces +document the opposite convention (autolens_workspace/AGENTS.md: "Scripts are +run from the repository root so relative paths to ``dataset/`` and ``output/`` +resolve correctly"), and the auto-simulate guards rely on it:: + + if al.util.dataset.should_simulate(str(dataset_path)): + subprocess.run([sys.executable, "scripts/imaging/simulator.py"], check=True) + +That root-relative path resolves from a script run at the root and cannot +resolve from a notebook run in ``notebooks//`` — the subprocess exits 2 +("can't open file") and every auto-simulating notebook fails. + +nbconvert exposes **no CLI flag** for the kernel's working directory. The knob +is ``resources['metadata']['path']``, which nbclient turns into the kernel's +``cwd`` (see ``nbclient/client.py``, ``_async_start_new_kernel``), and that is +reachable only from the Python API. Hence this module: ``build_util`` still +runs it as a **subprocess**, so process isolation, the ``BUILD_SCRIPT_TIMEOUT`` +and the per-notebook environment are all unchanged — only the kernel's cwd +moves. + +Failure contract +---------------- +On a cell error the ``CellExecutionError`` is deliberately left **uncaught** so +Python prints its own traceback to stderr. That output contains the +``CellExecutionError`` marker and ends with the failing cell's terminal +``: `` line, which is exactly what +``build_util.is_clean_skip_exit`` parses to tell an intentional +``sys.exit(0)`` skip from a genuine failure. Do not wrap it in a handler that +reformats the message, or that skip detection breaks. + +Usage:: + + python run_notebook.py +""" + +import sys +from pathlib import Path + +import nbformat +from nbconvert.preprocessors import ExecutePreprocessor + + +def run(notebook_path: str, root: str) -> None: + nb_path = Path(notebook_path) + nb = nbformat.read(nb_path, as_version=4) + + ep = ExecutePreprocessor(timeout=None, kernel_name="python3") + + try: + # The whole point: pin the kernel's cwd to the workspace root rather + # than letting it default to the notebook's own directory. + ep.preprocess(nb, {"metadata": {"path": str(root)}}) + finally: + # Write back even on failure so a partially-executed notebook keeps its + # outputs, matching `nbconvert --output ` (which writes in place). + nbformat.write(nb, nb_path) + + +if __name__ == "__main__": + if len(sys.argv) != 3: + print(f"usage: {Path(__file__).name} ", + file=sys.stderr) + sys.exit(2) + run(sys.argv[1], sys.argv[2]) diff --git a/tests/test_run_notebook_cwd.py b/tests/test_run_notebook_cwd.py new file mode 100644 index 00000000..36c6c89c --- /dev/null +++ b/tests/test_run_notebook_cwd.py @@ -0,0 +1,97 @@ +"""Regression tests for the notebook kernel's working directory. + +`jupyter nbconvert --execute` starts the kernel in the **notebook's own +directory**, but the workspaces document execution from the repo root and their +auto-simulate guards shell out to root-relative simulator paths:: + + if al.util.dataset.should_simulate(str(dataset_path)): + subprocess.run([sys.executable, "scripts/imaging/simulator.py"], check=True) + +Under nbconvert that subprocess exits 2 ("can't open file") and every +auto-simulating notebook fails. `autohands/run_notebook.py` fixes this by +pinning the kernel cwd to the workspace root via +`resources['metadata']['path']`. + +These tests lock in the two properties that matter: the kernel really runs at +the given root (not the notebook's directory), and a root-relative subprocess +therefore resolves. They are skipped when a Jupyter kernel is unavailable. +""" + +import json +import subprocess +import sys +from pathlib import Path + +import pytest + +PROJECT_ROOT = Path(__file__).parent.parent +RUNNER = PROJECT_ROOT / "autohands" / "run_notebook.py" + +jupyter = pytest.importorskip("nbformat") +pytest.importorskip("nbconvert") + + +def _write_nb(path: Path, source: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + nb = { + "cells": [{ + "cell_type": "code", + "execution_count": None, + "metadata": {}, + "outputs": [], + "source": source, + }], + "metadata": {"kernelspec": { + "display_name": "Python 3", "language": "python", "name": "python3", + }}, + "nbformat": 4, + "nbformat_minor": 5, + } + path.write_text(json.dumps(nb)) + + +def _outputs(path: Path) -> str: + nb = json.loads(path.read_text()) + return "".join( + "".join(o.get("text", "")) for o in nb["cells"][0].get("outputs", []) + ) + + +def _run(nb_path: Path, root: Path): + return subprocess.run( + [sys.executable, str(RUNNER), str(nb_path), str(root)], + capture_output=True, text=True, timeout=300, + ) + + +def test_kernel_cwd_is_the_given_root_not_the_notebook_dir(tmp_path): + """The whole point of the runner: cwd is the root, not notebooks/sub/.""" + nb = tmp_path / "notebooks" / "sub" / "cwd.ipynb" + _write_nb(nb, 'import os\nprint("CWD:", os.getcwd())') + + result = _run(nb, tmp_path) + + assert result.returncode == 0, result.stderr + out = _outputs(nb) + assert f"CWD: {tmp_path}" in out + # the failure mode this exists to prevent + assert str(nb.parent) not in out + + +def test_root_relative_subprocess_resolves(tmp_path): + """The auto-simulate guard shape: a root-relative script path must run.""" + (tmp_path / "scripts").mkdir(parents=True, exist_ok=True) + (tmp_path / "scripts" / "sim.py").write_text('print("simulated")') + + nb = tmp_path / "notebooks" / "deep" / "guard.ipynb" + _write_nb( + nb, + "import subprocess, sys\n" + 'r = subprocess.run([sys.executable, "scripts/sim.py"], check=True)\n' + 'print("GUARD_OK")', + ) + + result = _run(nb, tmp_path) + + assert result.returncode == 0, result.stderr + assert "GUARD_OK" in _outputs(nb)