[StafaH]

Description

launch_simulation reads sys.platform during device selection without importing sys, causing simulation startup to fail with NameError on every platform. Add the missing standard-library import so the launcher can initialize the runtime.

Validation

• All 11 existing tests in test_launch_simulation_require_kit.py passed.
• The existing kitless-launch test reproduces the original NameError when the import is removed.
• Previously verified five-step GPU runs through the built wheel for Zoo, H1, Block and Tackle, Snowball Smash, and Teapot Fill.
• Full formatting, lint, and changelog checks passed against the upstream develop baseline.

Type of change

• Bug fix

Release backport

• Backport this pull request to the active release branch after it merges into develop

Checklist

• I have read and understood the contribution guidelines
• I have run pre-commit checks with uv run isaaclab --format
• No documentation changes are required for this import fix
• My changes generate no new warnings
• I have validated the changed behavior with existing focused coverage
• I have added a changelog fragment for the touched source package
• My name already exists in CONTRIBUTORS.md

[greptile-apps]

Confidence Score: 5/5

[Low risk] Adds missing import to simulation launcher.

The scoped launcher fix appears safe to merge.

Findings

1.   Mutable release artifact action ▶
2.  Default check may verify wrong version ▶
3.  Successful QA path lacks coverage ▶

Summary

The PR adds the missing sys import used by simulation launcher device selection and records the fix in a changelog fragment. Changes since the previous review remove the proposed uvx release and QA machinery, leaving the launcher fix as the PR’s scope.

_{Reviews (2) · Last reviewed commit: "Keep only the simulation launcher bug fi..."}

[greptile-apps]

Mutable release artifact action

The new PyPI publishing job runs actions/download-artifact@v7 with id-token: write, then publishes the wheel it places in dist/ without checking its integrity. If the mutable v7 reference is repointed, that action could replace the validated wheel before publication. Pin the artifact action in this publishing path to an immutable commit.

How this was verified: The credentialed publish job downloads the artifact through the mutable action and immediately passes the resulting wheel to uv publish.

[greptile-apps]

Default check may verify wrong version

If a future release is a prerelease, this unpinned PyPI check can install an earlier stable version instead of the wheel just published. That version could pass the catalog checks, leaving the release job green without confirming that the bare uvx isaaclab command selects this release. Compare the installed version in the QA report with RELEASE_VERSION.

[greptile-apps]

Successful QA path lacks coverage

These runner tests cover command failures and timeouts, but none exercises a successful catalog, probe, and help check or --run argument forwarding. Since the release gate depends on those paths, a regression that skips a check or builds the wrong successful command could go unnoticed. Add a controlled successful-path test.

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!

[StafaH]

run-ci

[isaaclab-review-bot]

Isaac Lab Review Bot

All reviewed code categories look good.

• PR description: Remove the unrelated prior GPU demo runs and qualify “every platform” as the device-selection path evaluating sys.platform; the focused test result is relevant, and no performance evidence is needed.
• Design and architecture: Architecture looks good.
• Scope and over-engineering: No over-engineering issues.
• API: API changes look good.
• Compatibility and deprecation: No breaking changes identified.
• Implementation: Implementation looks good.
• Style consistency: Style consistency looks good.
• Test quality: Test quality looks good.

No blocking issues. No inline findings.

Automated review; human maintainers own approval decisions.

[StafaH]

run-ci

[AntoineRichard]

🤖 AI review. Generated with Claude Code and checked by @AntoineRichard before posting.

Summary

The title and description describe a one-line sys import fix, but that fix is already on develop. #8305 (merged Oct 5) added import sys to sim_launcher.py and shipped its own fragment, fix-sim-launcher-sys-import.fixed.rst. This branch's merge base is #8190, so the sim_launcher.py hunk does nothing after merge.

What's left is the docs change from 1f1b1f35 (embedding benchmark snapshots so the downloaded HTML works offline). The description doesn't mention it.

Findings

1. One concern per PR (scope). About 98% of the diff is the docs feature: a Sphinx directive, a JS rewrite, an RST change and a new test. Suggest removing the sim_launcher.py change and retitling to something like [Docs] Embed benchmark snapshots so downloaded HTML works offline, with the description rewritten around that change. Alternatively, close this and open the docs change on its own.

2. Duplicate changelog entry. source/isaaclab/changelog.d/fix-simulation-launcher-platform-check.fixed.rst describes the same fix as #8305's fragment, so the release notes would list it twice. Suggest deleting it. The docs change probably needs only a .skip fragment.

3. The Validation section doesn't match the diff. The five-step GPU runs (Zoo, H1, Block and Tackle, …) are left over from the uvx-demos work. Nothing validates the docs change. Suggest replacing them with the pytest tools/test/test_doc_benchmarks.py command and its pass count, a docs build, and a check that environments.html works when opened from disk (file://).

4. Page weight (docs/_extensions/isaaclab_docs.py, IsaacLabBenchmarkData). The two CSVs (~135 KB develop + ~67 KB release) are inlined as csv.DictReader JSON. That repeats every header name on every row, so the payload is likely about 2× the CSV size, added to every load of environments.html. Suggest embedding the raw CSV text, or a compact {header, rows: [[...]]} shape. If the size is acceptable, a one-line note in the description with the added KB would cover it.

5. Leftover defensive code (environment-browser.js, renderBenchmarks). The try/catch, benchmarkErrors, and the data-benchmark-error UI handled fetch failures. They now only wrap synchronous filters that won't realistically throw. Meanwhile JSON.parse(document.querySelector("[data-environment-benchmark-rows]").textContent), the call that can fail when the directive is missing, sits outside the try. Suggest removing the error path. A missing directive is better caught at build time.

6. Test shape (tools/test/test_doc_benchmarks.py). The test runs two full sphinx -W builds in subprocesses, while sibling tests (test_doc_redirects.py, test_doc_source_links.py) call the extension directly. The </script> / quoted-value escaping case is a real regression guard and worth keeping. The second build mostly re-tests Sphinx's own note_dependency incremental tracking. Suggest dropping it, or adding one line to the description on why stale snapshots after a CSV refresh need their own guard.

Minor: the "release"/"develop" channels and the _static/benchmarks path are hardcoded in the directive, while data-benchmark-*-dates stays in the RST. This is fine as is; passing the channels as directive arguments would keep them in one place. Replacing the hand-written JS CSV parser with Python's csv is a good simplification.