Report the resolved backends in the run startup summary - #7026
Conversation
The train and play startup summary was printed before launch_simulation scanned the config, so a run reached through an automatic selector was reported by the family it asked for rather than the backend it resolves to. A headless `physics=ovphysx renderer=rtx` run printed `Renderer rtx` and then launched OVRTX. Scan the config in show_run_summary so every row names the backend that will run, and move pre_launch_video_config above the summary in the train entrypoints so the scan sees the recording visualizer a `--video` run injects. Requesting the livestream Kit visualizer moves into scan() so a direct caller reaches the same launch decision as launch_simulation.
Greptile SummaryThe PR makes startup summaries resolve automatic physics and renderer selectors before reporting the selected backends.
Confidence Score: 5/5The PR appears safe to merge with no actionable correctness, security, or quality issues identified. The summary and launch paths scan the same configuration without intervening mutations, automatic backend resolution is idempotent, and livestream and video visualizer handling remains consistent across the affected entrypoints. Important Files Changed
Sequence DiagramsequenceDiagram
participant Entry as Train/Play entrypoint
participant Video as pre_launch_video_config
participant Summary as show_run_summary
participant Scan as scan
participant Launch as launch_simulation
Entry->>Video: Inject video visualizer when required
Entry->>Summary: Render startup details
Summary->>Scan: Resolve automatic backends
Scan-->>Summary: Mutated concrete configuration
Entry->>Launch: Launch using same configuration
Launch->>Scan: Re-scan resolved configuration
Scan-->>Launch: Same runtime decision
Reviews (1): Last reviewed commit: "Report the resolved backends in the run ..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The startup summary now resolves automatic physics and renderer selectors through the shared scan API, with video configuration applied first and tests covering repeated scans. One public API documentation issue remains: scan now mutates both the configuration and launcher arguments, but documents only the configuration mutation.
- Design and architecture: Backend resolution remains centralized in the launcher module, and the reordered training entrypoints satisfy the documented requirement to apply video-related configuration before summary resolution. The repeated-scan tests cover the summary-to-launch path.
- API:
scanremains signature-compatible, but its observable side effects have expanded: livestream handling writes the Kit visualizer into caller-ownedlauncher_args. This new behavior should be stated in the public docstring, especially because downstream summary rendering reads the mutated arguments. - Implementation: The moved
_ensure_livestream_kit_visualizercall directly establishes the undocumented launcher-argument mutation, while the existing test confirms the write-back throughlauncher_args.visualizer. Add a concise docstring note describing this side effect.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| same launch decision. | ||
| """ | ||
| # Livestreaming implies a Kit visualizer; make that visible to auto RTX resolution. | ||
| _ensure_livestream_kit_visualizer(launcher_args) |
There was a problem hiding this comment.
🔵 Suggestion · Api — scan mutates launcher_args without documenting it
scan is public and now forces the Kit visualizer into the caller-owned launcher_args for livestream runs, a side effect previously confined to launch_simulation. The docstring updated in this same hunk only states that cfg is mutated in place, yet show_run_summary reads args_cli afterwards for the visualizer row, so a direct caller can observe its namespace changed silently. Add one docstring line stating that livestream runs get the Kit visualizer written back into launcher_args.
| same launch decision. | ||
| """ | ||
| # Livestreaming implies a Kit visualizer; make that visible to auto RTX resolution. | ||
| _ensure_livestream_kit_visualizer(launcher_args) |
There was a problem hiding this comment.
Livestreaming requires a Kit viewport, but nothing in the config says so. scan needs to be self sufficient so that when it is called from the show_summary this information is already known
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "selectors, expected_physics, expected_renderer, expected_presets", |
There was a problem hiding this comment.
marking which selectors were used should be more descriptive for this test
| config pinned it. | ||
| """ | ||
| if requested != resolved: | ||
| return resolved |
There was a problem hiding this comment.
if the name was resolved by auto selection output the name that was resolved
StafaH
left a comment
There was a problem hiding this comment.
Thanks Matthew for fixing this.
Can we remove the renderer/physics from preset row, I'd prefer not to repeat there.
Either we show the resolved name, or we show the actual name and resolved in brackets (similar to how default is handled).
I think the second option suits us.
Example:
renderer: rtx (ovrtx)
because it follows the default logic which currently does:
renderer: default (newton_renderer)
done, thanks this makes sense @StafaH |
Signed-off-by: Kelly Guo <kellyg@nvidia.com>
Description
The train and play startup summary was printed before
launch_simulationscanned the config, so arun reached through an automatic selector was reported by the family it asked for rather than the
backend it resolves to.
physics=physxresolves toPhysxAutoCfgandrenderer=rtxto anauto_rtxplaceholder; both are resolved insidescan(), using the same "does this run need Kit"decision, well after the panel was drawn.
reported
Renderer rtxand then launched OVRTX.show_run_summarynow scans the config before drawing the panel, so every row names the backendthat will run alongside the choice the run stopped at, following the existing
default (<resolved>)form: a backend reached through a family the command line named is reportedas
<family> (<resolved>), and one the run named neither directly nor by family staysdefault (<resolved>).Two supporting changes:
pre_launch_video_configmoves above the summary in the four train entrypoints (the playentrypoints already had this order). It injects a headless Kit visualizer for
--videoruns, soscanning before it ran would resolve
auto_rtxto OVRTX and bake that in. As a side effect thesummary of a
--videorun no longer reportsVisualizer none (headless).launch_simulationintoscan(), so acaller that scans directly reaches the same launch decision the launcher does.
launch_simulationstill scans, and does not need a cached or precomputed result: resolving aplaceholder consumes it, so the second walk observes the same signals.
test_scanning_twice_reaches_the_same_launch_decisionpins that property.
Type of change
Screenshots
--task Isaac-Cartpole-Camera-Direct renderer=rtx physics=ovphysx, headless:The run launches OVRTX in both cases; only the report changes.
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