Reworks modular installation and add tests and updates docs#5650
Conversation
github-actions
Bot
added
bug
Greptile SummaryThis PR refactors the IsaacLab installation model from an explicit submodule enumeration (requiring users to know internal dependency chains) to a tiered system of core submodules always installed, optional heavy submodules, and named extra-feature sets with optional selectors. It adds conda CI infrastructure and a comprehensive suite of unit and integration tests covering the new install paths.
Confidence Score: 5/5The installation refactor is well-scoped, all I/O paths are unit-tested with mocks, and E2E integration tests cover the major install combinations. The install dispatch logic is correct, the bracket-depth parser handles nested commas properly, and the conda mixin cleanup is guarded with hasattr checks. All findings are style or UX polish with no functional correctness issues. source/isaaclab/isaaclab/cli/commands/install.py — the newton/visualizer auto-extra overlap and the contrib no-selector messaging are minor but worth a second look. Important Files Changed
Reviews (2): Last reviewed commit: "install contrib by default" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if "[" in token: | ||
| bracket_pos = token.index("[") | ||
| name = token[:bracket_pos].strip() | ||
| editable = token[bracket_pos:].strip() | ||
| selector = token[bracket_pos + 1 : token.index("]")].strip() |
There was a problem hiding this comment.
ValueError for malformed bracket tokens
token.index("]") raises ValueError when a token contains [ but no closing ] (e.g., the user types rl[rsl-rl by accident). The old code used token[bracket_pos:] and never called .index("]"), so it was immune to this. The install command will crash with an unhandled exception instead of emitting a warning and continuing. The new test file has no test covering this case.
| if feature_name == "newton": | ||
| print_info("Installing newton extras (newton[sim], PyOpenGL-accelerate, imgui-bundle)...") | ||
| run_command(pip_cmd + ["install", "--editable", f"{source_dir}/isaaclab_newton[all]"]) |
There was a problem hiding this comment.
selector parameter is silently ignored for the newton feature. If a user passes newton[sim] or any other selector, it is discarded without warning, which can be confusing since every other feature respects its selector. A warning makes the contract explicit.
| if feature_name == "newton": | |
| print_info("Installing newton extras (newton[sim], PyOpenGL-accelerate, imgui-bundle)...") | |
| run_command(pip_cmd + ["install", "--editable", f"{source_dir}/isaaclab_newton[all]"]) | |
| if feature_name == "newton": | |
| if selector: | |
| print_warning( | |
| f"'newton' does not support selectors (got '{selector}'). " | |
| "Installing all newton extras." | |
| ) | |
| print_info("Installing newton extras (newton[sim], PyOpenGL-accelerate, imgui-bundle)...") | |
| run_command(pip_cmd + ["install", "--editable", f"{source_dir}/isaaclab_newton[all]"]) |
| " Install optional heavy dependencies for a feature on top of the core.\n" | ||
| " Supports an optional selector in brackets:\n" | ||
| " rl[rsl-rl|skrl|sb3|rl-games] (default: all)\n" | ||
| " visualizer[newton|rerun|viser] (default: all)\n" |
There was a problem hiding this comment.
newton|rerun|viser as visualizer selectors, but the docs table in both RST files (and the isaaclab_visualizers extras) also include kit. Omitting it here leaves users unaware of the kit backend.
| " visualizer[newton|rerun|viser] (default: all)\n" | |
| " visualizer[kit|newton|rerun|viser] (default: all)\n" |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔬 Code Review: Modular Installation Rework
Overview
This PR introduces a significant architectural improvement to Isaac Lab's installation system, moving from a submodule-centric model to a cleaner core+extras approach. The design is sound and well-documented.
🔴 Critical: CI Test Collection Failure
File: source/isaaclab/test/install_ci/test_install_command_parsing.py:28
The Installation Tests CI job fails during test collection:
ModuleNotFoundError: No module named 'isaaclab'
Root Cause: The test file attempts to import from isaaclab.cli.commands.install before the package is installed in the CI environment. Line 26-27 adds the source path to sys.path:
_ISAACLAB_SRC = Path(__file__).resolve().parents[4] / "isaaclab"
if str(_ISAACLAB_SRC) not in sys.path:
sys.path.insert(0, str(_ISAACLAB_SRC))However, from test_install_command_parsing.py, parents[4] resolves to the repository root, but the target should be source/isaaclab/isaaclab (the actual Python package directory), not source/isaaclab.
Fix: Change line 26 to:
_ISAACLAB_SRC = Path(__file__).resolve().parents[3] # source/isaaclabOr more explicitly:
_ISAACLAB_SRC = Path(__file__).resolve().parents[4] / "source" / "isaaclab"🟡 Warnings
1. CHANGELOG.rst Entry Missing
Per CONTRIBUTING.md, user-facing changes require a changelog entry. This PR significantly changes the CLI interface for --install and should be documented.
2. Documentation: Inconsistent Framework Name Separator
The docs use both rsl-rl (hyphen) and show examples, but the old code used rsl_rl (underscore). While the new code handles both, the docs should clarify if both are valid or if one is deprecated.
Update (da8f581): Empty commit to retrigger CI — no code changes. Previous review comments still apply.
Update (887bf19): Improves Docker bind-mount permission handling in tools/run_install_ci.py. The change iterates over existing files in the results directory and sets their permissions to 0o666, preventing PermissionError when stale files owned by root exist from previous CI runs. This is a good defensive fix for CI reliability. ✅ No concerns with this change.
Update (82d505e): Significant improvement to JUnit XML results handling in tools/run_install_ci.py. Instead of bind-mounting the results directory (which caused permission issues), the script now:
- Creates a named container (using
uuidfor uniqueness) instead of using--rm - Writes pytest results to a container-internal path (
/tmp/isaaclab-installci-results.xml) - Copies results out using
docker cpafter the container exits - Properly cleans up with
docker rm -fin afinallyblock - Handles timeout gracefully by killing the named container
This is an excellent architectural change that completely avoids the root-owned-file permission issues inherent in bind-mount approaches. The implementation includes proper error handling and warnings. ✅ Well done!
Update (47679f3): Major refinement of the optional submodule/extra feature model:
Changes:
contribandovare now optional submodules (not extra features)ovmaps to two packages:isaaclab_ov+isaaclab_ovphysx- Removed
isaaclab_contrib,isaaclab_ov,isaaclab_ovphysxfrom core submodules - New
_install_optional_submodule_extra_dependencies()handles dependency extras:contrib[rlinf]for rlinf dependenciesov[ovrtx|ovphysx|all]for OV runtime wheels
- Updated docs with clear examples for new selector syntax
- Test preset changed from
presets=newton→presets=newton_mjwarp - Comprehensive test coverage added for the new model
Assessment:
✅ The architectural separation is cleaner — core packages are truly lightweight, while heavy dependencies require explicit opt-in via selectors. This matches the stated goal of making base installation faster.
✅ The _install_optional_submodule_extra_dependencies() implementation properly validates selectors and handles the all convenience selector for ov.
MANUAL_EXTRA_FEATURES set is now empty (set()). If this is intentional and there are no longer features excluded from all, the constant might be removed for clarity (or documented as a placeholder for future use).
Overall, this commit significantly improves the installation model. The CI test collection issue from the original review may still need addressing.
Update (e2d8a46): Excellent refinement! This commit addresses the previous concern about MANUAL_EXTRA_FEATURES being empty.
Key architectural change: isaaclab_ov and isaaclab_ovphysx are now core submodules (always installed). This allows task configs to import OV preset classes without requiring the heavy OV runtime wheels.
What changed:
ovmoved fromOPTIONAL_ISAACLAB_SUBMODULEStoVALID_EXTRA_FEATURESMANUAL_EXTRA_FEATURES = {"ov"}— OV runtime deps excluded from-i all(addresses previous concern ✅)- New dedicated
_install_ov_extra_dependencies()function for cleaner code separation - Docs clarified: OV source packages are core; use
ov[ovrtx|ovphysx|all]for runtime wheels - Tests updated to verify new behavior
Assessment: ✅ This is a well-thought-out design decision. Users get the OV abstractions/presets out of the box, but the heavy runtime wheels (~GB) remain opt-in. The code is cleaner with the dedicated OV handler, and the test coverage properly validates the new model.
No new concerns with this commit.
Update (4b653b4): Moves isaaclab_contrib from optional submodule back to core. This is the right call — other core packages (like isaaclab_tasks) may import contrib utilities, so it shouldn't require explicit opt-in. The heavy runtime deps (rlinf) still require contrib[rlinf] selector. contrib is now an extra feature (like ov) — source always installed, heavy deps opt-in.
Final architecture after all commits:
| Tier | Packages | When installed |
|---|---|---|
| Core (always) | isaaclab, assets, contrib, experimental, newton, ov, ovphysx, physx, rl, tasks, tasks_experimental, visualizers | Every -i invocation |
| Optional submodules | mimic, teleop | -i all or explicit token |
| Extra features (auto) | newton, rl, visualizer | -i all (default) |
| Extra features (manual) | contrib[rlinf], ov[ovrtx/ovphysx] | Explicit selector only |
Verdict: Ready to merge. ✅ All CI green (39 checks). The installation model is clean, well-tested, and has converged on a sensible design. No remaining concerns.
…public into fix-modular-install
# Description Fixes the uv run path that got broken by #5650. Updates the available options to align closer with the new logic for ./isaaclab.sh -i modular installation. Also adds more unit tests for uv run and pip package. ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) ## Checklist - [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 - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [ ] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task -->
1 participant
Description
Refactors the IsaacLab installation model to simplify the user experience, adds
comprehensive installation CI tests (including conda).
Motivation
The previous installation model required users to name every individual submodule
(
./isaaclab.sh -i assets,tasks,physx,contrib,newton,rl[rsl-rl]), exposing internalimplementation details. Users shouldn't need to know that
tasksdepends onphysxor that
contribexists — they just want to train with Newton or run with Isaac Sim.Changes
1. Installation model refactor
source/isaaclab/isaaclab/cli/commands/install.pyVALID_ISAACLAB_SUBMODULES/VALID_RL_FRAMEWORKSwith three typedconstants that make the tier structure explicit:
CORE_ISAACLAB_SUBMODULES: list[str]— always installed (isaaclab, assets,contrib, experimental, newton, ov, ovphysx, physx, rl, tasks, tasks_experimental,
visualizers)
OPTIONAL_ISAACLAB_SUBMODULES: dict[str, str]— opt-in heavy submodules(
mimic,teleop)VALID_EXTRA_FEATURES: set[str]— opt-in heavy dependency groups(
contrib,newton,ov,rl,visualizer)command_install(install_type)to always install all core submodules,then layer optional submodules and extra feature dependencies on top.
_install_extra_feature(feature_name, selector)replacing_install_extra_frameworks../isaaclab.sh -i(no args) or-i allinstalls everything includingmimicand
teleop../isaaclab.sh -i noneinstalls all core submodules with no optional extras.tasks,assets) emit a[WARNING]and are skippedgracefully.
source/isaaclab/isaaclab/cli/__init__.py-ihelp text to document the three-tier model with examples.source/isaaclab/setup.pyEXTRAS_REQUIREtoisaacsimandallonly.source/isaaclab_mimic/setup.pyrobomimicextra.pyproject.toml(root)[project.dependencies]: lists all core submodules (bare, no extras).[project.optional-dependencies]: simplified toisaacsimandall.2. Documentation
docs/source/setup/installation/include/src_build_isaaclab.rstsets with their selectors; updated all example commands.
docs/source/setup/installation/kitless_installation.rst3. Installation CI tests
New and updated test files
test_install_command_parsing.py_split_install_items, constant consistency, andcommand_installdispatch logic (all mocked, no pip)test_isaaclabx_i_none.py./isaaclab.sh -i noneinstalls all core submodules; optional submodules absenttest_isaaclabx_i_rl.pyrl[rsl-rl],rl[skrl],rl[sb3]each install the right framework;rl(no selector) installs alltest_isaaclabx_i_mimic.pymimicis importable after-i mimic; absent after-i nonetest_isaaclabx_i_visualizer.pyvisualizer[rerun],visualizer[viser],visualizer(all) install the right backendstest_install_workflow_training.pytest_isaaclabx_i_physx.py-i physx)test_isaaclabx_uv_smoke.pyassets/tasksare always-installed core;newtonis an extratest_isaaclabx_uv_training.pynewton,rl[all]test_install_workflow_training.py— E2E matrixtest_uv_none_installs_core_submodules-i none@uvtest_uv_newton_rsl_rl_trains_cartpole-i newton,rl[rsl-rl]@uvtest_uv_newton_ov_rsl_rl_trains_cartpole-i newton,ov,rl[rsl-rl]@uvtest_uv_all_trains_cartpole-i all@uvtest_conda_none_installs_core_submodules-i none(conda env)@condatest_conda_newton_rsl_rl_trains_cartpole-i newton,rl[rsl-rl](conda env)@condasource/isaaclab/test/install_ci/utils.pydrop_keys(env, keys)helper for stripping venv/conda activation markersbefore creating isolated environments.
Conda_Mixinwithcreate_conda_env(),destroy_conda_env(), andrun_in_conda_env()for conda-based test classes.source/isaaclab/test/install_ci/conftest.pycondaandtimeoutas known pytest markers.4. Conda CI infrastructure
docker/Dockerfile.installci-conda(new)Dockerfile.installciimage.install-tests-condaCI job.tools/run_install_ci.py_build_image()helper.--condaflag: builds the uv base image first, then the conda layer ontop; routes to the conda image for the Docker run.
.github/workflows/install-ci.ymlInstallation Tests (uv); added-m uvso it only runsuv-marked tests.
Installation Tests (conda)job with--condaand-m conda,timeout-minutes: 150(extra headroom for two-stage Docker build).render_table/any_matchshell functions: replacedprintf '%s\n' "$files" | grepwithgrep <<< "$files"to avoid broken-pipesignals when
grepexits early with-mor-q.Testing
test_install_command_parsing.py: 41 unit tests, all pass without GPU or network../isaaclab.sh -i newton,ov,rl[rsl_rl]→ clean install, no warnings./isaaclab.sh train --rl_library rsl_rl --task Isaac-Cartpole-Direct-v0 --num_envs=16 --max_iterations=10 "presets=newton" --headless→ 10 iterations, exit 0, ~2900 steps/secMigration guide
./isaaclab.sh -i assets,tasks,physx,contrib./isaaclab.sh -i none(all core always installed)./isaaclab.sh -i assets,tasks,ov,rl[rsl-rl]./isaaclab.sh -i ov,rl[rsl-rl]./isaaclab.sh -i newton,rl[all]./isaaclab.sh -i mimic,teleopuv pip install isaaclab[tasks,rl,assets]uv pip install isaaclab[all]