Add loading screen for train/play, default newton and rsl_rl across core - #6933
Conversation
Greptile SummaryThe PR adds an interactive startup screen around train/play initialization and allows unified entrypoints to infer an RL backend from task registration. It also changes the shipped core-task defaults to Newton MJWarp and the Newton renderer.
Confidence Score: 5/5The PR appears safe to merge with no concrete changed-code defects identified. The loading-screen lifecycle, backend dispatch fallback, inherited task presets, and coordinated Newton defaults have reachable implementations consistent with their documented startup flows, and the security lead concerned unchanged pre-existing parsing behavior. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as Unified train/play CLI
participant Registry as Task registry
participant Screen as LoadingScreen
participant Launcher as Simulation launcher
participant Env as Isaac Lab environment
participant Agent as RL backend
CLI->>Registry: Resolve task and default RL library
CLI->>Screen: Open and show run summary
Screen->>Screen: Redirect startup output
CLI->>Launcher: Launch selected simulation backend
Launcher->>Screen: Report startup activities
CLI->>Env: Create environment
Env->>Screen: Report scene, physics, and manager setup
CLI->>Agent: Build wrapper and policy
CLI->>Screen: Close and restore console
CLI->>Agent: Begin training or playback
Reviews (1): Last reviewed commit: "Make startup more detailed, add logo" | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The loading-screen integration, activity reporting, optional RL-library resolution, and Newton-default migration are broadly coherent. Before merge, add the required isaaclab_newton changelog fragment and ensure a task’s marked default RL library is validated against the supported libraries before dispatch.
- Design and architecture: The loading screen is cleanly scoped to startup, with output spooling and replay on failure, while the global report_activity hook keeps instrumentation decoupled from callers. The backend resolver has one boundary issue: registry metadata is trusted without checking that the selected library is supported by the requested action, allowing dispatch to fail with an uncaught KeyError.
- API: LoadingScreen, format_run_summary, and report_activity are exported and documented, and Timer(activity=...) is an additive keyword-compatible extension. resolve_rl_library should uphold its documented selection boundary by returning a marked default only when it appears in the supplied libraries collection; otherwise it should fall back or return None.
- Implementation: The screen lifecycle, progress handling, ticker shutdown, and file-descriptor restoration are internally consistent. The isaaclab_newton Timer instrumentation also requires a changelog fragment for that touched package under the repository’s per-package changelog rule.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| NewtonManager._pending_extended_state_attributes = set() | ||
| cls._prepare_builder_for_finalize(cls._builder) | ||
| with Timer(name="newton_finalize_builder", msg="Finalize builder took:"): | ||
| with Timer(name="newton_finalize_builder", msg="Finalize builder took:", activity="Finalizing physics model"): |
There was a problem hiding this comment.
🟡 Warning · Implementation — Missing changelog fragment for isaaclab_newton
This PR modifies source/isaaclab_newton/ (Timer activity labels here and at lines 2010, 2081) but adds no fragment under source/isaaclab_newton/changelog.d/, while isaaclab, isaaclab_rl, and isaaclab_tasks each received one. Repository rules require one fragment per touched package so the nightly job can compile the changelog and version bump. Add <slug>.rst, or <slug>.skip if these labels are considered non-user-facing.
| except gym.error.Error: | ||
| return None | ||
| marked = spec.kwargs.get("default_rl_library") | ||
| if marked is not None: |
There was a problem hiding this comment.
🟡 Warning · Api — Marked RL library bypasses libraries filter
resolve_rl_library documents that it returns a name from libraries or None, but the default_rl_library branch returns the registered value unchecked. dispatch.run_cli then evaluates backends[library], so a task marking a library that is not a supported backend for the requested action raises an uncaught KeyError instead of the documented fallback or the clean "argument required" exit. Validate the marker against libraries before returning it.
64e33e7 to
021b5c8
Compare
021b5c8 to
2540ad2
Compare
Render the summary and progress as one live display so wrapping, logos, and the bar follow the available terminal width.
The loading screen added helpers above the capture flags in entrypoints/common.py, shifting the documented line range into resolve_play_task_name's docstring. Sphinx then failed to lex the unterminated string as Python and fell back to relaxed highlighting. Point the range at the four --capture_env_sensors arguments the surrounding prose describes.
Description
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there