-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Report the resolved backends in the run startup summary #7026
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
eb36de5
c30b1bc
9ee3d3b
245fbdf
60b6230
438b183
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| Changed | ||
| ^^^^^^^ | ||
|
|
||
| * Changed :func:`~isaaclab.app.scan` to request the Kit visualizer for livestreamed runs itself, | ||
| instead of relying on :func:`~isaaclab.app.launch_simulation` to do so beforehand. Callers that | ||
| scan a config directly now resolve the ``rtx`` renderer preset the same way the launcher does. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -257,7 +257,14 @@ def scan(cfg, launcher_args: argparse.Namespace | dict | None = None) -> Scan: | |
| place). Automatic PhysX configurations and RTX | ||
| renderer placeholders (``renderer_type="auto_rtx"``) are also resolved | ||
| at this stage using the full *launcher_args* context. | ||
|
|
||
| The walk mutates *cfg* in place, and resolving a placeholder consumes it, so | ||
| a second walk of the same config observes the same signals and reaches the | ||
| same launch decision. | ||
| """ | ||
| # Livestreaming implies a Kit visualizer; make that visible to auto RTX resolution. | ||
| _ensure_livestream_kit_visualizer(launcher_args) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Livestreaming requires a Kit viewport, but nothing in the config says so. |
||
|
|
||
| physics_str = _get_arg(launcher_args, "physics", None) | ||
| physics_cfgs: list[PhysicsCfg] = [] | ||
| concrete_physics_cfgs: list[PhysicsCfg] = [] | ||
|
|
@@ -509,11 +516,8 @@ def launch_simulation( | |
| if launcher_args is None: | ||
| launcher_args = {} | ||
|
|
||
| # Livestreaming implies a Kit visualizer; make that visible to auto RTX | ||
| # resolution during the single scan. | ||
| _ensure_livestream_kit_visualizer(launcher_args) | ||
|
|
||
| # The single walk: collect every signal and apply the --physics override. | ||
| # The single walk: collect every signal, apply the --physics override, and | ||
| # resolve the automatic PhysX and RTX placeholders. | ||
| config_scan = scan(cfg, launcher_args) | ||
| effective_cfg = config_scan.effective_cfg | ||
| physics_cfg = config_scan.resolved_physics_cfg | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| Fixed | ||
| ^^^^^ | ||
|
|
||
| * Fixed the train and play startup summary reporting ``physics=physx`` and ``renderer=rtx`` by the | ||
| name the command line asked for. Both name a backend family that is resolved at launch, so the | ||
| summary now names the backend the run will actually use next to the family it was asked for -- | ||
| ``rtx (ovrtx)`` for a kitless ``physics=ovphysx renderer=rtx`` run and ``rtx (isaacsim_rtx)`` | ||
| when the run needs Kit. | ||
| * Fixed the summary of a ``--video`` training run reporting no visualizer, which happened because | ||
| the recording visualizer was injected after the summary was printed. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,7 @@ | |
| import torch | ||
| from PIL import Image | ||
|
|
||
| from isaaclab.app import AppLauncher, LoadingScreen | ||
| from isaaclab.app import AppLauncher, LoadingScreen, scan | ||
| from isaaclab.envs import DirectMARLEnvCfg, ManagerBasedRLEnvCfg | ||
| from isaaclab.renderers.renderer_cfg import RendererCfg | ||
| from isaaclab.utils.dict import print_dict | ||
|
|
@@ -545,9 +545,15 @@ def show_run_summary( | |
| ) -> None: | ||
| """Print a summary of the backends and scale a run is about to use. | ||
|
|
||
| Values the run did not pick itself are shown as ``default (<resolved>)``, so | ||
| it is clear which backends came from the command line and which are the | ||
| task's own defaults. | ||
| Every row names the backend that will run, alongside the choice the run stopped at. | ||
| A backend reached through a family the command line named -- ``physics=physx`` and | ||
| ``renderer=rtx`` name a family that launch resolves -- is shown as | ||
| ``<family> (<resolved>)``, and a backend the run named neither directly nor by | ||
| family is shown as ``default (<resolved>)``. | ||
|
|
||
| Resolving those selectors mutates *env_cfg* in place, exactly as the following | ||
| :func:`~isaaclab.app.launch_simulation` call would; call this after every other | ||
| pre-launch config change, in particular :func:`pre_launch_video_config`. | ||
|
|
||
| Args: | ||
| screen: Loading screen that owns the console. | ||
|
|
@@ -559,27 +565,54 @@ def show_run_summary( | |
| selected = _selected_preset_names() | ||
| device = getattr(args_cli, "device", None) or env_cfg.sim.device | ||
| num_envs = getattr(args_cli, "num_envs", None) or env_cfg.scene.num_envs | ||
|
|
||
| # Names read before the scan resolves the automatic selectors, so a row can report | ||
| # the family the run asked for next to the backend that family resolved to | ||
| requested_physics = _physics_name(env_cfg.sim.physics) | ||
| requested_renderer = _renderer_name(env_cfg) | ||
| scan(env_cfg, args_cli) | ||
| physics = _physics_name(env_cfg.sim.physics) | ||
| renderer = _renderer_name(env_cfg) | ||
|
|
||
| screen.summary( | ||
| f"Isaac Lab · {action}", | ||
| { | ||
| "Task": args_cli.task, | ||
| "Workflow": _workflow_name(env_cfg), | ||
| "RL library": library, | ||
| "Physics": _label(physics, selected=selected), | ||
| "Renderer": "n/a (no camera sensors)" if renderer is None else _label(renderer, selected=selected), | ||
| "Presets": _additional_preset_names({physics, renderer}), | ||
| "Physics": _label(requested_physics, physics, selected=selected), | ||
| "Renderer": ( | ||
| "n/a (no camera sensors)" | ||
| if renderer is None | ||
| else _label(requested_renderer or renderer, renderer, selected=selected) | ||
| ), | ||
| "Presets": _additional_preset_names({requested_physics, physics, requested_renderer, renderer}), | ||
| "Visualizer": _visualizer_name(args_cli, env_cfg), | ||
| "Device": str(device), | ||
| "Environments": str(num_envs), | ||
| }, | ||
| ) | ||
|
|
||
|
|
||
| def _label(name: str, *, selected: set[str] = frozenset()) -> str: | ||
| """Mark *name* as a default unless the run asked for it by name.""" | ||
| return name if name in selected else f"default ({name})" | ||
| def _label(requested: str, resolved: str, *, selected: set[str] = frozenset()) -> str: | ||
| """Name the backend a row reports, and where the run stopped choosing it. | ||
|
|
||
| Args: | ||
| requested: Preset name the config carried before launch resolved its automatic | ||
| selectors, which names a backend family when it differs from *resolved*. | ||
| resolved: Preset name of the backend that will run. | ||
| selected: Preset names the command line asked for. | ||
|
|
||
| Returns: | ||
| The resolved name when the run asked for that backend, ``<family> (<resolved>)`` | ||
| when it asked for the family the backend was picked from, and | ||
| ``default (<resolved>)`` when it asked for neither. | ||
| """ | ||
| if resolved in selected: | ||
| return resolved | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the name was resolved by auto selection output the name that was resolved |
||
| if requested in selected: | ||
| return f"{requested} ({resolved})" | ||
| return f"default ({resolved})" | ||
|
|
||
|
|
||
| def _selected_preset_names() -> set[str]: | ||
|
|
@@ -599,14 +632,16 @@ def _selected_preset_names() -> set[str]: | |
|
|
||
|
|
||
| def _additional_preset_names(shown: Container[str | None]) -> str: | ||
| """Return the presets the command line asked for beyond those with a row of their own. | ||
| """Return the presets the command line asked for that no other row names. | ||
|
|
||
| Domain presets such as ``presets=cube`` do not surface anywhere else in the | ||
| summary, so they are listed here; the physics and renderer presets are left | ||
| out because their own rows already name them. | ||
| summary, so they are listed here. A preset a row already reports is left out, | ||
| whether it names the backend that will run or the family the row resolved it | ||
| from -- ``renderer=rtx`` is reported by an ``rtx (ovrtx)`` renderer row. | ||
|
|
||
| Args: | ||
| shown: Preset names already reported by the physics and renderer rows. | ||
| shown: Preset names reported by the physics and renderer rows, including | ||
| the families those rows resolved from. | ||
|
|
||
| Returns: | ||
| The remaining preset names in command-line order, comma separated, or | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -356,3 +356,55 @@ def test_additional_preset_names_lists_presets_without_a_row_of_their_own( | |
| """The presets row names the chosen presets that physics and renderer do not already report.""" | ||
| monkeypatch.setattr(_rl_common.sys, "argv", ["train.py"] + argv) | ||
| assert _rl_common._additional_preset_names({"newton_mjwarp", "rtx"}) == expected | ||
|
|
||
|
|
||
| class _RecordingScreen: | ||
| """Loading screen stand-in that keeps the summary fields instead of drawing them.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| self.fields: dict[str, str] = {} | ||
|
|
||
| def summary(self, title: str, fields: dict[str, str]) -> None: | ||
| self.fields = fields | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "selectors, expected_physics, expected_renderer, expected_presets", | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. marking which selectors were used should be more descriptive for this test |
||
| [ | ||
| (["physics=ovphysx", "renderer=rtx"], "ovphysx", "rtx (ovrtx)", "none"), | ||
| (["physics=isaacsim_physx", "renderer=rtx"], "isaacsim_physx", "rtx (isaacsim_rtx)", "none"), | ||
| # ``physx`` reaches the physics backend the same way ``rtx`` reaches the renderer | ||
| (["physics=physx", "renderer=rtx"], "physx (ovphysx)", "rtx (ovrtx)", "none"), | ||
| # a run that names no backend reports the ones the task pinned as defaults | ||
| ([], "default (isaacsim_physx)", "default (isaacsim_rtx)", "none"), | ||
| # a domain preset has no row of its own | ||
| (["physics=physx", "presets=depth"], "physx (isaacsim_physx)", "default (isaacsim_rtx)", "depth"), | ||
| ], | ||
| ) | ||
| def test_run_summary_reports_the_backends_the_run_resolves_to( | ||
| selectors: list[str], | ||
| expected_physics: str, | ||
| expected_renderer: str, | ||
| expected_presets: str, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| """``physics=physx`` and ``renderer=rtx`` name a family; the summary names what will run. | ||
|
|
||
| A row reached through such a selector names the resolved backend on its own, since | ||
| the run never asked for a different one; the selector is listed under presets. Only | ||
| a backend the task pinned and the run never named is marked a default. | ||
| """ | ||
| import isaaclab_tasks # noqa: F401 | ||
| from isaaclab_tasks.utils import resolve_task_config | ||
|
|
||
| task = "Isaac-Cartpole-Camera-Direct" | ||
| monkeypatch.setattr(_rl_common.sys, "argv", ["train.py", *selectors]) | ||
| env_cfg, _ = resolve_task_config(task, "rsl_rl_cfg_entry_point") | ||
| screen = _RecordingScreen() | ||
| args_cli = argparse.Namespace(task=task, device=None, num_envs=None, visualizer=None) | ||
|
|
||
| _rl_common.show_run_summary(screen, args_cli, env_cfg, library="rsl_rl", action="train") | ||
|
|
||
| assert screen.fields["Physics"] == expected_physics | ||
| assert screen.fields["Renderer"] == expected_renderer | ||
| assert screen.fields["Presets"] == expected_presets | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Internal: added a test covering that scanning a config twice reaches the same launch decision, which | ||
| the run summary relies on now that it resolves the launch placeholders before ``launch_simulation`` | ||
| scans them again. No source change in this package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 Suggestion · Api — scan mutates launcher_args without documenting it
scanis public and now forces the Kit visualizer into the caller-owned launcher_args for livestream runs, a side effect previously confined tolaunch_simulation. The docstring updated in this same hunk only states that cfg is mutated in place, yetshow_run_summaryreadsargs_cliafterwards 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.