[Backport] PR #7540 to release/3.0.0 - #7556
Closed
isaaclab-bot[bot] wants to merge 1 commit into
Closed
Conversation
…m only in Kit images (#7540) 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-<hash>` 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-active-release --> 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 df44b19)
github-actions
Bot
added
documentation
kellyguo11
marked this pull request as ready for review
September 4, 2026 02:19
kellyguo11
requested review from
hujc7,
kellyguo11 and
myurasov-nv
as code owners
September 4, 2026 02:19
Contributor
Greptile SummaryThis backport adds a pre-publication invariant check for freshly built base images and avoids treating kit-less runtime mounts as an Isaac Sim installation.
Confidence Score: 4/5The backport appears safe to merge after considering one non-blocking supply-chain hardening issue in the build workflow. The image-verification and kit-less symlink changes preserve the intended build and test flows, while the newly introduced mutable third-party action reference unnecessarily exposes a secret-bearing self-hosted job to upstream tag retargeting. Files Needing Attention: .github/workflows/build.yaml
|
| Filename | Overview |
|---|---|
| .github/actions/ecr-build-push-pull/action.yml | Adds optional host-side pytest verification after a full build and before either ECR publication step. |
| .github/workflows/build.yaml | Enables base-image verification and installs uv, but introduces a mutable third-party action reference in a credential-bearing self-hosted job. |
| .github/actions/run-tests/run_tests.sh | Restricts _isaac_sim symlink creation to images containing an executable Isaac Sim launcher. |
| docker/test/test_image_invariants.py | Adds a container-image test that rejects dangling prebundle __init__.py symlinks. |
| docker/test/test_container_profiles.py | Adds structural regression tests for verification ordering, workflow wiring, and conditional Isaac Sim linking. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
B[Build local base image] --> V[Run image invariant tests]
V -->|Pass| C[Tag commit image]
C --> P[Push commit tag to ECR]
P --> D[Push dependency-cache tag]
V -->|Fail| F[Fail job without publishing]
Reviews (1): Last reviewed commit: "[CI] Skip the image invariants on a deps..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| # The GPU runners have no uv on PATH; the invariant check below needs it. | ||
| - name: Set up uv | ||
| uses: astral-sh/setup-uv@v6 |
Contributor
There was a problem hiding this comment.
The new astral-sh/setup-uv@v6 reference is mutable while executing on a self-hosted runner with the workflow-level NGC_API_KEY and write-capable token permissions. Pinning the action to a reviewed commit prevents an upstream tag change from exposing credentials, altering the subsequent image build, or compromising the runner.
How this was verified: The changed action reference was traced to its job's workflow-level secret, token permissions, and self-hosted runner.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Isaac Lab Review Bot
The backport adds an opt-in pre-publication image invariant check to the ECR build action, enables it for the base image workflow, and avoids creating an _isaac_sim link for kit-less images.
- Design and architecture: The verification step is ordered after a full local build and before the commit and dependency image tags are pushed. Exact-tag and dependency-cache hits intentionally bypass verification; previously cached images therefore remain a residual compatibility risk until invalidated or rebuilt.
- API: The new
verify-test-pathaction input is optional and defaults to empty, preserving existing callers. Opting in requiresuvon the runner, which is documented and configured for the base-image build job. - Implementation: The verification command receives the image tag and test path through environment variables and propagates pytest failures before image publication. The guarded
_isaac_simsymlink preserves the Kit-based path while preventing kit-less images from being misidentified as downloaded Isaac Sim installations. Focused tests cover action ordering, workflow integration, and symlink guarding.
No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.
Automated review; human maintainers own approval decisions.
Contributor
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
Contributor
|
closing in favor of safer cherry pick in #7558 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backports #7540 to
release/3.0.0.The original cherry-pick conflicted. An NVIDIA inference model proposed this resolution, and deterministic validation confirmed that it changes no paths outside the original PR. Because conflict resolution cannot be certified as an exact patch replay, this PR is intentionally a draft and requires release-maintainer review.
df44b19aaec2d7750272b4f634b18f043e90c8c22af02510cc7cac6171fa25bf912f1c4dc83c3279717354a71abcab042fb671d7c91e38da661f279f