diff --git a/.github/workflows/build-images.yml b/.github/workflows/build-images.yml index e8b981d7..0dab0fb7 100644 --- a/.github/workflows/build-images.yml +++ b/.github/workflows/build-images.yml @@ -46,7 +46,11 @@ jobs: outputs: matrix: ${{ steps.matrix.outputs.matrix }} steps: + # Full history: the matrix below diffs against the base commit to work + # out which images this change can actually affect. - uses: actions/checkout@v4 + with: + fetch-depth: 0 - uses: actions/setup-python@v5 with: @@ -94,12 +98,40 @@ jobs: print(f"all lockfiles present ({checked} projects)") PY + - name: Which images this change can affect + id: changed + # An image installs its own uv projects from their locks, so a change + # to one lock cannot alter another image. Diffing lets the matrix drop + # the ones it cannot affect: the generative image compiles the PyG + # extensions from source on arm64 and takes about two hours, which is + # pure cost on a change to, say, venv/mlip/uv.lock. When the base is + # unknown (a new branch, a forced push) the list is left empty and + # every image is built. + run: | + BASE="" + if [ "${{ github.event_name }}" = "pull_request" ]; then + BASE="origin/${{ github.base_ref }}" + elif [ "${{ github.event_name }}" = "push" ]; then + BASE="${{ github.event.before }}" + fi + : > /tmp/changed.txt + if [ -n "$BASE" ] && git rev-parse --verify -q "$BASE^{commit}" >/dev/null; then + git diff --name-only "$BASE...HEAD" > /tmp/changed.txt || : > /tmp/changed.txt + fi + echo "paths changed against '${BASE}': $(wc -l < /tmp/changed.txt)" + cat /tmp/changed.txt + - name: Render build matrix id: matrix env: FILTER: ${{ inputs.images }} run: | - MATRIX="$(python docker/render.py matrix)" + # An explicit dispatch selection wins over the diff. + if [ -n "$FILTER" ]; then + MATRIX="$(python docker/render.py matrix)" + else + MATRIX="$(python docker/render.py matrix --changed - < /tmp/changed.txt)" + fi if [ -n "$FILTER" ]; then MATRIX="$(FILTER="$FILTER" python - <<'PY' import json, os, subprocess diff --git a/docker/render.py b/docker/render.py index ee97c83b..13a27d36 100755 --- a/docker/render.py +++ b/docker/render.py @@ -154,11 +154,60 @@ def per_platform(image: dict, key: str, platform: str, default): return value +def image_project_paths(image: dict) -> set[str]: + """The paths that own this image's contents. + + An image installs one uv project (``venv``) or several (``venvs``) from + their locks, so only those files can change what it contains. + """ + projects = image.get("venvs") or [image["venv"]] + return { + path + for project in projects + for path in (f"venv/{project}/uv.lock", f"venv/{project}/pyproject.toml") + } + + +def select_images(spec: dict, changed: set[str]) -> list[str]: + """Images a change can affect, given the paths it touches. + + Everything an image installs comes from its own project locks or from the + build itself, so a change to `venv/mlip/uv.lock` cannot alter the + generative image and rebuilding it is pure cost. Anything not owned by a + single image — docker/, this workflow, the sources the images copy in — is + shared, and selects every image. An empty change set also selects every + image: without information, build everything rather than guess. + """ + names = [image["name"] for image in spec["images"]] + if not changed: + return names + owners = { + path: image["name"] + for image in spec["images"] + for path in image_project_paths(image) + } + wanted = set() + for path in changed: + owner = owners.get(path) + if owner is None: + return names + wanted.add(owner) + return [name for name in names if name in wanted] + + def cmd_matrix(spec: dict, args: argparse.Namespace) -> int: """Print the GitHub Actions matrix as JSON, one entry per image+platform.""" runner = {"linux/amd64": "ubuntu-24.04", "linux/arm64": "ubuntu-24.04-arm"} + # args may be None: cmd_matrix is also called directly from the tests. + changed = set(getattr(args, "changed", None) or ()) + if "-" in changed: + changed.discard("-") + changed.update(line.strip() for line in sys.stdin if line.strip()) + wanted = select_images(spec, changed) include = [] for image in spec["images"]: + if image["name"] not in wanted: + continue for platform in image["platforms"]: if "venvs" in image: dockerfile = "docker/Dockerfile.cuda" @@ -266,6 +315,17 @@ def main() -> int: p.set_defaults(func=cmd_server_map) p = sub.add_parser("matrix", help="GitHub Actions build matrix as JSON") + p.add_argument( + "--changed", + action="append", + metavar="PATH", + default=None, + help=( + "A path this change touches; repeat it, or pass '-' to read paths " + "from stdin. Only the images those paths can affect are emitted. " + "Without it the full matrix is printed." + ), + ) p.set_defaults(func=cmd_matrix) args = parser.parse_args() diff --git a/tests/test_images_and_manifests.py b/tests/test_images_and_manifests.py index ad312679..f79a9807 100644 --- a/tests/test_images_and_manifests.py +++ b/tests/test_images_and_manifests.py @@ -310,3 +310,74 @@ def fail_create(args, **kwargs): image_publish.publish( self.matrix("cpu"), self.REGISTRY, self.RUN_ID, VERSION ) + + +class TestMatrixSelection: + """A change only rebuilds the images it can affect. + + The build matrix is narrowed by diffing, because the generative image + compiles the PyG extensions from source on arm64 and takes about two hours; + rebuilding it for a change to `venv/mlip/uv.lock` is pure cost. Anything the + filter cannot attribute to exactly one image must select every image, since + building too much only wastes time while building too little ships a broken + image. + """ + + ALL = [image["name"] for image in SPEC["images"]] + + def test_no_information_builds_everything(self): + assert render.select_images(SPEC, set()) == self.ALL + + def test_a_project_lock_selects_only_its_image(self): + assert render.select_images(SPEC, {"venv/mlip/uv.lock"}) == ["mlip"] + assert render.select_images(SPEC, {"venv/fairchem/pyproject.toml"}) == [ + "fairchem" + ] + + def test_a_multi_project_image_is_selected_by_either_project(self): + for project in ("adit", "diffcsp", "mattergen"): + assert render.select_images(SPEC, {f"venv/{project}/uv.lock"}) == [ + "generative" + ] + + def test_the_build_itself_selects_everything(self): + for path in ( + "docker/Dockerfile", + "docker/Dockerfile.cuda", + "docker/images.json", + ): + assert render.select_images(SPEC, {path}) == self.ALL, path + + def test_the_sources_images_copy_select_everything(self): + # Images install the repository itself, so a source change can alter any + # of them; nothing in the filter narrows that. + for path in ("src/mcp_server/mace_server.py", "pyproject.toml", "README.md"): + assert render.select_images(SPEC, {path}) == self.ALL, path + + def test_one_unattributable_path_selects_everything(self): + mixed = {"venv/mlip/uv.lock", "src/utils/mlips/base.py"} + assert render.select_images(SPEC, mixed) == self.ALL + + def test_the_real_change_this_rule_exists_for(self): + """Both FairChem and MatGL bumps touch locks the generative image does + not use; that image must drop out of the matrix.""" + changed = {"venv/mlip/uv.lock", "venv/fairchem/uv.lock"} + assert render.select_images(SPEC, changed) == ["mlip", "fairchem"] + + def test_matrix_emits_only_the_selected_images(self): + for changed, expected in ( + ([], self.ALL), + (["venv/mlip/uv.lock"], ["mlip"]), + (["venv/adit/uv.lock"], ["generative"]), + ): + argv = [ + sys.executable, + str(PROJECT_ROOT / "docker" / "render.py"), + "matrix", + ] + for path in changed: + argv += ["--changed", path] + result = subprocess.run(argv, capture_output=True, text=True) + assert result.returncode == 0, result.stderr + images = sorted({e["image"] for e in json.loads(result.stdout)["include"]}) + assert images == sorted(expected), changed