Updates environment and installation docs#5879
Conversation
kellyguo11
requested review from
Mayankm96,
hhansen-bdai and
jtigue-bdai
as code owners
github-actions
Bot
added
documentation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Documentation Update Review
This PR introduces significant improvements to the documentation infrastructure and environment listing. Here's my analysis:
Highlights ✨
1. Auto-generated Environment List
- New tooling (
tools/environ_docs.py,tools/update_environments_rst.py) that generates the comprehensive environment list from the Gym registry - This is a great maintainability win - ensures docs stay in sync with actual registered environments
- Well-structured with proper test coverage in
tools/test/test_environ_docs.py
2. Sphinx Extension for Clone Snippets
- New
docs/_extensions/isaaclab_docs.pyprovides custom directives likeisaaclab-clone-commands,isaaclab-clone-https,isaaclab-quickstart-install - Uses a configurable
isaaclab_latest_branch(default:release/3.0.0-beta2) that's centrally managed - Eliminates duplication of clone commands across multiple doc pages
3. Updated References to release/3.0.0-beta2
- README.md links updated from
maintorelease/3.0.0-beta2 - Version table updated accordingly
Suggestions & Questions
1. Selective Install Documentation
- The new
include/selective_install.rstprovides good coverage of the modular install system - Consider adding a brief note about when to use
isaacsimvs kit-less mode for users who are unsure
2. Environment Count Validation
- The generated table shows 166 training environments - is there a CI check to validate this number doesn't decrease unexpectedly?
- The
--checkflag inupdate_environments_rst.pyis good for CI, but consider adding it to pre-commit hooks
3. Copy Button CSS
- The change in
custom.cssto keep copy button always visible (opacity: 1) is nice for discoverability
4. Removed Compatibility Warning
- The PR removes the compatibility warning about Isaac Lab develop vs Isaac Sim develop branches
- Is this intentional because the breaking change was resolved?
5. Minor Observations
- In
docs/conf.py, the newrst_prologsets the substitution for|isaaclab_latest_branch|- good approach for consistency - The
isaaclab_docs.pyextension is well-documented with proper typing
Code Quality
- Good separation between generation logic (
environ_docs.py) and the CLI script (update_environments_rst.py) - Test coverage looks reasonable for the new tooling
- The
RL_LIBRARY_OVERRIDESdict for tasks without cfg entry points is a practical solution
Overall this is a well-structured documentation improvement that will reduce maintenance burden and ensure consistency across the docs.
Greptile SummaryThis PR introduces maintainer tooling to auto-generate the comprehensive environment table in
Confidence Score: 4/5Safe to merge — all changes are documentation and maintainer tooling with no impact on runtime code. The core logic in docs/_extensions/isaaclab_docs.py — the Important Files Changed
Reviews (1): Last reviewed commit: "Updates environment and installation doc..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def run(self) -> list[nodes.Node]: | ||
| branch = _branch(self.config) | ||
| platform = self.options["platform"].strip().lower() | ||
| if platform not in {"linux", "windows"}: | ||
| raise self.error(f"Unsupported platform '{platform}'. Use 'linux' or 'windows'.") |
There was a problem hiding this comment.
:platform: silently raises KeyError
self.options["platform"] raises a bare KeyError when the directive author omits :platform:. directives.unchanged_required validates the value only when the option is present; it does not make the option required by the framework. Any .rst file that includes .. isaaclab-quickstart-install:: without :platform: linux/windows will produce a cryptic traceback at doc-build time rather than a helpful directive error.
| def _branch(config) -> str: | ||
| """Return the release branch pinned in installation docs.""" | ||
| return getattr(config, "isaaclab_latest_branch", "main") |
There was a problem hiding this comment.
_branch fallback is "main", but the registered default in setup() is "release/3.0.0-beta2". These two values should agree so that if the extension is ever used outside a fully-configured Sphinx app, the branch embedded in clone snippets does not silently revert to main.
| def _branch(config) -> str: | |
| """Return the release branch pinned in installation docs.""" | |
| return getattr(config, "isaaclab_latest_branch", "main") | |
| def _branch(config) -> str: | |
| """Return the release branch pinned in installation docs.""" | |
| return getattr(config, "isaaclab_latest_branch", "release/3.0.0-beta2") |
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!
| def find_inference_task_name(task_id: str, registry_ids: set[str]) -> str | None: | ||
| """Return the inference/play task ID paired with *task_id*, if registered.""" | ||
| base = task_id.rsplit("-", 1)[0] | ||
| for suffix in _INFERENCE_TASK_SUFFIXES: | ||
| candidate = f"{base}{suffix}" | ||
| if candidate in registry_ids: | ||
| return candidate | ||
| return None |
There was a problem hiding this comment.
task_id.rsplit("-", 1)[0] always drops the trailing token (e.g. v0), then appends -Play-v0 / -Eval-v0 to build the candidate. If a future task is registered as -v1 or -v2, the lookup will silently fail to find its matching -Play-v1 or -Eval-v1 inference variant, producing a blank "Inference Task Name" cell.
There was a problem hiding this comment.
🔍 Code Review — Updated for Latest Push (cf15634)
This PR makes strong structural improvements to documentation maintainability. I've re-reviewed after the second commit ("allow for kitless pip install") and have some actionable findings:
Finding 1: Algorithm Label Accuracy Loss in Auto-Generated Environment Table
Severity: Medium | tools/environ_docs.py
The _infer_algorithm() function defaults to "PPO" when there's no suffix on the entry-point key. This causes information loss for tasks that previously had distinct algorithm labels:
Isaac-Repose-Cube-Shadow-OpenAI-FF-Direct-v0: was rl_games (FF) → now rl_games (PPO)Isaac-Repose-Cube-Shadow-OpenAI-LSTM-Direct-v0: was rl_games (LSTM) → now rl_games (PPO)
The issue is at line ~277 in environ_docs.py:
def _infer_algorithm(algo_suffix: str, entry_value: object, library: str) -> str:
if not algo_suffix:
if library == "rl_games" and isinstance(entry_value, str) and "vision" in entry_value.lower():
return "VISION"
return "PPO" # ← loses FF/LSTM distinctionSuggested fix: Extend the heuristic to check for "lstm" or "ff" in the entry-point value string, similar to the existing "vision" check. Alternatively, add these to RL_LIBRARY_OVERRIDES.
Finding 2: --enable_cameras Annotation Dropped Without Replacement
Severity: Medium | docs/source/overview/environments.rst
The old table included (Requires running with --enable_cameras) annotations on camera-based environments (e.g., Isaac-Cartpole-Camera-Showcase-*, Isaac-Repose-Cube-Shadow-Vision-Direct-v0). The auto-generated table removes these entirely without any alternative indication.
Users running these tasks without --enable_cameras will get non-obvious failures. Consider adding a footnote or a "Camera" workflow tag, or noting the requirement in the environ_docs.py row generation (perhaps from a special entry-point kwarg).
Finding 3: [kitless] Extra Includes PyOpenGL-accelerate — Platform Compatibility
Severity: Low | tools/wheel_builder/res/python_packages.toml
The new [kitless] extra pins PyOpenGL-accelerate==3.1.10. This package requires compilation (C extension) and can fail on systems without appropriate build tools. On Windows especially, pre-built wheels may not be available for all Python versions.
{ "kitless" = [
...
"PyOpenGL-accelerate==3.1.10",
...
] }Consider adding a note in the pip installation docs that this may require build tools, or make this optional (e.g., platform-specific markers).
Finding 4: selective_install.rst Is an Include Fragment — Verify .. _installation-selective-install: Label Placement
Severity: Low | docs/source/setup/installation/kitless_installation.rst
The cross-reference label .. _installation-selective-install: is defined in kitless_installation.rst just before the .. include:: include/selective_install.rst directive. Since the label lives outside the included content, it correctly anchors to the section. However, selective_install.rst starts with a ~~~~~~~~ heading (subtitle level). If it's ever included from a different context with different heading hierarchy, the heading levels could conflict.
This is fine for now but worth noting for future includes.
Finding 5: isaaclab_pip_installation.rst — [all] Extra Description Ambiguity
Severity: Low | docs/source/setup/installation/isaaclab_pip_installation.rst
The extras table states:
all— RL frameworks only (SB3, SKRL, RSL-RL). Combine withisaacsimfor a full install
But the source code in python_packages.toml shows [all] also includes onnxscript, tqdm, and rich — not just RL frameworks. The description "RL frameworks only" is slightly misleading. Consider: "RL frameworks and common training utilities."
Finding 6: Windows Quickstart Snippet Missing uv Install Step
Severity: Low | docs/_extensions/isaaclab_docs.py
The _quickstart_kitless function includes a curl ... | sh uv installation step for Linux but the Windows variant assumes uv is already installed. While uv installation differs by platform, new users on Windows may not have uv. A comment pointing to uv's install docs (e.g., :: Install uv: https://docs.astral.sh/uv/getting-started/installation/) would help.
Overall Assessment
This is a well-structured PR that significantly improves documentation maintainability:
- ✅ Centralized branch management via Sphinx config
- ✅ Auto-generated environment table with proper CI check flag (
--check) - ✅ Good test coverage for the new tooling
- ✅ Clean removal of stale compatibility warnings
- ✅ New
[kitless]pip extra properly separates Newton-only installs
The most impactful findings are #1 (algorithm labels) and #2 (camera annotations), which represent information loss in the migration from manual to auto-generated docs.
There was a problem hiding this comment.
📋 Code Review — PR #5879 (Updated for Latest Commit 7f50812)
This PR delivers substantial documentation infrastructure improvements: auto-generated environment tables, shared Sphinx directives for clone/install commands, and a new kitless pip extra. The third commit corrects the default branch from release/3.0.0-beta2 to develop and adds a README compatibility warning.
✅ What Works Well
- DRY documentation via Sphinx directives —
isaaclab_docs.pyeliminates copy-paste of clone/install commands across 6+ pages. Branch updates now happen in one place. - Auto-generated environment table —
tools/environ_docs.pywithSTART-AUTO-GENERATED/END-AUTO-GENERATEDmarkers makes the comprehensive table maintainable. The--checkflag enables CI enforcement. - Strong test coverage —
tools/test/test_environ_docs.py(246 lines) covers parsing, rendering, and patching logic. - Changelog entry — Breaking changes to pip extras are clearly documented.
- Modular install documentation — Refactoring the selective install section into a standalone include (
selective_install.rst) improves reusability.
🔍 Findings
Finding 1: Default Branch Inconsistency Between conf.py and README
Severity: Low | README.md + docs/conf.py
The README references release/3.0.0-beta2 in the version table while conf.py now defaults isaaclab_latest_branch to develop. This is intentional (the table tracks Isaac Sim compatibility) but could confuse contributors about which branch to clone.
Suggestion: Add a brief comment in the README version table clarifying that clone commands use develop (via Sphinx) while the table maps stable releases.
Finding 2: Algorithm Label Fallback in _infer_algorithm() May Lose Precision
Severity: Medium | tools/environ_docs.py
def _infer_algorithm(algo_suffix: str, entry_value: object, library: str) -> str:
if not algo_suffix:
if library == "rl_games" and isinstance(entry_value, str) and "vision" in entry_value.lower():
return "VISION"
return "PPO"Tasks using LSTM or FF-specific configs (e.g., Isaac-Repose-Cube-Shadow-OpenAI-FF-Direct-v0 which previously listed rl_games (FF)) now show rl_games (PPO) because the algorithm suffix is empty and the entry-point string does not contain "vision". Similarly, Isaac-Repose-Cube-Shadow-OpenAI-LSTM-Direct-v0 previously listed rl_games (LSTM).
Suggestion: Extend the heuristic to detect "lstm" and "ff" in entry_value strings, or introduce a manual override map similar to RL_LIBRARY_OVERRIDES.
Finding 3: _domain_presets_for_docs Filtering Logic
Severity: Low | tools/environ_docs.py
The function filters domain presets that duplicate physics/renderer names, which is sensible. However, this means if a task has domain presets named identically to physics backends (but with different semantics), they would be silently omitted. The test test_format_presets_rst_hides_domain_names_duplicated_by_physics validates this is intentional, but a code comment explaining the rationale would help future maintainers.
Finding 4: Deleted pip_extras_note.rst Without Migration Note
Severity: Low | docs/source/setup/installation/include/pip_extras_note.rst
The old note explaining that [all] extras were needed for training scripts is deleted. The new documentation explains [kitless] and [isaacsim,all] well, but users who relied on the [rl,tasks] extras pattern from external links/bookmarks will get 404s. Consider adding a brief mention in the changelog or migration section.
Finding 5: rl_games Excluded from [kitless] and [all] Extras
Severity: Low | tools/wheel_builder/res/python_packages.toml
Both all and kitless extras have a commented-out rl-games entry with a TODO note about Python 3.11+ support. The pip installation docs correctly document this and direct users to install the fork manually. This is fine, but the TODO should track which upstream issue to watch.
Finding 6: exclude_patterns in conf.py Now Hides include/*
Severity: Low | docs/conf.py
"source/setup/installation/include/*",This ensures include fragments are not built as standalone pages, which is correct. However, if a new fragment is added to include/ and the developer forgets to use .. include::, it will silently vanish from the build rather than raising an error.
Finding 7: Hardcoded Isaac Sim Version in Directives
Severity: Low | docs/_extensions/isaaclab_docs.py
The _quickstart_isaacsim function hardcodes isaacsim[all,extscache]==6.0.0 and torch==2.10.0. When Isaac Sim 6.1 releases, these will need manual updates. Consider extracting version pins to conf.py config values (similar to isaaclab_latest_branch) for single-source-of-truth versioning.
Summary
| Category | Count |
|---|---|
| Strengths noted | 5 |
| Medium severity | 1 |
| Low severity | 6 |
Overall this is a well-structured documentation infrastructure PR that significantly reduces maintenance burden. The primary actionable item is the algorithm label precision loss (Finding 2) — it causes a regression in information accuracy for ~2-3 environment entries in the generated table compared to the previous hand-maintained version. The remaining findings are minor improvements for future maintainability.
CI checks are currently pending on the latest push.
Update (commit 3a75af7): Reviewed incremental changes since 7f50812.
Previous findings status:
- ✅ Finding 3 (
_domain_presets_for_docsfiltering logic): Addressed — new_PHYSICS_BACKEND_MIRROR_NAMESfrozenset makes the filtering explicit with clear documentation, and_physics_names_for_docsadds implicit PhysX detection. Well-tested with 3 new test cases. - ✅ Inline comment on
isaaclab_docs.py:145(Windows missinguvinstall): Partially addressed — both Windows snippets now include a:: Install uv:comment with a link to the uv installation docs. This is a batch comment (not executable), but it clearly guides users to install uv first. - Remaining findings (1, 2, 4–7 and inline comments on lines 103, 18, 245) were not addressed in this push — original comments still stand.
New changes reviewed (no new issues found):
_infer_implicit_physics_names()correctly detects implicit PhysX fromdefault = PhysxCfg(...)fields_physics_names_for_docs()integration intocollect_environment_doc_rowsis clean- Environment table updates in
environments.rst(adding(direct only)qualifiers,newton_mjwarpfor Digit, removing stalepresets=entries) look accurate - New tests provide good coverage of the preset filtering edge cases
Update (commit 2b8740e): Formatting-only changes. No new issues.
Changes reviewed:
- Import reordering in
tools/environ_docs.pyandtools/test/test_environ_docs.py - Refactored
try/excepttocontextlib.suppress()pattern - Line wrapping adjustments
All previous findings remain unchanged — see above.
Update (commit 8e69441): Reviewed incremental changes since 2b8740e.
Previous findings status:
- ✅ Finding 5 (
rl_gamesexcluded from extras): Fully addressed —rl-gamesis now included in[all],[kitless], and[rl-games]extras via the Python 3.11+ compatible fork. The manual install note in docs is correctly removed, and thekitless/alldescriptions now list RL-Games. Test coverage added intest_wheel_builder_metadata.py. - Remaining findings (1, 2, 4, 6, 7 and inline comments on lines 103, 18, 245) were not addressed in this push — original comments still stand.
New changes reviewed (no new issues):
- Digit environments correctly updated to remove
newton_mjwarpphysics support (closed-loop articulation incompatibility) - New
.. note::with cross-reference anchorknown-issues-closed-loop-newtoninenvironments.rstand matching label inissues.rst— clean Sphinx cross-referencing python_packages.tomlproperly declares transitive dependencies (aiohttp,gym,standard-distutils) alongside therl-gamesfork in all three aggregate extras (rl-games,all,kitless)- Changelog entry (
rl-games-wheel-extra.rst) is well-formatted - Test refactor extracting
_isaaclab_rl_optional_dependencies()helper is clean; newtest_wheel_builder_rl_games_extra_matches_source_packagevalidates consistency
Update (commit 6a30379): Reviewed incremental changes since 8e69441.
Previous findings status:
⚠️ Finding 5 (rl_gamesin extras): Previously marked as fixed, but this commit intentionally reverts it —rl-gamesis now commented out from[all]extras with a TODO for when the upstream package supports Python 3.11. Docs correctly direct users to install the fork manually. This is a deliberate simplification, not a regression.kitlessextra removed entirely — All references to[kitless]have been removed from docs, tests, andpython_packages.toml. The[all]extra now serves the "RL frameworks without Isaac Sim" use case. This is a clean simplification that reduces the install matrix.- Remaining findings (1, 2, 4, 6, 7 and inline comments on lines 103, 18, 245) were not addressed in this push — original comments still stand.
New changes reviewed (no new issues):
pip_extras_note.rstadded as a reusable include — clean 4-line note- Docs now describe only two extras (
isaacsimandall) — consistent throughout rl_gamesmanual install instructions added with correct fork URL and branch- Changelog entry (
rl-games-wheel-extra.rst) correctly removed since the feature was reverted - CI install test updated from
[kitless]to[all]— correct since[all]installs RL frameworks without requiring Isaac Sim - Test cleanup in
test_wheel_builder_metadata.pyremoveskitlessreferences and the rl-games test — consistent with the package changes
Signed-off-by: Kelly Guo <kellyg@nvidia.com>
Signed-off-by: Kelly Guo <kellyg@nvidia.com>
…nvs, shutdown error, mjcf fix (#5888) # Description Cherry-pick PRs from develop: - #5866 - #5879 - #5882 - #5884 - #5889 --------- Signed-off-by: Yize Wang <yizew@nvidia.com> Signed-off-by: Kelly Guo <kellyg@nvidia.com> Co-authored-by: YizeWang <37894497+YizeWang@users.noreply.github.com> Co-authored-by: Yize Wang <yizew@nvidia.com>
1 participant
Description
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there