From 717354a71abcab042fb671d7c91e38da661f279f Mon Sep 17 00:00:00 2001 From: hujc Date: Thu, 3 Sep 2026 19:13:54 -0700 Subject: [PATCH] [CI] Skip the image invariants on a deps-cache hit and link _isaac_sim only in Kit images (#7540) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every first CI run of a new commit on a dependency-stable branch has failed since #7405 landed (develop included): `Build Base Docker Image` dies in 20 s and skips the 17 test jobs behind it, and `Multi-GPU training smoke (kit-less)` exits before pytest. Both are CI plumbing, fixed here. inspect` on the commit tag after `ecr-build-push-pull`. The action leaves an image on the runner only after a full build or an exact-tag pull. On a deps-cache hit it just aliases the commit tag onto the `deps-` manifest in ECR and pulls nothing, so the inspect fails: ``` Error response from daemon: No such image: isaac-lab-ci:develop-8ca264bd8e5528d7d9b7b6f58a9d8cd051c5933a ``` develop runs 33733992373, 33740164999 and 33781460422 fail this way; PRs pass only after a deps change (full build) or on a re-run of an already built head (exact-tag pull). The step also ran too late to be a gate. The action pushes the commit tag and the deps tag inside itself, so an image that fails the check is already published. That is not hypothetical: on `newton-world-prefixes` (run 33735349483) `deps-6aebe721c56a91f7` was pushed at 09:05:07 and the invariant failed at 09:05:11, leaving an image whose Isaac Sim extensions cannot load in the shared cache. Any later run whose install inputs hash the same inherits it. Fix: move the assertion into the action, before every push, behind a `verify-command` input that the base image job passes. A failure now publishes nothing, so the next run rebuilds and fails again rather than serving the bad image from the cache. Skipping it on a cache hit is then correct by construction — every published image passed when it was built, and the deps hash covers exactly the install inputs the invariant asserts against. `run_tests.sh` bind-mounts nine writable runtime directories under `/isaac-sim/…` into every container, so `/isaac-sim` exists as a directory in the kit-less image too, and the container script then runs `ln -s /isaac-sim _isaac_sim` unconditionally. #7466 taught `isaaclab.sh` to treat a `_isaac_sim` directory without a source-build marker as a downloaded Isaac Sim and to refuse it next to an active venv; the kit-less image sets `VIRTUAL_ENV=/opt/isaaclab-venv`, so every run stops with: ``` [ERROR] Downloaded Isaac Sim packages cannot be combined with a Python virtual environment. ``` run-tests script without exempting the kit-less lane. Every PR's smoke run since 11:43Z on 2026-09-03 fails identically. Fix: plant the link only when `/isaac-sim/python.sh` exists. Kit images are unchanged; the kit-less image resolves its interpreter through `VIRTUAL_ENV`, as it did before Every place that creates the link, and how it interacts with the guard: | Site | Creates | Verdict | | --- | --- | --- | | `run-tests/run_tests.sh` | `/isaac-sim` in every image | **Fixed here**: guarded on `python.sh` | | `multi-gpu/multi_gpu_host_launcher.sh` | `/isaac-sim`, unguarded | Correct today: its only caller passes `CI_IMAGE_TAG`, the Kit image. The kit-less lane goes through `run-tests` | | `Dockerfile.base`, `Dockerfile.curobo` | `ISAACSIM_ROOT_PATH`, alongside `VIRTUAL_ENV` | Correct: `UV_PYTHON=${ISAACSIM_ROOT_PATH}/kit/python/bin/python3`, so the venv is on the bundled Python and #7405's exemption applies | | `Dockerfile.kitless` | never; asserts `test ! -e "${ISAACLAB_PATH}/_isaac_sim"` | Correct | | `cli/commands/misc.py` (`--isaacsim_source`) | source-build link | Correct: writes `.isaaclab_source_build` | | `docs/.../installation/index.rst` ×3 | user-run `ln -s` | Out of scope, see below | - `docker/test/test_container_profiles.py` gains two regression tests — one pinning the kit-less guard, one pinning that verification precedes both push steps. Each fails without its fix. 15 pass in that file, 20 with `test_carb_env_shim.py`. - `yaml.safe_load` on both YAML files, `bash -n` on the script, pre-commit on the changed files. - This PR touches `.github/actions/{ecr-build-push-pull,run-tests}/` and `build.yaml`, which the detect-changes patterns of both workflows match, so its own CI exercises both paths: the base image job takes the deps-cache-hit path (no dependency change here) and the kit-less smoke job runs. Three checkers implement the same "downloaded Isaac Sim vs. virtual environment" rule and #7405 updated only two, leaving user-facing divergences that this PR does not touch: - `isaaclab.bat` has neither the venv-on-bundled-Python exemption nor the `--isaacsim_source` escape, so on Windows the remedy the Linux error message recommends is still rejected. - `cli/commands/envs.py::_reject_downloaded_isaac_sim` has no exemption either, so `isaaclab -c/-u` rejects a venv created on the bundled Python that `isaaclab.sh` accepts. - The docs link a *source build* with a bare `ln -s`, which leaves no `.isaaclab_source_build` marker, so the same tree is treated as "downloaded" unless linked via `--isaacsim_source`. `isaaclab.sh` also lacks the `python.sh` precondition its Python twin (`cli/utils.py`) has, but that is deliberate: `test_launcher_rejects_downloaded_isaac_sim_with_active_environment` pins the fail-closed behaviour on a bare directory. That is why the fix here is in the CI script. - Bug fix (non-breaking change which fixes an issue) - [x] Backport this pull request to the active release branch after it merges into `develop` - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation (the action README documents the new output) - [x] My changes generate no new warnings - [ ] Tests: CI-only change; the workflows' own runs on this PR are the test - [ ] Changelog: no source package changed - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there (cherry picked from commit df44b19aaec2d7750272b4f634b18f043e90c8c2) --- .github/actions/ecr-build-push-pull/README.md | 13 +++++ .../actions/ecr-build-push-pull/action.yml | 27 ++++++++++ .github/actions/run-tests/run_tests.sh | 7 ++- .github/workflows/build.yaml | 10 ++++ docker/test/test_container_profiles.py | 36 +++++++++++++ docker/test/test_image_invariants.py | 54 +++++++++++++++++++ 6 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 docker/test/test_image_invariants.py diff --git a/.github/actions/ecr-build-push-pull/README.md b/.github/actions/ecr-build-push-pull/README.md index 2e306c978b8e..2dea04797126 100644 --- a/.github/actions/ecr-build-push-pull/README.md +++ b/.github/actions/ecr-build-push-pull/README.md @@ -16,6 +16,19 @@ ECR is also used as the BuildKit layer cache. ecr-url: (optional, complete url for ECR storage) ``` +## Verifying a freshly built image + +Pass `verify-test-path` to assert against the image before it is published: + +```yaml + verify-test-path: docker/test/test_image_invariants.py +``` + +The tests run only on a full build, with `IMAGE_TAG` set, so the caller's job needs `uv` +(`astral-sh/setup-uv`). A failure fails the action with nothing pushed, so the next run +rebuilds rather than serving the bad image from the deps cache. Exact-tag and deps-cache hits skip +them: that image passed when it was built. + ## ECR URL resolution order 1. `ecr-url` input diff --git a/.github/actions/ecr-build-push-pull/action.yml b/.github/actions/ecr-build-push-pull/action.yml index b289b4ebb7d8..7c595e7d3407 100644 --- a/.github/actions/ecr-build-push-pull/action.yml +++ b/.github/actions/ecr-build-push-pull/action.yml @@ -37,6 +37,16 @@ inputs: description: Tag used for the ECR layer cache image (e.g. "cache-base", "cache-curobo"). required: false default: 'cache' + verify-test-path: + description: > + Path to a test file or directory asserted against a freshly built image, before it is + tagged or pushed. Tests run with IMAGE_TAG set; a failure fails the action with nothing + published, so the next run rebuilds instead of inheriting the bad image from the cache. + + Not run on an exact-tag or deps-cache hit: those serve an image that already passed when it + was built. + required: false + default: '' pull-on-deps-hit: description: > Pull the image locally after a deps-cache hit. Needed by jobs that run @@ -242,6 +252,23 @@ runs: cache-to: ${{ steps.resolve-ecr.outputs.available == 'true' && format('type=registry,ref={0},mode=max', env.CACHE_IMAGE) || '' }} deps-hash: ${{ steps.deps-hash.outputs.hash }} + # Assert against the image while it is only local: the push steps below publish under both + # the commit tag and the deps tag, and a deps-cache hit later serves that image without + # rebuilding it, so anything published unverified stays unverified. + - name: Verify freshly built image + if: > + inputs.verify-test-path != '' && + steps.pull-exact.outputs.hit != 'true' && + steps.deps-cache.outputs.deps-cache-hit != 'true' + shell: bash + env: + IMAGE_TAG: ${{ inputs.image-tag }} + TEST_PATH: ${{ inputs.verify-test-path }} + run: | + set -euo pipefail + uv run --no-project --with pytest \ + python -m pytest -q "${TEST_PATH}" + - name: Tag built image with ECR-prefixed name if: > steps.resolve-ecr.outputs.available == 'true' && diff --git a/.github/actions/run-tests/run_tests.sh b/.github/actions/run-tests/run_tests.sh index 3779ab10c562..fbccbfa45dd3 100755 --- a/.github/actions/run-tests/run_tests.sh +++ b/.github/actions/run-tests/run_tests.sh @@ -320,8 +320,11 @@ run_tests() { set -e cd /workspace/isaaclab mkdir -p tests - rm _isaac_sim || true - ln -s /isaac-sim _isaac_sim + # The runtime mounts above create /isaac-sim in every image. Link it only where Kit + # lives there: in the kit-less image the link would read as a downloaded Isaac Sim, + # which isaaclab.sh refuses to combine with the image's VIRTUAL_ENV. + rm -f _isaac_sim + if [ -x /isaac-sim/python.sh ]; then ln -s /isaac-sim _isaac_sim; fi if [ -n \"\${WARP_CACHE_PATH:-}\" ]; then ./isaaclab.sh -p tools/verify_warp_cache.py fi diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 2ff42e847a07..ce2791c8cf84 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -182,6 +182,15 @@ jobs: fetch-depth: 1 lfs: true + # The GPU runners have no uv on PATH; the invariant check below needs it. + - name: Set up uv + uses: astral-sh/setup-uv@v6 + with: + enable-cache: true + + # #6329 aborts the pip install when it strands a prebundled package's __init__.py + # (nvbugs 6343978: 14 Isaac Sim extensions fail to load). The images install with + # ``uv sync``, which never runs that guard, so assert the same invariant on the image. - name: Build and push to ECR uses: ./.github/actions/ecr-build-push-pull with: @@ -190,6 +199,7 @@ jobs: isaacsim-version: ${{ needs.config.outputs.isaacsim_image_tag }} dockerfile-path: docker/Dockerfile.base cache-tag: cache-base + verify-test-path: docker/test/test_image_invariants.py build-curobo: name: Build cuRobo Docker Image diff --git a/docker/test/test_container_profiles.py b/docker/test/test_container_profiles.py index 39d39c974829..9a17fc87aab7 100644 --- a/docker/test/test_container_profiles.py +++ b/docker/test/test_container_profiles.py @@ -15,6 +15,7 @@ from docker.utils import ContainerInterface, volume_mounts DOCKER_DIR = Path(__file__).resolve().parents[1] +REPO_ROOT = DOCKER_DIR.parent @pytest.fixture @@ -288,6 +289,41 @@ def test_kitless_compose_service_has_no_isaac_sim_mounts(): assert all("/kit/" not in mount["target"].lower() for mount in mounts) +def test_image_is_verified_before_it_is_published(): + """A published image must be a verified one. + + The push steps publish under both the commit tag and the deps tag, and a later deps-cache hit + serves that image without rebuilding it, so anything published unverified stays unverified. + """ + action = yaml.safe_load( + (REPO_ROOT / ".github" / "actions" / "ecr-build-push-pull" / "action.yml").read_text(encoding="utf-8") + ) + names = [step["name"] for step in action["runs"]["steps"] if "name" in step] + + assert names.index("Verify freshly built image") < names.index("Push to ECR") < names.index("Push deps tag") + + build = yaml.safe_load((REPO_ROOT / ".github" / "workflows" / "build.yaml").read_text(encoding="utf-8")) + (base_build,) = [ + step for step in build["jobs"]["build"]["steps"] if step.get("uses") == "./.github/actions/ecr-build-push-pull" + ] + + assert base_build["with"]["verify-test-path"] == "docker/test/test_image_invariants.py" + + +def test_run_tests_links_isaac_sim_only_where_kit_is_installed(): + """The kit-less image has no Kit under ``/isaac-sim``, which the runtime mounts create anyway. + + Linking it as ``_isaac_sim`` there reads as a downloaded Isaac Sim, which ``isaaclab.sh`` + refuses to combine with the image's ``VIRTUAL_ENV``. + """ + script = (REPO_ROOT / ".github" / "actions" / "run-tests" / "run_tests.sh").read_text(encoding="utf-8") + + link_lines = [line.strip() for line in script.splitlines() if "ln -s /isaac-sim _isaac_sim" in line] + + assert link_lines + assert all("/isaac-sim/python.sh" in line for line in link_lines), link_lines + + def test_kitless_volume_key_resolves_owned_image_paths(monkeypatch: pytest.MonkeyPatch): """The explicit kit-less volume key resolves the paths prepared by its Dockerfile.""" monkeypatch.setenv("DOCKER_ISAACLAB_PATH", "/workspace/isaaclab") diff --git a/docker/test/test_image_invariants.py b/docker/test/test_image_invariants.py new file mode 100644 index 000000000000..74aea9920244 --- /dev/null +++ b/docker/test/test_image_invariants.py @@ -0,0 +1,54 @@ +# Copyright (c) 2022-2026, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md). +# All rights reserved. +# +# SPDX-License-Identifier: BSD-3-Clause + +"""Invariants asserted against a *built* container image. + +The image under test is named by ``IMAGE_TAG``; the tests skip when it is unset so a plain +``pytest docker/test`` stays green on a machine with no image. Run one explicitly with:: + + IMAGE_TAG=isaac-lab-base:latest pytest docker/test/test_image_invariants.py +""" + +from __future__ import annotations + +import os +import subprocess + +import pytest + +IMAGE_TAG = os.environ.get("IMAGE_TAG", "") + + +def _in_image(script: str) -> str: + """Run ``script`` with bash inside the image under test and return its stdout.""" + result = subprocess.run( + ["docker", "run", "--rm", "--entrypoint", "bash", IMAGE_TAG, "-lc", script], + capture_output=True, + text=True, + check=True, + ) + return result.stdout + + +@pytest.fixture(autouse=True) +def _require_image(): + if not IMAGE_TAG: + pytest.skip("IMAGE_TAG is unset; no built image to assert against") + + +def test_no_prebundled_package_lost_its_entry_point(): + """A dangling ``__init__.py`` in a prebundle stops Isaac Sim extensions loading. + + Isaac Sim shares prebundled packages between extensions as per-file symlinks, so deleting + or replacing one strands every symlink into it. #6329 added this invariant to the pip + install path after nvbugs 6343978, where it cost 438 error lines and 14 failed extensions. + The images now install with ``uv sync``, which never calls that code, so assert it here. + + Only ``__init__.py`` is fatal: the shipped image already carries dangling submodules and + ``.pyi`` stubs that no extension imports - 41 of them, against develop's 48 - mostly + generated protobuf stubs inside an Omniverse extension's own prebundle. + """ + broken = _in_image('find / -path "*pip_prebundle*" -xtype l -name "__init__.py" 2>/dev/null || true').strip() + assert not broken, "prebundled packages lost their entry point:\n" + broken