diff --git a/docs/developer-guide.md b/docs/developer-guide.md index 0b89c69aa..1bd88dbf8 100644 --- a/docs/developer-guide.md +++ b/docs/developer-guide.md @@ -185,7 +185,7 @@ This builds the nanobind `_task_interface` extension **and** pre-builds all runt | 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 | -| Pinned pto-isa commit (CI `PTO_ISA_COMMIT`) | Re-run `pip install` — onboard a2a3 `host_runtime.so` embeds pto-isa SDMA headers, so a commit bump must rebuild it. `pip install` reads `pto_isa.pin` by default; to build against a different commit without changing the pin, set `PTO_ISA_ROOT` at a pre-checked-out clone or run `python simpler_setup/build_runtimes.py --pto-isa-commit ` (issue #1067). | +| Pinned pto-isa commit (CI `PTO_ISA_COMMIT`) | Re-run `pip install` — onboard a2a3 `host_runtime.so` embeds pto-isa SDMA headers, so a commit bump must rebuild it. The cmake cache stamp and the `host_runtime` ccache key both fold in the resolved pto-isa commit, so a bump invalidates the stale object automatically — no manual `ccache -C` / `rm -rf build/` needed (issue #1139). `pip install` reads `pto_isa.pin` by default; to build against a different commit without changing the pin, set `PTO_ISA_ROOT` at a pre-checked-out clone or run `python simpler_setup/build_runtimes.py --pto-isa-commit ` (issue #1067). | ### Runtime binary lookup diff --git a/simpler_setup/runtime_builder.py b/simpler_setup/runtime_builder.py index b03449087..c371d114b 100644 --- a/simpler_setup/runtime_builder.py +++ b/simpler_setup/runtime_builder.py @@ -9,6 +9,7 @@ import fcntl import json import logging +import os import shutil import subprocess from concurrent.futures import ThreadPoolExecutor @@ -41,38 +42,56 @@ def _get_git_head(repo_root: Path) -> str: return "" -def _invalidate_cache_if_stale(target_cache_dir: Path, current_commit: str) -> None: - """Clear target_cache_dir if it was built from a different git commit. +def _abbrev_stamp(stamp: str) -> str: + """Abbreviate each commit in a (possibly composite) cache stamp for logging. + + The stamp is ```` or ``:pto-isa=``. + Truncating the whole string (e.g. ``stamp[:20]``) hides a pto-isa-only + change: the 40-char runtime sha prefix is identical, so both the old and + new stamps render the same — masking exactly the bump the log is meant to + surface (issue #1139). Shorten each sha segment independently so a + pto-isa-only change stays visible. + """ + runtime, sep, isa = stamp.partition(":pto-isa=") + if sep: + return f"{runtime[:12]}:pto-isa={isa[:12]}" + return runtime[:12] + + +def _invalidate_cache_if_stale(target_cache_dir: Path, current_stamp: str) -> None: + """Clear target_cache_dir if it was built from a different source stamp. git does not update file mtimes on checkout, so cmake's incremental build - cannot detect that source files changed. Comparing the HEAD commit stored - at last build time against the current HEAD is a reliable signal that - sources may have changed and a clean rebuild is needed. + cannot detect that source files changed. Comparing the stamp stored at last + build time against the current one is a reliable signal that sources may + have changed and a clean rebuild is needed. The stamp is the runtime repo + HEAD, extended with the pto-isa commit for builds that embed pto-isa + headers (see RuntimeBuilder._build_cache_stamp). - When the current commit can't be determined (no git, transient failure), + When the current stamp can't be determined (no git, transient failure), fall through to a clean rebuild — a fresh compile is cheap relative to the risk of linking against stale objects. """ - if not current_commit: + if not current_stamp: if target_cache_dir.is_dir(): - logger.info("git HEAD unavailable, clearing cmake cache: %s", target_cache_dir) + logger.info("build stamp unavailable, clearing cmake cache: %s", target_cache_dir) shutil.rmtree(target_cache_dir) target_cache_dir.mkdir(parents=True, exist_ok=True) return commit_file = target_cache_dir / _GIT_COMMIT_FILE if commit_file.is_file(): - cached_commit = commit_file.read_text().strip() - if cached_commit == current_commit: + cached_stamp = commit_file.read_text().strip() + if cached_stamp == current_stamp: return logger.info( - "git HEAD changed (%s → %s), clearing cmake cache: %s", - cached_commit[:12], - current_commit[:12], + "build stamp changed (%s → %s), clearing cmake cache: %s", + _abbrev_stamp(cached_stamp), + _abbrev_stamp(current_stamp), target_cache_dir, ) shutil.rmtree(target_cache_dir) target_cache_dir.mkdir(parents=True, exist_ok=True) - commit_file.write_text(current_commit + "\n") + commit_file.write_text(current_stamp + "\n") @dataclass @@ -160,9 +179,70 @@ def _resolve_target_dirs(self, config_dir: Path, build_config: dict, target: str return include_dirs, source_dirs def _requires_pto_isa_compat_validation(self) -> bool: - """Return True when this runtime embeds PTO-ISA headers into host code.""" + """Return True when this runtime embeds PTO-ISA headers into host code. + + Scoped on arch/variant, not on ``SIMPLER_ENABLE_PTO_SDMA_WORKSPACE`` + (the actual flag that pulls pto-isa headers into the host .so). This is + deliberately coarser: should a2a3 onboard ever turn that workspace off, + a pto-isa bump would still invalidate this target's cache even though it + no longer embeds pto-isa. That over-invalidation only costs an extra + recompile — it can never serve a stale object — so erring wide is the + safe direction, and keeping the scope arch/variant-based matches the + cmake-side define gating in src/a2a3/platform/onboard/host/CMakeLists.txt. + """ return self._arch == "a2a3" and self._variant == "onboard" + def _resolve_build_pto_isa_commit(self) -> str: + """Return the pto-isa commit baked into this build, or "" when irrelevant. + + Only a2a3 onboard host code embeds pto-isa headers, so a pto-isa bump + must invalidate that build's cmake cache even when the runtime repo + HEAD is unchanged (issue #1139: a stale host_runtime.so compiled + against the old headers otherwise survives a reinstall). For every + other arch/variant the pto-isa revision does not affect the compiled + objects, so return "" and leave the stamp keyed on the runtime HEAD. + + SIMPLER_RUN_PTO_ISA_COMMIT is exported by + ``simpler_setup.pto_isa.ensure_pto_isa_root`` as the resolved revision; + it matches the commit ``write_pto_isa_build_metadata`` records. + """ + if not self._requires_pto_isa_compat_validation(): + return "" + commit = os.environ.get("SIMPLER_RUN_PTO_ISA_COMMIT", "").strip() + if commit: + return commit + root = os.environ.get("PTO_ISA_ROOT", "").strip() + if root: + from .pto_isa import get_pto_isa_head # noqa: PLC0415 + + commit = get_pto_isa_head(root) + if commit: + # The CMake ccache-bust define (CMakeLists.txt) reads this env + # var, not PTO_ISA_ROOT's HEAD. Write the resolved commit back + # so that define stays in lockstep with the stamp computed here + # — otherwise the cmake cache invalidates but ccache can still + # serve the stale host_runtime object (issue #1139). + os.environ["SIMPLER_RUN_PTO_ISA_COMMIT"] = commit + return commit + return "" + + def _build_cache_stamp(self) -> str: + """Stamp identifying the sources this build was compiled from. + + Combines the runtime repo HEAD with the pto-isa commit (a2a3 onboard + only) so the cmake cache is invalidated whenever either changes. An + empty runtime HEAD is preserved verbatim so the + ``_invalidate_cache_if_stale`` 'unavailable → clean rebuild' path still + fires rather than being masked by a present pto-isa commit. + """ + runtime_commit = _get_git_head(PROJECT_ROOT) + if not runtime_commit: + return "" + pto_isa_commit = self._resolve_build_pto_isa_commit() + if pto_isa_commit: + return f"{runtime_commit}:pto-isa={pto_isa_commit}" + return runtime_commit + def _lookup_binaries(self, name: str, output_dir: Path) -> RuntimeBinaries: """Look up pre-built binaries from output_dir. @@ -268,7 +348,7 @@ def get_binaries(self, name: str, build: bool = False) -> RuntimeBinaries: compiler = self._runtime_compiler - current_commit = _get_git_head(PROJECT_ROOT) + cache_stamp = self._build_cache_stamp() def _compile_target(target: str) -> Path: include_dirs, source_dirs = self._resolve_target_dirs(config_dir, build_config, target) @@ -282,7 +362,7 @@ def _compile_target(target: str) -> Path: lock_path = cache_dir / f".{target}.lock" with open(lock_path, "w") as lock_fd: fcntl.flock(lock_fd, fcntl.LOCK_EX) - _invalidate_cache_if_stale(cache_dir / target, current_commit) + _invalidate_cache_if_stale(cache_dir / target, cache_stamp) return compiler.compile( # type: ignore[return-value] target, include_dirs, diff --git a/src/a2a3/platform/onboard/host/CMakeLists.txt b/src/a2a3/platform/onboard/host/CMakeLists.txt index ae29e867c..f0a403b14 100644 --- a/src/a2a3/platform/onboard/host/CMakeLists.txt +++ b/src/a2a3/platform/onboard/host/CMakeLists.txt @@ -115,6 +115,22 @@ if(SIMPLER_ENABLE_PTO_SDMA_WORKSPACE) target_compile_definitions(host_runtime PRIVATE SIMPLER_ENABLE_PTO_SDMA_WORKSPACE=1) endif() +# Bake the resolved pto-isa commit into the compile command. pto-isa headers +# (e.g. kSdmaMaxChan in sdma_workspace_manager.hpp) are compiled into this .so, +# but a pto-isa update leaves the runtime repo HEAD — and the pto-isa header +# mtimes — untouched, so a plain reinstall serves a stale object from ccache. +# This define perturbs the ccache key so a pto-isa bump forces a real recompile. +# It is intentionally unused in code (cache-bust only). See issue #1139. +# SIMPLER_RUN_PTO_ISA_COMMIT is exported by ensure_pto_isa_root() before the build. +# Gated on the env var (set for a2a3 onboard), not SIMPLER_ENABLE_PTO_SDMA_WORKSPACE: +# if that workspace is ever turned off, a pto-isa bump still forces a recompile of a +# target that no longer embeds pto-isa. That over-invalidation only costs a rebuild and +# never serves a stale object, so erring wide is intentional (see _resolve_build_pto_isa_commit). +if(NOT "$ENV{SIMPLER_RUN_PTO_ISA_COMMIT}" STREQUAL "") + target_compile_definitions(host_runtime PRIVATE + SIMPLER_PTO_ISA_BUILD_COMMIT="$ENV{SIMPLER_RUN_PTO_ISA_COMMIT}") +endif() + # Include directories - always include local headers target_include_directories(host_runtime PRIVATE diff --git a/tests/ut/py/test_runtime_builder.py b/tests/ut/py/test_runtime_builder.py index 96f41096d..9179ed8f7 100644 --- a/tests/ut/py/test_runtime_builder.py +++ b/tests/ut/py/test_runtime_builder.py @@ -8,6 +8,7 @@ # ----------------------------------------------------------------------------------------------------------- """Tests for RuntimeBuilder class.""" +import os import textwrap from pathlib import Path from unittest.mock import patch @@ -360,6 +361,116 @@ def test_clears_when_commit_unavailable(self, tmp_path): assert cache_dir.is_dir() +class TestAbbrevStamp: + """Stamp abbreviation must keep a pto-isa-only change visible in the log.""" + + def test_pure_runtime_sha_truncated(self): + from simpler_setup.runtime_builder import _abbrev_stamp # noqa: PLC0415 + + assert _abbrev_stamp("0123456789abcdef0123456789abcdef01234567") == "0123456789ab" + + def test_composite_keeps_both_segments_visible(self): + """A pto-isa-only bump shares the runtime prefix; both shas must show.""" + from simpler_setup.runtime_builder import _abbrev_stamp # noqa: PLC0415 + + runtime = "0123456789abcdef0123456789abcdef01234567" + old = _abbrev_stamp(f"{runtime}:pto-isa=aaaaaaaaaaaaaaaa") + new = _abbrev_stamp(f"{runtime}:pto-isa=bbbbbbbbbbbbbbbb") + assert old == "0123456789ab:pto-isa=aaaaaaaaaaaa" + assert new == "0123456789ab:pto-isa=bbbbbbbbbbbb" + assert old != new # the bump is not hidden behind an identical prefix + + +class TestBuildCacheStamp: + """Test cmake cache stamp composition (runtime HEAD + pto-isa commit).""" + + def _make_builder(self, platform): + from simpler_setup.platform_info import parse_platform # noqa: PLC0415 + from simpler_setup.runtime_builder import RuntimeBuilder # noqa: PLC0415 + + builder = RuntimeBuilder.__new__(RuntimeBuilder) + builder.platform = platform + builder._arch, builder._variant = parse_platform(platform) + return builder + + def test_a2a3_onboard_folds_in_pto_isa_commit(self, monkeypatch): + """a2a3 onboard with a resolved pto-isa commit → composite stamp.""" + import simpler_setup.runtime_builder as rb_module # noqa: PLC0415 + + monkeypatch.setattr(rb_module, "_get_git_head", lambda _root: "runtime_sha") + monkeypatch.setenv("SIMPLER_RUN_PTO_ISA_COMMIT", "isa_sha") + + builder = self._make_builder("a2a3") + assert builder._build_cache_stamp() == "runtime_sha:pto-isa=isa_sha" + + def test_non_a2a3_onboard_uses_pure_runtime_sha(self, monkeypatch): + """Other arch/variant ignores pto-isa → stamp keyed on runtime HEAD only.""" + import simpler_setup.runtime_builder as rb_module # noqa: PLC0415 + + monkeypatch.setattr(rb_module, "_get_git_head", lambda _root: "runtime_sha") + monkeypatch.setenv("SIMPLER_RUN_PTO_ISA_COMMIT", "isa_sha") + + builder = self._make_builder("a2a3sim") + assert builder._build_cache_stamp() == "runtime_sha" + + def test_empty_runtime_head_yields_empty_stamp(self, monkeypatch): + """No runtime HEAD → empty stamp, preserving the 'unavailable → clean rebuild' path.""" + import simpler_setup.runtime_builder as rb_module # noqa: PLC0415 + + monkeypatch.setattr(rb_module, "_get_git_head", lambda _root: "") + monkeypatch.setenv("SIMPLER_RUN_PTO_ISA_COMMIT", "isa_sha") + + builder = self._make_builder("a2a3") + assert builder._build_cache_stamp() == "" + + +class TestResolveBuildPtoIsaCommit: + """Test pto-isa commit resolution and cmake/ccache lockstep write-back.""" + + def _make_builder(self, platform): + from simpler_setup.platform_info import parse_platform # noqa: PLC0415 + from simpler_setup.runtime_builder import RuntimeBuilder # noqa: PLC0415 + + builder = RuntimeBuilder.__new__(RuntimeBuilder) + builder.platform = platform + builder._arch, builder._variant = parse_platform(platform) + return builder + + def test_non_a2a3_onboard_returns_empty(self, monkeypatch): + monkeypatch.setenv("SIMPLER_RUN_PTO_ISA_COMMIT", "isa_sha") + + builder = self._make_builder("a2a3sim") + assert builder._resolve_build_pto_isa_commit() == "" + + def test_prefers_run_commit_env(self, monkeypatch): + monkeypatch.setenv("SIMPLER_RUN_PTO_ISA_COMMIT", "isa_sha") + monkeypatch.setenv("PTO_ISA_ROOT", "/should/not/be/read") + + builder = self._make_builder("a2a3") + assert builder._resolve_build_pto_isa_commit() == "isa_sha" + + def test_fallback_resolves_root_and_writes_back_for_lockstep(self, monkeypatch): + """PTO_ISA_ROOT fallback must export the resolved commit so the cmake + ccache-bust define (which reads SIMPLER_RUN_PTO_ISA_COMMIT) stays in + lockstep with the stamp computed here (issue #1139).""" + from simpler_setup import pto_isa # noqa: PLC0415 + + monkeypatch.delenv("SIMPLER_RUN_PTO_ISA_COMMIT", raising=False) + monkeypatch.setenv("PTO_ISA_ROOT", "/some/pto-isa") + monkeypatch.setattr(pto_isa, "get_pto_isa_head", lambda _root: "resolved_sha") + + builder = self._make_builder("a2a3") + assert builder._resolve_build_pto_isa_commit() == "resolved_sha" + assert os.environ["SIMPLER_RUN_PTO_ISA_COMMIT"] == "resolved_sha" + + def test_unresolvable_returns_empty(self, monkeypatch): + monkeypatch.delenv("SIMPLER_RUN_PTO_ISA_COMMIT", raising=False) + monkeypatch.delenv("PTO_ISA_ROOT", raising=False) + + builder = self._make_builder("a2a3") + assert builder._resolve_build_pto_isa_commit() == "" + + # --- Full integration tests (real compilation) ---