From eb2c8fdee180e25ba368136edb7014845345e25a Mon Sep 17 00:00:00 2001 From: Chao Wang <26245345+ChaoWao@users.noreply.github.com> Date: Sun, 26 Jul 2026 21:53:51 -0700 Subject: [PATCH] Fix: refuse a _task_interface built from a different revision of the tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An editable install pins the compiled extension at install time (`editable.rebuild = false`) while `python/simpler/*.py` is read live. Switching branches or rebasing therefore moves the Python out from under a fixed binary, nothing rebuilds, and a changed struct layout makes attributes read as 0 with no error anywhere. That is not hypothetical. A worktree installed before #1309 (which dropped `block_dim` from `CallConfig`) and then branched onto a main containing it read `aicpu_thread_num` as 0, and the whole onboard L2 suite failed with `launch_aicpu_num (0) must be in range [1, 4]` — a plausible-looking runtime rejection that reads as a product bug. It cost a full bisect, and an A/B against main "reproduced" it because both arms shared the same stale binary. The same skew hit again a few hours later as `AttributeError: run_stream_set_create_count` after a rebase across #1464. `_task_interface` now records the commit it was built from, and `simpler.task_interface` compares it against the working tree at import, raising with the one command that fixes it. Loud beats silent here: the alternative is not an error, it is wrong values. A warning would also have been the wrong choice — pytest relegates import-time warnings to its end-of-run summary, and in the failure above the same warning would have appeared in *both* arms of the A/B and been dismissed as noise. A *missing* stamp raises too, rather than being treated as "cannot tell". The attribute is absent only on an extension compiled before it existed, which in a checkout new enough to run the check is by definition a different revision — and is the state of every already-installed worktree the day this lands. Keyed on git HEAD, matching `RuntimeBuilder._build_cache_stamp` — mtimes do not survive a branch switch, which is why that stamp is git-based too. Deliberately the whole HEAD rather than a subset: excluding paths means maintaining a second "what cannot affect the build" list beside the one CI already keeps, and the guard exists precisely so correctness does not rest on such judgments. It costs little — 120 of the last 200 commits touch the ABI surface anyway, so most of the reinstalls it forces were owed regardless and merely became visible; the rest cost one ~20 s reinstall. Inert outside a source tree: a wheel has no `.git` to compare against, and a build made without git carries an empty stamp. The rebuild table gained the trigger it was missing and no longer contradicts itself. It was framed as "what did you change", so it had no row for the case where you changed nothing and the tree moved — the one that bites, and the one where verifying that `import simpler` resolves into your worktree gives false confidence. Its "no rebuild needed" rows now say what is actually true: nothing to recompile, but the guard keys on HEAD, so a commit still needs a reinstall before the next import. Co-Authored-By: Claude Opus 5 (1M context) --- docs/developer-guide.md | 23 +++++++++- python/bindings/CMakeLists.txt | 17 ++++++++ python/bindings/task_interface.cpp | 10 +++++ python/simpler/task_interface.py | 67 ++++++++++++++++++++++++++++++ 4 files changed, 115 insertions(+), 2 deletions(-) diff --git a/docs/developer-guide.md b/docs/developer-guide.md index 5d406c5cdf..60cfd4ec17 100644 --- a/docs/developer-guide.md +++ b/docs/developer-guide.md @@ -183,10 +183,29 @@ This builds the nanobind `_task_interface` extension **and** pre-builds all runt | First time / clean checkout | `pip install --no-build-isolation -e .` | | Runtime C++ source (`src/{arch}/runtime/`, `src/{arch}/platform/`) | Re-run `pip install --no-build-isolation -e .` (or `.` for a non-editable install). Incremental via the cmake caches under `build/cache/` (~1-2s). | | Nanobind bindings (`python/bindings/`) | Re-run `pip install --no-build-isolation -e .` (no rebuild-on-import; `editable.rebuild = false`) | -| Python-only code (`python/*.py`, `simpler_setup/*.py`) | No rebuild needed (editable install) | -| Examples / kernels (`examples/{arch}/`, `tests/st/`) | No rebuild needed, just re-run | +| Nothing — but `HEAD` moved (branch switch, rebase, pull, **or your own commit**) | Re-run `pip install ...`. You may have changed no compiled file, yet the tree moved out from under a binary frozen at install time. `simpler.task_interface` refuses to import on this skew (see below) rather than let struct fields read as 0. | +| Python-only code (`python/*.py`, `simpler_setup/*.py`) | Nothing to recompile (editable install) — but see the row above: once you *commit* it, the import guard keys on `HEAD`, so reinstall before the next import. | +| Examples / kernels (`examples/{arch}/`, `tests/st/`) | Nothing to recompile, just re-run — same commit caveat as the row above | | `pto_isa.pin` changed | Re-run `pip install`. The cmake cache stamp and the `host_runtime` ccache key include the pinned PTO-ISA commit for a2a3 onboard (and a5 onboard when the SDMA overlay is enabled), so a pin bump invalidates stale runtime objects automatically. | +`_task_interface` records the commit it was compiled from, and +`simpler.task_interface` compares it against the working tree at import, +raising when they differ. It is git-based for the same reason +`RuntimeBuilder._build_cache_stamp` is — mtimes do not survive a branch switch — +and deliberately keyed on the whole HEAD rather than on the binding sources +alone: a root `CMakeLists.txt`, `pyproject.toml` or nanobind change alters the +extension without touching `python/bindings/`, and under-detecting here means +silently wrong values. The cost of over-detecting is one ~20 s reinstall, and +120 of the last 200 commits touched the ABI surface anyway — so most of those +reinstalls are ones you owed regardless, merely made visible. + +Verifying that `import simpler` resolves into your worktree does **not** cover +this: under an editable install that only proves the *Python* is live, while the +compiled extension stays at whatever `pip install` produced. + +The check is inert outside a source tree — a wheel has no `.git` to compare +against, and a build made without git carries an empty stamp. + A `pto_isa.pin` bump changes the SDMA headers embedded by `host_runtime.so`. Install-time runtime builds and run-time kernel compilation both read `pto_isa.pin`; use a different ISA revision by updating that file. diff --git a/python/bindings/CMakeLists.txt b/python/bindings/CMakeLists.txt index 4aa4d98d09..75d24dff82 100644 --- a/python/bindings/CMakeLists.txt +++ b/python/bindings/CMakeLists.txt @@ -30,8 +30,25 @@ set(HIERARCHICAL_SOURCES ${HIERARCHICAL_SRC}/worker.cpp ) +# Stamp the source commit into the module so `simpler.task_interface` can refuse +# to run against a binding built from a different tree. Same notion of "which +# sources is this built from" as RuntimeBuilder._build_cache_stamp, and git for +# the same reason: mtimes do not survive a branch switch. Empty when git is +# unavailable (release tarball, no .git), which disables the check. +execute_process( + COMMAND git rev-parse HEAD + WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} + OUTPUT_VARIABLE SIMPLER_BUILD_COMMIT + OUTPUT_STRIP_TRAILING_WHITESPACE + ERROR_QUIET +) + nanobind_add_module(_task_interface ${BINDING_SOURCES} ${HIERARCHICAL_SOURCES}) +target_compile_definitions(_task_interface PRIVATE + SIMPLER_BUILD_COMMIT="${SIMPLER_BUILD_COMMIT}" +) + target_sources(_task_interface PRIVATE ${CMAKE_SOURCE_DIR}/src/common/worker/chip_worker.cpp # Host-side assert_impl / get_stacktrace for the unified Tensor's diff --git a/python/bindings/task_interface.cpp b/python/bindings/task_interface.cpp index 20bab31309..16c109d92d 100644 --- a/python/bindings/task_interface.cpp +++ b/python/bindings/task_interface.cpp @@ -562,9 +562,19 @@ void append_cleanup_error(std::string &cleanup_error, const std::string &message // Module definition // ============================================================================ +#ifndef SIMPLER_BUILD_COMMIT +#define SIMPLER_BUILD_COMMIT "" +#endif + NB_MODULE(_task_interface, m) { m.doc() = "Nanobind bindings for task_interface (DataType, Tensor, TaskArgs variants)"; + // Source commit this extension was compiled from; "" when git was + // unavailable at build time. simpler.task_interface compares it against the + // working tree so a binding built from other sources cannot be used + // silently — struct layouts differ and fields read as garbage. + m.attr("__build_commit__") = SIMPLER_BUILD_COMMIT; + // --- DataType enum --- nb::enum_(m, "DataType") .value("FLOAT32", DataType::FLOAT32) diff --git a/python/simpler/task_interface.py b/python/simpler/task_interface.py index 5190143239..b18142329c 100644 --- a/python/simpler/task_interface.py +++ b/python/simpler/task_interface.py @@ -29,8 +29,10 @@ from dataclasses import dataclass from enum import IntEnum from math import prod +from pathlib import Path from typing import Any +import _task_interface as _ti_module # pyright: ignore[reportMissingImports] from _task_interface import ( # pyright: ignore[reportMissingImports] MAILBOX_ERROR_MSG_SIZE, MAILBOX_OFF_ERROR_MSG, @@ -57,6 +59,71 @@ read_args_from_blob, ) + +def _assert_bindings_match_source_tree() -> None: + """Refuse a `_task_interface` built from a different revision of this tree. + + An editable install pins the compiled extension at install time + (``editable.rebuild = false``) while this file is read live, so switching + branches or rebasing moves the Python source out from under a fixed binary. + Nothing then rebuilds, and a changed struct layout — `CallConfig` losing a + field, say — makes attributes read as 0 with no error at all. That surfaces + much later as a plausible-looking runtime rejection + (``launch_aicpu_num (0) must be in range [1, 4]``) and reads as a product + bug, so it is worth one git call at import to stop. + + Only source-tree installs are checked: a wheel has no ``.git`` to compare + against, and a build with no git available carries an empty stamp. + + A *missing* stamp is not the same as an empty one. The attribute only + disappears on an extension compiled before it existed, which in a checkout + new enough to run this function is by definition a different revision — the + exact case this guards, and the one every already-installed worktree is in + the moment this lands. Treating it like "cannot tell" would let precisely + those through. + """ + import subprocess # noqa: PLC0415 + + repo_root = Path(__file__).resolve().parents[2] + if not (repo_root / ".git").exists(): + return + if not hasattr(_ti_module, "__build_commit__"): + raise ImportError( + "_task_interface predates the build stamp, so it was compiled before the " + "revision you are running and its struct layouts may not match the Python " + "that drives them — fields can read as 0 with no error.\n" + "Rebuild: pip install --no-build-isolation -e ." + ) + built_from: str = _ti_module.__build_commit__ + if not built_from: + return + try: + result = subprocess.run( + ["git", "rev-parse", "HEAD"], + cwd=str(repo_root), + capture_output=True, + text=True, + timeout=5, + check=False, + ) + except Exception: # noqa: BLE001 + return + if result.returncode != 0: + return + head = result.stdout.strip() + if not head or head == built_from: + return + raise ImportError( + f"_task_interface was built from {built_from[:12]}, but this source tree is at " + f"{head[:12]}. The compiled extension does not rebuild on import " + f"(editable.rebuild = false), so its struct layouts may no longer match the " + f"Python that drives them — fields can read as 0 with no error.\n" + f"Rebuild: pip install --no-build-isolation -e ." + ) + + +_assert_bindings_match_source_tree() + __all__ = [ "DataType", "get_element_size",