From 724b24a50ffb2786a281ff0aff338a874d72ce9d Mon Sep 17 00:00:00 2001 From: David Tingdahl Date: Wed, 9 Sep 2026 15:03:46 +0200 Subject: [PATCH 01/11] Add num_rerenders_on_reset default and generic CallbackRecorderTerm Add num_rerenders_on_reset field to IsaacLabArenaManagerBasedRLEnvCfg with default value of 5 to prevent stale camera frames after reset. Implement CallbackRecorderTerm, a generic IsaacLab RecorderTerm subclass that forwards lifecycle callbacks (post-step, pre-reset, close) to externally-supplied handler functions. This provides correctly-timed hooks without requiring a new RecorderTerm subclass for each use case. Co-Authored-By: Claude Haiku 4.5 --- .../isaaclab_arena_manager_based_env_cfg.py | 6 ++ .../recording/callback_recorder_term.py | 79 +++++++++++++++++++ .../tests/test_callback_recorder_term.py | 76 ++++++++++++++++++ ...st_isaaclab_arena_manager_based_env_cfg.py | 14 ++++ 4 files changed, 175 insertions(+) create mode 100644 isaaclab_arena/recording/callback_recorder_term.py create mode 100644 isaaclab_arena/tests/test_callback_recorder_term.py create mode 100644 isaaclab_arena/tests/test_isaaclab_arena_manager_based_env_cfg.py diff --git a/isaaclab_arena/environments/isaaclab_arena_manager_based_env_cfg.py b/isaaclab_arena/environments/isaaclab_arena_manager_based_env_cfg.py index ce619a7320..c89cef3728 100644 --- a/isaaclab_arena/environments/isaaclab_arena_manager_based_env_cfg.py +++ b/isaaclab_arena/environments/isaaclab_arena_manager_based_env_cfg.py @@ -86,6 +86,12 @@ class IsaacLabArenaManagerBasedRLEnvCfg(ManagerBasedRLEnvCfg): decimation: int = 8 wait_for_textures: bool = False + # Force extra RTX sensor refreshes after every reset. IsaacLab's own default (0) + # leaves camera buffers stale on the first frame of every episode after the + # first, so the previous episode's final rendered frame leaks in (corrupting + # RGB/depth/flow during datagen). See IsaacLab-Arena #339. + num_rerenders_on_reset: int = 5 + def apply_arena_global_settings() -> None: """Apply Arena's process-global RTX and physics settings before environment construction.""" diff --git a/isaaclab_arena/recording/callback_recorder_term.py b/isaaclab_arena/recording/callback_recorder_term.py new file mode 100644 index 0000000000..785c178878 --- /dev/null +++ b/isaaclab_arena/recording/callback_recorder_term.py @@ -0,0 +1,79 @@ +# Copyright (c) 2026, The Isaac Lab Arena Project Developers (https://github.com/isaac-sim/IsaacLab-Arena/blob/main/CONTRIBUTORS.md). +# All rights reserved. +# +# SPDX-License-Identifier: Apache-2.0 +"""Forward IsaacLab RecorderManager callbacks to externally-supplied functions. + +CallbackRecorderTerm carries no domain knowledge of its own. It exists so callers can +get correctly-timed per-step and pre-reset hooks (the same timing IsaacLab's own +RecorderManager already uses for HDF5 demo/mimic export) without writing a new +RecorderTerm subclass for each use case. +""" + +from __future__ import annotations + +from collections.abc import Callable, Sequence +from dataclasses import dataclass +from typing import TYPE_CHECKING + +from isaaclab.managers import RecorderTerm, RecorderTermCfg +from isaaclab.utils.configclass import configclass + +if TYPE_CHECKING: + from isaaclab.envs import ManagerBasedEnv + + +@dataclass(frozen=True) +class CallbackRecorderTermHandlers: + """Plain lifecycle callbacks a CallbackRecorderTerm forwards to. All optional.""" + + on_post_step: Callable[[ManagerBasedEnv], None] | None = None + """Called once per step, for every env, before any reset that step.""" + + on_pre_reset: Callable[[ManagerBasedEnv, Sequence[int]], None] | None = None + """Called once per resetting batch, before the reset is effective.""" + + on_close: Callable[[str | None], None] | None = None + """Called when the owning RecorderManager is closed.""" + + +class CallbackRecorderTerm(RecorderTerm): + """Forward RecorderManager's per-step and pre-reset callbacks to configured handlers. + + Writes no data of its own (always returns (None, None)) -- it exists purely to get + correctly-timed callbacks into arbitrary code, not to contribute to the exported + HDF5 dataset. + """ + + def __init__(self, cfg: CallbackRecorderTermCfg, env: ManagerBasedEnv) -> None: + super().__init__(cfg, env) + self._handlers = cfg.build_handlers(env) + + def record_post_step(self): + if self._handlers.on_post_step is not None: + self._handlers.on_post_step(self._env) + return None, None + + def record_pre_reset(self, env_ids): + if self._handlers.on_pre_reset is not None: + self._handlers.on_pre_reset(self._env, env_ids) + return None, None + + def close(self, file_path): + if self._handlers.on_close is not None: + self._handlers.on_close(file_path) + + +@configclass +class CallbackRecorderTermCfg(RecorderTermCfg): + """Configuration for a CallbackRecorderTerm.""" + + class_type: type[RecorderTerm] = CallbackRecorderTerm + + build_handlers: Callable[[ManagerBasedEnv], CallbackRecorderTermHandlers] = None + """Called once, at term construction time, with the live env, to obtain the handlers. + + Deferred like this (rather than passing already-built handlers) because RecorderTerm + construction happens during env.load_managers() -- callers that need to build something + from the env itself (e.g. a datagen collector) cannot do so before this point. + """ diff --git a/isaaclab_arena/tests/test_callback_recorder_term.py b/isaaclab_arena/tests/test_callback_recorder_term.py new file mode 100644 index 0000000000..d772ecdfe8 --- /dev/null +++ b/isaaclab_arena/tests/test_callback_recorder_term.py @@ -0,0 +1,76 @@ +# Copyright (c) 2025-2026, The Isaac Lab Arena Project Developers (https://github.com/isaac-sim/IsaacLab-Arena/blob/main/CONTRIBUTORS.md). +# All rights reserved. +# +# SPDX-License-Identifier: Apache-2.0 +"""Tests for CallbackRecorderTerm (no Isaac Sim required -- ManagerTermBase.__init__ only +stores cfg/env, so a plain object stands in for the env).""" + +from __future__ import annotations + +from isaaclab_arena.recording.callback_recorder_term import ( + CallbackRecorderTerm, + CallbackRecorderTermCfg, + CallbackRecorderTermHandlers, +) + + +class _FakeEnv: + """Minimal stand-in for a ManagerBasedEnv; CallbackRecorderTerm never inspects it.""" + + +def test_build_handlers_called_once_at_construction_with_env(): + calls: list[object] = [] + + def build_handlers(env): + calls.append(env) + return CallbackRecorderTermHandlers() + + env = _FakeEnv() + CallbackRecorderTerm(CallbackRecorderTermCfg(build_handlers=build_handlers), env) + + assert calls == [env] + + +def test_record_post_step_forwards_to_on_post_step_and_returns_none_none(): + calls: list[object] = [] + handlers = CallbackRecorderTermHandlers(on_post_step=calls.append) + env = _FakeEnv() + term = CallbackRecorderTerm(CallbackRecorderTermCfg(build_handlers=lambda _env: handlers), env) + + result = term.record_post_step() + + assert calls == [env] + assert result == (None, None) + + +def test_record_pre_reset_forwards_env_and_env_ids(): + calls: list[tuple[object, object]] = [] + handlers = CallbackRecorderTermHandlers(on_pre_reset=lambda env, env_ids: calls.append((env, env_ids))) + env = _FakeEnv() + term = CallbackRecorderTerm(CallbackRecorderTermCfg(build_handlers=lambda _env: handlers), env) + + result = term.record_pre_reset([0, 2]) + + assert calls == [(env, [0, 2])] + assert result == (None, None) + + +def test_close_forwards_to_on_close(): + calls: list[str | None] = [] + handlers = CallbackRecorderTermHandlers(on_close=calls.append) + env = _FakeEnv() + term = CallbackRecorderTerm(CallbackRecorderTermCfg(build_handlers=lambda _env: handlers), env) + + term.close("/tmp/dataset.hdf5") + + assert calls == ["/tmp/dataset.hdf5"] + + +def test_none_handlers_are_no_ops(): + handlers = CallbackRecorderTermHandlers() + env = _FakeEnv() + term = CallbackRecorderTerm(CallbackRecorderTermCfg(build_handlers=lambda _env: handlers), env) + + assert term.record_post_step() == (None, None) + assert term.record_pre_reset([0]) == (None, None) + term.close(None) # must not raise diff --git a/isaaclab_arena/tests/test_isaaclab_arena_manager_based_env_cfg.py b/isaaclab_arena/tests/test_isaaclab_arena_manager_based_env_cfg.py new file mode 100644 index 0000000000..f796e0e961 --- /dev/null +++ b/isaaclab_arena/tests/test_isaaclab_arena_manager_based_env_cfg.py @@ -0,0 +1,14 @@ +# Copyright (c) 2025-2026, The Isaac Lab Arena Project Developers (https://github.com/isaac-sim/IsaacLab-Arena/blob/main/CONTRIBUTORS.md). +# All rights reserved. +# +# SPDX-License-Identifier: Apache-2.0 +"""Tests for IsaacLabArenaManagerBasedRLEnvCfg defaults (no Isaac Sim required).""" + +from isaaclab_arena.environments.isaaclab_arena_manager_based_env_cfg import ( + IsaacLabArenaManagerBasedRLEnvCfg, +) + + +def test_default_reruns_after_reset_to_flush_stale_camera_frames(): + """A positive default avoids RTX sensors reading the previous episode's last frame.""" + assert IsaacLabArenaManagerBasedRLEnvCfg().num_rerenders_on_reset == 5 From d1e226df071009c78f328436423cf736eecce588 Mon Sep 17 00:00:00 2001 From: David Tingdahl Date: Wed, 9 Sep 2026 15:25:05 +0200 Subject: [PATCH 02/11] Fix Task 1 tests to run inside a SimulationApp context isaaclab.managers/isaaclab.envs need a running SimulationApp (omni.timeline is only importable once Kit has booted), so these tests were not actually sim-free as originally written. Rewrite both to use the codebase's established run_function_with_persistent_simulation_app pattern (see isaaclab_arena/tests/test_task_registry.py). --- .../tests/test_callback_recorder_term.py | 95 ++++++++++++++++--- ...st_isaaclab_arena_manager_based_env_cfg.py | 28 +++++- 2 files changed, 106 insertions(+), 17 deletions(-) diff --git a/isaaclab_arena/tests/test_callback_recorder_term.py b/isaaclab_arena/tests/test_callback_recorder_term.py index d772ecdfe8..cf05685ce7 100644 --- a/isaaclab_arena/tests/test_callback_recorder_term.py +++ b/isaaclab_arena/tests/test_callback_recorder_term.py @@ -2,23 +2,30 @@ # All rights reserved. # # SPDX-License-Identifier: Apache-2.0 -"""Tests for CallbackRecorderTerm (no Isaac Sim required -- ManagerTermBase.__init__ only -stores cfg/env, so a plain object stands in for the env).""" +"""Tests for CallbackRecorderTerm. + +Importing isaaclab_arena.recording.callback_recorder_term pulls in isaaclab.managers +via RecorderTerm, which needs a running SimulationApp (omni.timeline is only importable +once Kit has booted) -- see isaaclab_arena/tests/test_task_registry.py for the established +_test_/test_ + run_function_with_persistent_simulation_app pattern this mirrors. +""" from __future__ import annotations -from isaaclab_arena.recording.callback_recorder_term import ( - CallbackRecorderTerm, - CallbackRecorderTermCfg, - CallbackRecorderTermHandlers, -) +from isaaclab_arena.tests.utils.persistent_simulation_app import run_function_with_persistent_simulation_app class _FakeEnv: """Minimal stand-in for a ManagerBasedEnv; CallbackRecorderTerm never inspects it.""" -def test_build_handlers_called_once_at_construction_with_env(): +def _test_build_handlers_called_once_at_construction_with_env(simulation_app): + from isaaclab_arena.recording.callback_recorder_term import ( + CallbackRecorderTerm, + CallbackRecorderTermCfg, + CallbackRecorderTermHandlers, + ) + calls: list[object] = [] def build_handlers(env): @@ -29,9 +36,23 @@ def build_handlers(env): CallbackRecorderTerm(CallbackRecorderTermCfg(build_handlers=build_handlers), env) assert calls == [env] + return True -def test_record_post_step_forwards_to_on_post_step_and_returns_none_none(): +def test_build_handlers_called_once_at_construction_with_env(): + result = run_function_with_persistent_simulation_app( + _test_build_handlers_called_once_at_construction_with_env + ) + assert result + + +def _test_record_post_step_forwards_to_on_post_step_and_returns_none_none(simulation_app): + from isaaclab_arena.recording.callback_recorder_term import ( + CallbackRecorderTerm, + CallbackRecorderTermCfg, + CallbackRecorderTermHandlers, + ) + calls: list[object] = [] handlers = CallbackRecorderTermHandlers(on_post_step=calls.append) env = _FakeEnv() @@ -41,9 +62,23 @@ def test_record_post_step_forwards_to_on_post_step_and_returns_none_none(): assert calls == [env] assert result == (None, None) + return True -def test_record_pre_reset_forwards_env_and_env_ids(): +def test_record_post_step_forwards_to_on_post_step_and_returns_none_none(): + result = run_function_with_persistent_simulation_app( + _test_record_post_step_forwards_to_on_post_step_and_returns_none_none + ) + assert result + + +def _test_record_pre_reset_forwards_env_and_env_ids(simulation_app): + from isaaclab_arena.recording.callback_recorder_term import ( + CallbackRecorderTerm, + CallbackRecorderTermCfg, + CallbackRecorderTermHandlers, + ) + calls: list[tuple[object, object]] = [] handlers = CallbackRecorderTermHandlers(on_pre_reset=lambda env, env_ids: calls.append((env, env_ids))) env = _FakeEnv() @@ -53,9 +88,23 @@ def test_record_pre_reset_forwards_env_and_env_ids(): assert calls == [(env, [0, 2])] assert result == (None, None) + return True -def test_close_forwards_to_on_close(): +def test_record_pre_reset_forwards_env_and_env_ids(): + result = run_function_with_persistent_simulation_app( + _test_record_pre_reset_forwards_env_and_env_ids + ) + assert result + + +def _test_close_forwards_to_on_close(simulation_app): + from isaaclab_arena.recording.callback_recorder_term import ( + CallbackRecorderTerm, + CallbackRecorderTermCfg, + CallbackRecorderTermHandlers, + ) + calls: list[str | None] = [] handlers = CallbackRecorderTermHandlers(on_close=calls.append) env = _FakeEnv() @@ -64,9 +113,23 @@ def test_close_forwards_to_on_close(): term.close("/tmp/dataset.hdf5") assert calls == ["/tmp/dataset.hdf5"] + return True -def test_none_handlers_are_no_ops(): +def test_close_forwards_to_on_close(): + result = run_function_with_persistent_simulation_app( + _test_close_forwards_to_on_close + ) + assert result + + +def _test_none_handlers_are_no_ops(simulation_app): + from isaaclab_arena.recording.callback_recorder_term import ( + CallbackRecorderTerm, + CallbackRecorderTermCfg, + CallbackRecorderTermHandlers, + ) + handlers = CallbackRecorderTermHandlers() env = _FakeEnv() term = CallbackRecorderTerm(CallbackRecorderTermCfg(build_handlers=lambda _env: handlers), env) @@ -74,3 +137,11 @@ def test_none_handlers_are_no_ops(): assert term.record_post_step() == (None, None) assert term.record_pre_reset([0]) == (None, None) term.close(None) # must not raise + return True + + +def test_none_handlers_are_no_ops(): + result = run_function_with_persistent_simulation_app( + _test_none_handlers_are_no_ops + ) + assert result diff --git a/isaaclab_arena/tests/test_isaaclab_arena_manager_based_env_cfg.py b/isaaclab_arena/tests/test_isaaclab_arena_manager_based_env_cfg.py index f796e0e961..21185188dd 100644 --- a/isaaclab_arena/tests/test_isaaclab_arena_manager_based_env_cfg.py +++ b/isaaclab_arena/tests/test_isaaclab_arena_manager_based_env_cfg.py @@ -2,13 +2,31 @@ # All rights reserved. # # SPDX-License-Identifier: Apache-2.0 -"""Tests for IsaacLabArenaManagerBasedRLEnvCfg defaults (no Isaac Sim required).""" +"""Tests for IsaacLabArenaManagerBasedRLEnvCfg defaults. -from isaaclab_arena.environments.isaaclab_arena_manager_based_env_cfg import ( - IsaacLabArenaManagerBasedRLEnvCfg, -) +Importing isaaclab_arena_manager_based_env_cfg pulls in isaaclab.envs -> isaaclab.managers, +which needs a running SimulationApp (omni.timeline is only importable once Kit has +booted) -- see isaaclab_arena/tests/test_task_registry.py for the established +_test_/test_ + run_function_with_persistent_simulation_app pattern this mirrors. +""" +from __future__ import annotations -def test_default_reruns_after_reset_to_flush_stale_camera_frames(): +from isaaclab_arena.tests.utils.persistent_simulation_app import run_function_with_persistent_simulation_app + + +def _test_default_reruns_after_reset_to_flush_stale_camera_frames(simulation_app): """A positive default avoids RTX sensors reading the previous episode's last frame.""" + from isaaclab_arena.environments.isaaclab_arena_manager_based_env_cfg import ( + IsaacLabArenaManagerBasedRLEnvCfg, + ) + assert IsaacLabArenaManagerBasedRLEnvCfg().num_rerenders_on_reset == 5 + return True + + +def test_default_reruns_after_reset_to_flush_stale_camera_frames(): + result = run_function_with_persistent_simulation_app( + _test_default_reruns_after_reset_to_flush_stale_camera_frames + ) + assert result From 975490b7b730b4231b0ec2bc952f160bff137a77 Mon Sep 17 00:00:00 2001 From: David Tingdahl Date: Thu, 10 Sep 2026 08:51:49 +0200 Subject: [PATCH 03/11] Add classify_outcome, reading termination state directly Co-Authored-By: Claude Haiku 4.5 --- isaaclab_arena/evaluation/episode_outcome.py | 33 ++++++++++++ isaaclab_arena/tests/test_episode_outcome.py | 54 ++++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 isaaclab_arena/evaluation/episode_outcome.py create mode 100644 isaaclab_arena/tests/test_episode_outcome.py diff --git a/isaaclab_arena/evaluation/episode_outcome.py b/isaaclab_arena/evaluation/episode_outcome.py new file mode 100644 index 0000000000..6fd0b08e01 --- /dev/null +++ b/isaaclab_arena/evaluation/episode_outcome.py @@ -0,0 +1,33 @@ +# Copyright (c) 2025-2026, The Isaac Lab Arena Project Developers (https://github.com/isaac-sim/IsaacLab-Arena/blob/main/CONTRIBUTORS.md). +# All rights reserved. +# +# SPDX-License-Identifier: Apache-2.0 +"""Classify how a datagen episode ended, from the env's termination state.""" + +from __future__ import annotations + +from typing import Any, Literal + +EpisodeOutcome = Literal["success", "failure", "timeout"] + + +def classify_outcome(env: Any, env_id: int) -> EpisodeOutcome: + """Classify env_id's just-finished episode from its active termination terms. + + Mirrors record_core_episode_results in isaaclab_arena/recording/common_terms.py, + which reads the same "success" termination term for its own per-episode record. + + Args: + env: IsaacLab environment instance (must have a termination_manager). + env_id: Index of the env whose episode just ended. + + Returns: + "success" if the success termination term fired, "timeout" if the time_out + term fired, otherwise "failure". + """ + active_terms = env.termination_manager.active_terms + if "success" in active_terms and bool(env.termination_manager.get_term("success")[env_id]): + return "success" + if "time_out" in active_terms and bool(env.termination_manager.get_term("time_out")[env_id]): + return "timeout" + return "failure" diff --git a/isaaclab_arena/tests/test_episode_outcome.py b/isaaclab_arena/tests/test_episode_outcome.py new file mode 100644 index 0000000000..68dcc0ebaa --- /dev/null +++ b/isaaclab_arena/tests/test_episode_outcome.py @@ -0,0 +1,54 @@ +# Copyright (c) 2025-2026, The Isaac Lab Arena Project Developers (https://github.com/isaac-sim/IsaacLab-Arena/blob/main/CONTRIBUTORS.md). +# All rights reserved. +# +# SPDX-License-Identifier: Apache-2.0 +"""Unit tests for episode outcome classification (no Isaac Sim required).""" + +from __future__ import annotations + +import torch + +from isaaclab_arena.evaluation.episode_outcome import classify_outcome + + +class _FakeTerminationManager: + """Minimal stand-in for IsaacLab's TerminationManager.""" + + def __init__(self, terms: dict[str, torch.Tensor]) -> None: + self._terms = terms + + @property + def active_terms(self) -> list[str]: + return list(self._terms) + + def get_term(self, name: str) -> torch.Tensor: + return self._terms[name] + + +class _FakeEnv: + def __init__(self, termination_manager: _FakeTerminationManager) -> None: + self.termination_manager = termination_manager + + +def test_success_term_true_means_success(): + env = _FakeEnv(_FakeTerminationManager({"success": torch.tensor([True, False])})) + assert classify_outcome(env, 0) == "success" + + +def test_time_out_term_true_means_timeout(): + env = _FakeEnv( + _FakeTerminationManager({"success": torch.tensor([False]), "time_out": torch.tensor([True])}) + ) + assert classify_outcome(env, 0) == "timeout" + + +def test_neither_term_true_means_failure(): + env = _FakeEnv( + _FakeTerminationManager({"success": torch.tensor([False]), "time_out": torch.tensor([False])}) + ) + assert classify_outcome(env, 0) == "failure" + + +def test_no_success_or_time_out_terms_means_failure(): + env = _FakeEnv(_FakeTerminationManager({"object_dropped": torch.tensor([True])})) + assert classify_outcome(env, 0) == "failure" From 45cd94e0142c04361c54b4459bcecdf646b5c081 Mon Sep 17 00:00:00 2001 From: David Tingdahl Date: Thu, 10 Sep 2026 08:54:28 +0200 Subject: [PATCH 04/11] Add DatagenCollectorBase and its CallbackRecorderTerm handler adapter Signed-off-by: David Tingdahl --- .../evaluation/datagen_collector.py | 75 ++++++++++++ .../tests/test_datagen_collector.py | 110 ++++++++++++++++++ 2 files changed, 185 insertions(+) create mode 100644 isaaclab_arena/evaluation/datagen_collector.py create mode 100644 isaaclab_arena/tests/test_datagen_collector.py diff --git a/isaaclab_arena/evaluation/datagen_collector.py b/isaaclab_arena/evaluation/datagen_collector.py new file mode 100644 index 0000000000..b811f3ce04 --- /dev/null +++ b/isaaclab_arena/evaluation/datagen_collector.py @@ -0,0 +1,75 @@ +# Copyright (c) 2026, The Isaac Lab Arena Project Developers (https://github.com/isaac-sim/IsaacLab-Arena/blob/main/CONTRIBUTORS.md). +# All rights reserved. +# +# SPDX-License-Identifier: Apache-2.0 +"""Interface for datagen data collectors, and their CallbackRecorderTerm wiring. + +DatagenCollectorBase is implemented by the collecting package (e.g. nvblox_next's +datagen.arena_data_collector.DatagenCollector). build_datagen_callback_handlers adapts +one to the generic CallbackRecorderTermHandlers shape recording.callback_recorder_term +expects, keeping that module free of any datagen-specific knowledge. +""" + +from __future__ import annotations + +from abc import ABC, abstractmethod +from typing import Any + +from isaaclab_arena.evaluation.episode_outcome import EpisodeOutcome, classify_outcome +from isaaclab_arena.recording.callback_recorder_term import CallbackRecorderTermHandlers + + +class DatagenCollectorBase(ABC): + """Interface a datagen data collector implements, driven via CallbackRecorderTerm. + + Implementations record per-step data during a policy rollout. on_step fires after + every env.step (for every env, before any reset that step); on_episode_end fires + once per env_id right before that env's reset, while its terminal state is still + intact -- also the place to prepare that env's cameras for the next episode, since + the reset immediately following flushes any re-aimed poses via IsaacLab's + num_rerenders_on_reset. finalize/close run at rollout/job teardown. + """ + + @abstractmethod + def on_step(self, env: Any) -> None: + """Record one frame for every env. + + Reads env.obs_buf / env.action_manager / env.episode_length_buf directly (no + args beyond env: state lives on it). + """ + + @abstractmethod + def on_episode_end(self, env: Any, env_id: int, outcome: EpisodeOutcome = "timeout") -> None: + """Flush env_id's in-progress episode and prepare its cameras for the next one.""" + + @abstractmethod + def finalize(self, env: Any | None = None) -> None: + """Flush any in-progress episodes and stop recording. Idempotent.""" + + @abstractmethod + def close(self, env: Any | None = None) -> None: + """Finalize, then release resources such as spawned cameras. Idempotent.""" + + +def build_datagen_callback_handlers( + collector: DatagenCollectorBase, env: Any | None = None +) -> CallbackRecorderTermHandlers: + """Adapt a DatagenCollectorBase to the CallbackRecorderTerm handler shape. + + Args: + collector: The collector to drive. + env: If given, on_close calls collector.close(env) with this env instead of + None (CallbackRecorderTerm's on_close only ever receives a file_path, not + an env, so callers that need close(env) must bind env here at + build_handlers time -- see run_execution.py's usage). + """ + + def on_pre_reset(pre_reset_env: Any, env_ids) -> None: + for env_id in env_ids: + collector.on_episode_end(pre_reset_env, int(env_id), outcome=classify_outcome(pre_reset_env, int(env_id))) + + return CallbackRecorderTermHandlers( + on_post_step=collector.on_step, + on_pre_reset=on_pre_reset, + on_close=lambda _file_path: collector.close(env), + ) diff --git a/isaaclab_arena/tests/test_datagen_collector.py b/isaaclab_arena/tests/test_datagen_collector.py new file mode 100644 index 0000000000..b6c1d5fa36 --- /dev/null +++ b/isaaclab_arena/tests/test_datagen_collector.py @@ -0,0 +1,110 @@ +# Copyright (c) 2026, The Isaac Lab Arena Project Developers (https://github.com/isaac-sim/IsaacLab-Arena/blob/main/CONTRIBUTORS.md). +# All rights reserved. +# +# SPDX-License-Identifier: Apache-2.0 +"""Tests for DatagenCollectorBase and its CallbackRecorderTerm adapter. + +Importing isaaclab_arena.evaluation.datagen_collector needs a running SimulationApp +(it imports CallbackRecorderTermHandlers, which imports isaaclab.managers) -- see +isaaclab_arena/tests/test_task_registry.py for the established _test_/test_ + +run_function_with_persistent_simulation_app pattern this mirrors. +""" + +from __future__ import annotations + +from isaaclab_arena.tests.utils.persistent_simulation_app import run_function_with_persistent_simulation_app + + +def _build_fake_collector(): + """Build a fresh fake DatagenCollectorBase, deferring the import until Kit is running.""" + from isaaclab_arena.evaluation.datagen_collector import DatagenCollectorBase + + class _FakeCollector(DatagenCollectorBase): + def __init__(self) -> None: + self.steps: list[object] = [] + self.episode_ends: list[tuple[object, int, str]] = [] + self.finalized: list[object] = [] + self.closed: list[object] = [] + + def on_step(self, env): + self.steps.append(env) + + def on_episode_end(self, env, env_id, outcome="timeout"): + self.episode_ends.append((env, env_id, outcome)) + + def finalize(self, env=None): + self.finalized.append(env) + + def close(self, env=None): + self.closed.append(env) + + return _FakeCollector() + + +class _FakeTerminationManager: + active_terms: list[str] = [] + + def get_term(self, name): + raise AssertionError("no termination terms configured for this fake env") + + +class _FakeEnv: + def __init__(self) -> None: + self.termination_manager = _FakeTerminationManager() + + +def _test_on_post_step_forwards_to_collector_on_step(simulation_app): + from isaaclab_arena.evaluation.datagen_collector import build_datagen_callback_handlers + + collector = _build_fake_collector() + handlers = build_datagen_callback_handlers(collector) + env = _FakeEnv() + + handlers.on_post_step(env) + + assert collector.steps == [env] + return True + + +def test_on_post_step_forwards_to_collector_on_step(): + assert run_function_with_persistent_simulation_app(_test_on_post_step_forwards_to_collector_on_step) + + +def _test_on_pre_reset_calls_on_episode_end_per_env_id_with_classified_outcome(simulation_app): + from isaaclab_arena.evaluation.datagen_collector import build_datagen_callback_handlers + + collector = _build_fake_collector() + handlers = build_datagen_callback_handlers(collector) + env = _FakeEnv() # no active termination terms -> classify_outcome returns "failure" + + handlers.on_pre_reset(env, [0, 3]) + + assert collector.episode_ends == [(env, 0, "failure"), (env, 3, "failure")] + return True + + +def test_on_pre_reset_calls_on_episode_end_per_env_id_with_classified_outcome(): + assert run_function_with_persistent_simulation_app( + _test_on_pre_reset_calls_on_episode_end_per_env_id_with_classified_outcome + ) + + +def _test_on_close_forwards_env_via_closure_not_file_path(simulation_app): + """on_close's build_handlers closure captures env at call time and ignores file_path.""" + from isaaclab_arena.evaluation.datagen_collector import build_datagen_callback_handlers + + collector = _build_fake_collector() + env = _FakeEnv() + + def build_handlers(built_env): + return build_datagen_callback_handlers(collector, env=built_env) + + handlers = build_handlers(env) + handlers.on_close("/tmp/whatever.hdf5") + + assert collector.closed == [env] + return True + + +def test_on_close_forwards_env_via_closure_not_file_path(): + assert run_function_with_persistent_simulation_app(_test_on_close_forwards_env_via_closure_not_file_path) From c90baa4213f17be7ff84a44faba54ce6792835fb Mon Sep 17 00:00:00 2001 From: David Tingdahl Date: Thu, 10 Sep 2026 08:57:54 +0200 Subject: [PATCH 05/11] Thread a per-run datagen config dict through ArenaRunCfg and legacy JSON loading Signed-off-by: David Tingdahl --- isaaclab_arena/evaluation/arena_run.py | 6 ++++ .../evaluation/legacy_eval_config.py | 1 + isaaclab_arena/tests/test_arena_run.py | 9 +++++ .../tests/test_legacy_eval_config.py | 35 +++++++++++++++++++ 4 files changed, 51 insertions(+) diff --git a/isaaclab_arena/evaluation/arena_run.py b/isaaclab_arena/evaluation/arena_run.py index 3ffcbf5847..9dee714a69 100644 --- a/isaaclab_arena/evaluation/arena_run.py +++ b/isaaclab_arena/evaluation/arena_run.py @@ -65,6 +65,12 @@ class ArenaRunCfg: variations: dict[str, Any] = field(default_factory=dict) """Variation values applied when the environment is compiled.""" + datagen: dict[str, Any] | None = field(default=None) + """Per-run datagen collection config (output_dir, cameras, ...), or None to disable + collection for this run. Consumed by a datagen_collector_factory injected into + execute_experiment/experiment_runner.main -- Arena itself does not interpret its + contents beyond passing it to that factory.""" + def __post_init__(self) -> None: assert self.name, "run name must not be empty" assert self.num_rebuilds > 0, "num_rebuilds must be greater than zero" diff --git a/isaaclab_arena/evaluation/legacy_eval_config.py b/isaaclab_arena/evaluation/legacy_eval_config.py index e8d0356c74..046eaa2c2f 100644 --- a/isaaclab_arena/evaluation/legacy_eval_config.py +++ b/isaaclab_arena/evaluation/legacy_eval_config.py @@ -84,6 +84,7 @@ def _run_cfg_from_legacy_job( ), num_rebuilds=job_config.get("num_rebuilds", 1), variations=variations, + datagen=job_config.get("datagen"), ) diff --git a/isaaclab_arena/tests/test_arena_run.py b/isaaclab_arena/tests/test_arena_run.py index c27a525257..e187458240 100644 --- a/isaaclab_arena/tests/test_arena_run.py +++ b/isaaclab_arena/tests/test_arena_run.py @@ -66,3 +66,12 @@ def test_run_result_records_outcome_separately(): assert result.run_name == "test_run" assert result.metrics is None + + +def test_datagen_defaults_to_none(): + assert _run().datagen is None + + +def test_datagen_accepts_a_config_dict(): + datagen_cfg = {"output_dir": "/tmp/out", "width": 320} + assert _run(datagen=datagen_cfg).datagen == datagen_cfg diff --git a/isaaclab_arena/tests/test_legacy_eval_config.py b/isaaclab_arena/tests/test_legacy_eval_config.py index 1a12e0783e..8df4cccb40 100644 --- a/isaaclab_arena/tests/test_legacy_eval_config.py +++ b/isaaclab_arena/tests/test_legacy_eval_config.py @@ -174,6 +174,41 @@ def test_registered_environment_rejects_arguments_missing_from_its_typed_config( run_cfgs_from_legacy_eval_config(legacy_config, device="cpu") +def test_legacy_job_datagen_block_becomes_run_cfg_datagen(): + legacy_config = { + "jobs": [{ + "name": "job0", + "arena_env_args": { + "environment": "pick_and_place_maple_table", + }, + "policy_type": "zero_action", + "num_steps": 1, + "datagen": {"output_dir": "/tmp/out", "width": 320}, + }] + } + + (run,) = run_cfgs_from_legacy_eval_config(legacy_config, device="cpu") + + assert run.datagen == {"output_dir": "/tmp/out", "width": 320} + + +def test_legacy_job_without_datagen_block_leaves_it_none(): + legacy_config = { + "jobs": [{ + "name": "job0", + "arena_env_args": { + "environment": "pick_and_place_maple_table", + }, + "policy_type": "zero_action", + "num_steps": 1, + }] + } + + (run,) = run_cfgs_from_legacy_eval_config(legacy_config, device="cpu") + + assert run.datagen is None + + def test_legacy_runtime_status_is_not_a_run_configuration(): legacy_config = { "jobs": [{ From a38f1820e9cb73e69995b42ada0e7d6944866104 Mon Sep 17 00:00:00 2001 From: David Tingdahl Date: Thu, 10 Sep 2026 09:01:58 +0200 Subject: [PATCH 06/11] Wire an optional datagen_collector_factory through run_execution Signed-off-by: David Tingdahl --- isaaclab_arena/evaluation/run_execution.py | 61 +++++- isaaclab_arena/tests/test_run_execution.py | 239 +++------------------ 2 files changed, 90 insertions(+), 210 deletions(-) diff --git a/isaaclab_arena/evaluation/run_execution.py b/isaaclab_arena/evaluation/run_execution.py index eb8f4950ff..1d8c0ee010 100644 --- a/isaaclab_arena/evaluation/run_execution.py +++ b/isaaclab_arena/evaluation/run_execution.py @@ -14,9 +14,12 @@ from pathlib import Path from typing import TYPE_CHECKING +from isaaclab.managers.recorder_manager import RecorderManagerBaseCfg + from isaaclab_arena.assets.registries import EnvironmentRegistry, PolicyRegistry from isaaclab_arena.evaluation.arena_experiment import ArenaExperimentCfg from isaaclab_arena.evaluation.arena_run import ArenaRunCfg, ArenaRunResult, RunStatus +from isaaclab_arena.evaluation.datagen_collector import DatagenCollectorBase, build_datagen_callback_handlers from isaaclab_arena.evaluation.legacy_graph_environment_cli import ( LegacyGraphEnvironmentCfg, build_arena_builder_from_legacy_graph, @@ -24,6 +27,8 @@ from isaaclab_arena.evaluation.policy_runner import rollout_policy from isaaclab_arena.evaluation.resource_cleanup import close_run_resources from isaaclab_arena.metrics.aggregate_metrics import aggregate_metrics +from isaaclab_arena.recording.callback_recorder_term import CallbackRecorderTermCfg, CallbackRecorderTermHandlers +from isaaclab_arena.utils.configclass import combine_configclass_instances, make_configclass from isaaclab_arena.variations.variations_hydra import overrides_from_dict from isaaclab_arena.video.video_recording import VideoRecordingCfg, wrap_env_for_video @@ -41,6 +46,7 @@ def execute_experiment( record_viewport_video: bool = False, record_camera_video: bool = False, continue_on_error: bool = False, + datagen_collector_factory=None, ) -> list[ArenaRunResult]: """Execute an experiment's runs in order and return their results. @@ -50,6 +56,8 @@ def execute_experiment( record_viewport_video: Whether to record the viewport for each run. record_camera_video: Whether to record observation cameras for each run. continue_on_error: Whether to continue with later runs after one fails. + datagen_collector_factory: Optional Callable[[ArenaRunCfg, gym.Env], DatagenCollectorBase] + forwarded to build_and_run for each run. Returns: One result per attempted run, in execution order. @@ -67,6 +75,7 @@ def execute_experiment( record_camera_video=record_camera_video, video_base_dir=str(run_output_dir), ), + datagen_collector_factory=datagen_collector_factory, ) except Exception as error: results.append(ArenaRunResult(run_name=run_cfg.name, status=RunStatus.FAILED)) @@ -84,8 +93,14 @@ def build_and_run( cfg: ArenaRunCfg, output_dir: str | Path, video_cfg: VideoRecordingCfg | None = None, + datagen_collector_factory=None, ) -> ArenaRunResult: - """Build and execute one typed Arena run, then return its result.""" + """Build and execute one typed Arena run, then return its result. + + Args: + datagen_collector_factory: Optional Callable[[ArenaRunCfg, gym.Env], DatagenCollectorBase] + forwarded to _build_environment_from_cfg for each rebuild. + """ metrics_per_rebuild: list[MetricsDataCollection] = [] output_dir = str(output_dir) video_cfg = video_cfg or VideoRecordingCfg(video_base_dir=output_dir) @@ -105,7 +120,9 @@ def build_and_run( camera_name_prefix=f"robot-cam-rebuild{rebuild_index}", ) rebuild_cfg = _seed_cfg_for_rebuild(cfg, rebuild_index) - env = _build_environment_from_cfg(rebuild_cfg, rebuild_video_cfg.render_mode) + env = _build_environment_from_cfg( + rebuild_cfg, rebuild_video_cfg.render_mode, datagen_collector_factory=datagen_collector_factory + ) results_path = os.path.join(output_dir, f"episode_results_rebuild{rebuild_index}.jsonl") env.unwrapped.episode_recorder.set_job_name(cfg.name) env.unwrapped.episode_recorder.set_output_path(results_path) @@ -137,15 +154,53 @@ def _seed_cfg_for_rebuild(cfg: ArenaRunCfg, rebuild_index: int) -> ArenaRunCfg: return cfg +def _with_datagen_recorder_term( + recorders_cfg: RecorderManagerBaseCfg | None, + build_handlers, +) -> RecorderManagerBaseCfg: + """Merge a CallbackRecorderTerm using build_handlers into recorders_cfg. + + recorders_cfg may be None (no other recorder terms configured for this run). + """ + datagen_recorders_cfg = make_configclass( + "DatagenRecorderManagerCfg", + [("datagen_callback", CallbackRecorderTermCfg, CallbackRecorderTermCfg(build_handlers=build_handlers))], + bases=(RecorderManagerBaseCfg,), + )() + return combine_configclass_instances( + "RecorderManagerCfg", recorders_cfg, datagen_recorders_cfg, bases=(RecorderManagerBaseCfg,) + ) + + def _build_environment_from_cfg( cfg: ArenaRunCfg, render_mode: str | None, + datagen_collector_factory=None, ) -> gym.Env: - """Compile and instantiate a run's environment.""" + """Compile and instantiate a run's environment. + + Args: + datagen_collector_factory: Optional Callable[[ArenaRunCfg, gym.Env], DatagenCollectorBase]. + When given and cfg.datagen is not None, its collector is driven via a + CallbackRecorderTerm merged into the env's recorders config. + """ arena_builder = build_arena_builder_from_run_cfg(cfg) _, env_cfg, env_kwargs = arena_builder.build_registered() if env_cfg.recorders is not None: env_cfg.recorders.dataset_filename = f"dataset_{cfg.name}" + if datagen_collector_factory is not None and cfg.datagen is not None: + # ArenaEnvBuilder only sets env_cfg.recorders when mimic is disabled, so a + # requested datagen collector would silently never be invoked in mimic mode. + assert not cfg.environment_builder.mimic, ( + f"Run '{cfg.name}' requests datagen collection but mimic mode never sets env_cfg.recorders," + " so the collector would silently never be invoked" + ) + + def build_handlers(env: gym.Env, run_cfg: ArenaRunCfg = cfg) -> CallbackRecorderTermHandlers: + collector: DatagenCollectorBase = datagen_collector_factory(run_cfg, env) + return build_datagen_callback_handlers(collector, env=env) + + env_cfg.recorders = _with_datagen_recorder_term(env_cfg.recorders, build_handlers) return arena_builder.make_registered(env_cfg, env_kwargs, render_mode=render_mode) diff --git a/isaaclab_arena/tests/test_run_execution.py b/isaaclab_arena/tests/test_run_execution.py index d0ffc87c48..0b0cbab615 100644 --- a/isaaclab_arena/tests/test_run_execution.py +++ b/isaaclab_arena/tests/test_run_execution.py @@ -2,229 +2,54 @@ # All rights reserved. # # SPDX-License-Identifier: Apache-2.0 +"""Tests for run_execution's datagen recorder-term wiring. -from copy import deepcopy -from dataclasses import dataclass -from types import SimpleNamespace +Importing run_execution needs a running SimulationApp (it imports RecorderManagerBaseCfg +from isaaclab.managers.recorder_manager) -- see isaaclab_arena/tests/test_task_registry.py +for the established _test_/test_ + run_function_with_persistent_simulation_app pattern +this mirrors. +""" -import pytest +from __future__ import annotations -from isaaclab_arena.environments.arena_environment_factory import ArenaEnvironmentCfg -from isaaclab_arena.evaluation import run_execution -from isaaclab_arena.evaluation.arena_experiment import ArenaExperimentCfg -from isaaclab_arena.evaluation.arena_run import ArenaRunCfg, ArenaRunResult, RolloutLimitCfg, RunStatus -from isaaclab_arena.policy.policy_base import PolicyCfg +from isaaclab_arena.tests.utils.persistent_simulation_app import run_function_with_persistent_simulation_app -@dataclass -class _EnvironmentCfg(ArenaEnvironmentCfg): - pass +def _test_merges_datagen_term_into_none_recorders_cfg(simulation_app): + from isaaclab.managers.recorder_manager import RecorderManagerBaseCfg + from isaaclab_arena.evaluation.run_execution import _with_datagen_recorder_term + from isaaclab_arena.recording.callback_recorder_term import CallbackRecorderTermHandlers -@dataclass -class _PolicyCfg(PolicyCfg): - pass + merged = _with_datagen_recorder_term(None, build_handlers=lambda env: CallbackRecorderTermHandlers()) + assert isinstance(merged, RecorderManagerBaseCfg) + assert hasattr(merged, "datagen_callback") + return True -class _Policy: - def has_length(self): - return False +def test_merges_datagen_term_into_none_recorders_cfg(): + assert run_function_with_persistent_simulation_app(_test_merges_datagen_term_into_none_recorders_cfg) -class _EpisodeRecorder: - def set_job_name(self, name): - self.name = name - def set_output_path(self, path): - self.path = path +def _test_preserves_existing_recorder_terms_alongside_datagen_term(simulation_app): + from isaaclab.managers.recorder_manager import RecorderManagerBaseCfg + from isaaclab_arena.evaluation.run_execution import _with_datagen_recorder_term + from isaaclab_arena.recording.callback_recorder_term import CallbackRecorderTermHandlers + from isaaclab_arena.utils.configclass import make_configclass -def _environment(): - return SimpleNamespace(unwrapped=SimpleNamespace(episode_recorder=_EpisodeRecorder())) - - -def _run(**overrides): - values = { - "name": "test_run", - "environment": _EnvironmentCfg(), - "policy": _PolicyCfg(), - "rollout_limit": RolloutLimitCfg(num_episodes=5), - "num_rebuilds": 2, - } - values.update(overrides) - return ArenaRunCfg(**values) - - -def _experiment(*run_cfgs: ArenaRunCfg) -> ArenaExperimentCfg: - return ArenaExperimentCfg(runs={run_cfg.name: run_cfg for run_cfg in run_cfgs}) - - -def test_build_and_run_splits_episode_budget_without_mutating_config(monkeypatch, tmp_path): - run = _run() - rollout_limits = [] - received_run_cfgs = [] - - def make_environment(cfg, render_mode): - received_run_cfgs.append(cfg) - return _environment() - - monkeypatch.setattr(run_execution, "_build_environment_from_cfg", make_environment) - monkeypatch.setattr(run_execution, "_build_policy_from_cfg", lambda cfg: _Policy()) - monkeypatch.setattr(run_execution, "wrap_env_for_video", lambda env, video_cfg, steps, episodes: env) - monkeypatch.setattr(run_execution, "close_run_resources", lambda policy, env: None) - - def record_rollout(env, policy, num_steps, num_episodes): - rollout_limits.append((num_steps, num_episodes)) - - monkeypatch.setattr(run_execution, "rollout_policy", record_rollout) - - result = run_execution.build_and_run( - run, - output_dir=tmp_path, - ) - - base_seed = run.environment_builder.seed - run_seed_0 = deepcopy(run) # Rebuild 0 keeps the configured seed. - run_seed_1 = deepcopy(run) - run_seed_1.environment_builder.seed = base_seed + 1 - - assert result.run_name == "test_run" - assert result.status is RunStatus.COMPLETED - assert rollout_limits == [(None, 3), (None, 2)] - # Runs are the same except for their seeds. - assert received_run_cfgs == [run_seed_0, run_seed_1] - # The original config is never mutated. - assert run.rollout_limit == RolloutLimitCfg(num_episodes=5) - assert run.environment_builder.seed == base_seed - - -def test_seed_cfg_for_rebuild_offsets_seed_per_rebuild(): - run = _run(num_rebuilds=3) - base_seed = run.environment_builder.seed - - assert run_execution._seed_cfg_for_rebuild(run, 0).environment_builder.seed == base_seed - assert run_execution._seed_cfg_for_rebuild(run, 1).environment_builder.seed == base_seed + 1 - assert run_execution._seed_cfg_for_rebuild(run, 2).environment_builder.seed == base_seed + 2 - # The original config is never mutated. - assert run.environment_builder.seed == base_seed - - -def test_build_and_run_raises_and_closes_resources(monkeypatch, tmp_path): - closed_resources = [] - environment = _environment() - policy = _Policy() - - monkeypatch.setattr( - run_execution, - "_build_environment_from_cfg", - lambda cfg, render_mode: environment, - ) - monkeypatch.setattr(run_execution, "_build_policy_from_cfg", lambda cfg: policy) - monkeypatch.setattr(run_execution, "wrap_env_for_video", lambda env, video_cfg, steps, episodes: env) - monkeypatch.setattr( - run_execution, - "close_run_resources", - lambda closed_policy, closed_environment: closed_resources.append((closed_policy, closed_environment)), - ) - monkeypatch.setattr( - run_execution, - "rollout_policy", - lambda *args, **kwargs: (_ for _ in ()).throw(RuntimeError("rollout failed")), - ) - - with pytest.raises(RuntimeError, match="rollout failed"): - run_execution.build_and_run( - _run(rollout_limit=RolloutLimitCfg(num_steps=2), num_rebuilds=1), - output_dir=tmp_path, - ) - - assert closed_resources == [(policy, environment)] - - -def test_build_and_run_requires_a_limit_for_an_unbounded_policy(monkeypatch, tmp_path): - closed_resources = [] - environment = _environment() - policy = _Policy() - - monkeypatch.setattr( - run_execution, - "_build_environment_from_cfg", - lambda cfg, render_mode: environment, - ) - monkeypatch.setattr(run_execution, "_build_policy_from_cfg", lambda cfg: policy) - monkeypatch.setattr( - run_execution, - "close_run_resources", - lambda closed_policy, closed_environment: closed_resources.append((closed_policy, closed_environment)), - ) - - with pytest.raises(AssertionError, match="must configure num_steps or num_episodes"): - run_execution.build_and_run( - _run(rollout_limit=RolloutLimitCfg(), num_rebuilds=1), - output_dir=tmp_path, - ) - - assert closed_resources == [(policy, environment)] - - -def test_execute_experiment_runs_in_declaration_order(monkeypatch, tmp_path): - received = [] - - def build_and_run(run_cfg, output_dir, video_cfg): - received.append((run_cfg.name, output_dir, video_cfg.video_base_dir)) - return ArenaRunResult(run_name=run_cfg.name, status=RunStatus.COMPLETED) - - monkeypatch.setattr(run_execution, "build_and_run", build_and_run) - - results = run_execution.execute_experiment( - _experiment(_run(name="first"), _run(name="second")), - output_dir=tmp_path, + ExistingRecordersCfg = make_configclass( + "ExistingRecordersCfg", [("existing_term", object, "sentinel")], bases=(RecorderManagerBaseCfg,) ) + existing = ExistingRecordersCfg() - assert [result.run_name for result in results] == ["first", "second"] - assert received == [ - ("first", tmp_path / "first", str(tmp_path / "first")), - ("second", tmp_path / "second", str(tmp_path / "second")), - ] - - -def test_execute_experiment_records_failure_and_continues(monkeypatch, tmp_path): - attempted = [] - - def build_and_run(run_cfg, output_dir, video_cfg): - attempted.append(run_cfg.name) - if run_cfg.name == "failing": - raise RuntimeError("rollout failed") - return ArenaRunResult(run_name=run_cfg.name, status=RunStatus.COMPLETED) - - monkeypatch.setattr(run_execution, "build_and_run", build_and_run) - - results = run_execution.execute_experiment( - _experiment(_run(name="failing"), _run(name="passing")), - output_dir=tmp_path, - continue_on_error=True, - ) - - assert attempted == ["failing", "passing"] - assert [(result.run_name, result.status) for result in results] == [ - ("failing", RunStatus.FAILED), - ("passing", RunStatus.COMPLETED), - ] - - -def test_execute_experiment_stops_on_failure_by_default(monkeypatch, tmp_path): - attempted = [] - - def build_and_run(run_cfg, output_dir, video_cfg): - attempted.append(run_cfg.name) - raise RuntimeError("rollout failed") + merged = _with_datagen_recorder_term(existing, build_handlers=lambda env: CallbackRecorderTermHandlers()) - monkeypatch.setattr(run_execution, "build_and_run", build_and_run) + assert merged.existing_term == "sentinel" + assert hasattr(merged, "datagen_callback") + return True - with pytest.raises(RuntimeError, match="rollout failed"): - run_execution.execute_experiment( - _experiment(_run(name="failing"), _run(name="not_attempted")), - output_dir=tmp_path, - ) - assert attempted == ["failing"] +def test_preserves_existing_recorder_terms_alongside_datagen_term(): + assert run_function_with_persistent_simulation_app(_test_preserves_existing_recorder_terms_alongside_datagen_term) From 7e349c47a45a0044cb90793278bdc9a168e9f724 Mon Sep 17 00:00:00 2001 From: David Tingdahl Date: Thu, 10 Sep 2026 09:11:16 +0200 Subject: [PATCH 07/11] Add missing type annotations to datagen_collector_factory/build_handlers params Signed-off-by: David Tingdahl --- isaaclab_arena/evaluation/run_execution.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/isaaclab_arena/evaluation/run_execution.py b/isaaclab_arena/evaluation/run_execution.py index 1d8c0ee010..eee31df823 100644 --- a/isaaclab_arena/evaluation/run_execution.py +++ b/isaaclab_arena/evaluation/run_execution.py @@ -9,6 +9,7 @@ import os import traceback +from collections.abc import Callable from copy import deepcopy from dataclasses import fields, replace from pathlib import Path @@ -46,7 +47,7 @@ def execute_experiment( record_viewport_video: bool = False, record_camera_video: bool = False, continue_on_error: bool = False, - datagen_collector_factory=None, + datagen_collector_factory: Callable[[ArenaRunCfg, gym.Env], DatagenCollectorBase] | None = None, ) -> list[ArenaRunResult]: """Execute an experiment's runs in order and return their results. @@ -93,7 +94,7 @@ def build_and_run( cfg: ArenaRunCfg, output_dir: str | Path, video_cfg: VideoRecordingCfg | None = None, - datagen_collector_factory=None, + datagen_collector_factory: Callable[[ArenaRunCfg, gym.Env], DatagenCollectorBase] | None = None, ) -> ArenaRunResult: """Build and execute one typed Arena run, then return its result. @@ -156,7 +157,7 @@ def _seed_cfg_for_rebuild(cfg: ArenaRunCfg, rebuild_index: int) -> ArenaRunCfg: def _with_datagen_recorder_term( recorders_cfg: RecorderManagerBaseCfg | None, - build_handlers, + build_handlers: Callable[[gym.Env], CallbackRecorderTermHandlers], ) -> RecorderManagerBaseCfg: """Merge a CallbackRecorderTerm using build_handlers into recorders_cfg. @@ -175,7 +176,7 @@ def _with_datagen_recorder_term( def _build_environment_from_cfg( cfg: ArenaRunCfg, render_mode: str | None, - datagen_collector_factory=None, + datagen_collector_factory: Callable[[ArenaRunCfg, gym.Env], DatagenCollectorBase] | None = None, ) -> gym.Env: """Compile and instantiate a run's environment. From 7aa5c80aea738e17b3053a1e24fe28e8b19abede Mon Sep 17 00:00:00 2001 From: David Tingdahl Date: Thu, 10 Sep 2026 09:27:45 +0200 Subject: [PATCH 08/11] Thread datagen_collector_factory through experiment_runner.main Signed-off-by: David Tingdahl --- isaaclab_arena/evaluation/experiment_runner.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/isaaclab_arena/evaluation/experiment_runner.py b/isaaclab_arena/evaluation/experiment_runner.py index 7ab6b3c9a8..c1ede1bcaa 100644 --- a/isaaclab_arena/evaluation/experiment_runner.py +++ b/isaaclab_arena/evaluation/experiment_runner.py @@ -95,7 +95,15 @@ def _write_arena_experiment_result( return ArenaExperimentResult(experiment_output_directory, run_metadata_by_name).write() -def main(): +def main(datagen_collector_factory=None): + """Run an Arena Experiment (one or more typed or legacy-JSON Runs). + + Args: + datagen_collector_factory: Optional Callable[[ArenaRunCfg, gym.Env], DatagenCollectorBase]. + When given, each Run whose ArenaRunCfg.datagen is not None gets a collector + built from it, driven via a CallbackRecorderTerm. When None, no datagen + collection runs and behavior matches the plain evaluation path. + """ args_cli, experiment_overrides = parse_experiment_runner_args() experiment_config_path = validate_experiment_config_path(args_cli.experiment_config) legacy_experiment_config = load_legacy_json_experiment_config( @@ -173,6 +181,7 @@ def main(): record_viewport_video=args_cli.record_viewport_video, record_camera_video=args_cli.record_camera_video, continue_on_error=args_cli.continue_on_error, + datagen_collector_factory=datagen_collector_factory, ) for run_result in run_results: if run_result.metrics is not None: From a405f943884176b55fc03a03d10a7401b9259058 Mon Sep 17 00:00:00 2001 From: David Tingdahl Date: Thu, 10 Sep 2026 09:37:22 +0200 Subject: [PATCH 09/11] Add missing type annotation to datagen_collector_factory in experiment_runner.main Annotate the datagen_collector_factory parameter with the same type signature used in run_execution.execute_experiment: Callable[[ArenaRunCfg, gym.Env], DatagenCollectorBase] | None. Add required imports to TYPE_CHECKING block to support the annotation, matching the import convention already used in run_execution.py. This fixes the inconsistency flagged in code review (same issue class as Task 5). Signed-off-by: David Tingdahl --- isaaclab_arena/evaluation/experiment_runner.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/isaaclab_arena/evaluation/experiment_runner.py b/isaaclab_arena/evaluation/experiment_runner.py index c1ede1bcaa..2fae547e4e 100644 --- a/isaaclab_arena/evaluation/experiment_runner.py +++ b/isaaclab_arena/evaluation/experiment_runner.py @@ -5,6 +5,7 @@ from __future__ import annotations +from collections.abc import Callable from pathlib import Path from typing import TYPE_CHECKING @@ -23,8 +24,11 @@ from isaaclab_arena.video.video_recording import timestamped_run_dir if TYPE_CHECKING: + import gymnasium as gym + from isaaclab_arena.evaluation.arena_experiment import ArenaExperimentCfg - from isaaclab_arena.evaluation.arena_run import ArenaRunResult + from isaaclab_arena.evaluation.arena_run import ArenaRunCfg, ArenaRunResult + from isaaclab_arena.evaluation.datagen_collector import DatagenCollectorBase # TODO(cvolk): Move experiment-level variation inspection out of this CLI entry point. @@ -95,7 +99,7 @@ def _write_arena_experiment_result( return ArenaExperimentResult(experiment_output_directory, run_metadata_by_name).write() -def main(datagen_collector_factory=None): +def main(datagen_collector_factory: Callable[[ArenaRunCfg, gym.Env], DatagenCollectorBase] | None = None) -> None: """Run an Arena Experiment (one or more typed or legacy-JSON Runs). Args: From 8be16b879c1209a00433b7a26392be112f9cbf62 Mon Sep 17 00:00:00 2001 From: David Tingdahl Date: Thu, 10 Sep 2026 09:49:26 +0200 Subject: [PATCH 10/11] Fix black/isort formatting caught by a full-suite pre-commit run Task 1 and Task 2's test files were never run through pre-commit as their own diffs; running it across everything this branch touched (Task 7's verification step) caught black reformatting and an isort ordering issue. No behavior change -- all 10 affected tests still pass. Signed-off-by: David Tingdahl --- .../tests/test_callback_recorder_term.py | 16 ++++------------ isaaclab_arena/tests/test_episode_outcome.py | 8 ++------ .../test_isaaclab_arena_manager_based_env_cfg.py | 8 ++------ 3 files changed, 8 insertions(+), 24 deletions(-) diff --git a/isaaclab_arena/tests/test_callback_recorder_term.py b/isaaclab_arena/tests/test_callback_recorder_term.py index cf05685ce7..6d848ce1c5 100644 --- a/isaaclab_arena/tests/test_callback_recorder_term.py +++ b/isaaclab_arena/tests/test_callback_recorder_term.py @@ -40,9 +40,7 @@ def build_handlers(env): def test_build_handlers_called_once_at_construction_with_env(): - result = run_function_with_persistent_simulation_app( - _test_build_handlers_called_once_at_construction_with_env - ) + result = run_function_with_persistent_simulation_app(_test_build_handlers_called_once_at_construction_with_env) assert result @@ -92,9 +90,7 @@ def _test_record_pre_reset_forwards_env_and_env_ids(simulation_app): def test_record_pre_reset_forwards_env_and_env_ids(): - result = run_function_with_persistent_simulation_app( - _test_record_pre_reset_forwards_env_and_env_ids - ) + result = run_function_with_persistent_simulation_app(_test_record_pre_reset_forwards_env_and_env_ids) assert result @@ -117,9 +113,7 @@ def _test_close_forwards_to_on_close(simulation_app): def test_close_forwards_to_on_close(): - result = run_function_with_persistent_simulation_app( - _test_close_forwards_to_on_close - ) + result = run_function_with_persistent_simulation_app(_test_close_forwards_to_on_close) assert result @@ -141,7 +135,5 @@ def _test_none_handlers_are_no_ops(simulation_app): def test_none_handlers_are_no_ops(): - result = run_function_with_persistent_simulation_app( - _test_none_handlers_are_no_ops - ) + result = run_function_with_persistent_simulation_app(_test_none_handlers_are_no_ops) assert result diff --git a/isaaclab_arena/tests/test_episode_outcome.py b/isaaclab_arena/tests/test_episode_outcome.py index 68dcc0ebaa..51fc0969a6 100644 --- a/isaaclab_arena/tests/test_episode_outcome.py +++ b/isaaclab_arena/tests/test_episode_outcome.py @@ -36,16 +36,12 @@ def test_success_term_true_means_success(): def test_time_out_term_true_means_timeout(): - env = _FakeEnv( - _FakeTerminationManager({"success": torch.tensor([False]), "time_out": torch.tensor([True])}) - ) + env = _FakeEnv(_FakeTerminationManager({"success": torch.tensor([False]), "time_out": torch.tensor([True])})) assert classify_outcome(env, 0) == "timeout" def test_neither_term_true_means_failure(): - env = _FakeEnv( - _FakeTerminationManager({"success": torch.tensor([False]), "time_out": torch.tensor([False])}) - ) + env = _FakeEnv(_FakeTerminationManager({"success": torch.tensor([False]), "time_out": torch.tensor([False])})) assert classify_outcome(env, 0) == "failure" diff --git a/isaaclab_arena/tests/test_isaaclab_arena_manager_based_env_cfg.py b/isaaclab_arena/tests/test_isaaclab_arena_manager_based_env_cfg.py index 21185188dd..afd2b3ef87 100644 --- a/isaaclab_arena/tests/test_isaaclab_arena_manager_based_env_cfg.py +++ b/isaaclab_arena/tests/test_isaaclab_arena_manager_based_env_cfg.py @@ -17,16 +17,12 @@ def _test_default_reruns_after_reset_to_flush_stale_camera_frames(simulation_app): """A positive default avoids RTX sensors reading the previous episode's last frame.""" - from isaaclab_arena.environments.isaaclab_arena_manager_based_env_cfg import ( - IsaacLabArenaManagerBasedRLEnvCfg, - ) + from isaaclab_arena.environments.isaaclab_arena_manager_based_env_cfg import IsaacLabArenaManagerBasedRLEnvCfg assert IsaacLabArenaManagerBasedRLEnvCfg().num_rerenders_on_reset == 5 return True def test_default_reruns_after_reset_to_flush_stale_camera_frames(): - result = run_function_with_persistent_simulation_app( - _test_default_reruns_after_reset_to_flush_stale_camera_frames - ) + result = run_function_with_persistent_simulation_app(_test_default_reruns_after_reset_to_flush_stale_camera_frames) assert result From 7b21932f8c0e8c396a2aee225963c4be7a5952e6 Mon Sep 17 00:00:00 2001 From: David Tingdahl Date: Thu, 10 Sep 2026 10:14:59 +0200 Subject: [PATCH 11/11] Fix review findings: restore run_execution tests, recorder merge order, MISSING default Restore the 7 pre-existing build_and_run/execute_experiment/_seed_cfg_for_rebuild tests that a prior datagen task accidentally dropped when it replaced test_run_execution.py, wrapping all 9 tests (7 restored + 2 datagen) in the _test_/test_ + run_function_with_persistent_simulation_app pattern now required because run_execution imports isaaclab.managers.recorder_manager at module level, and updating fakes for _build_environment_from_cfg/build_and_run to accept the datagen_collector_factory parameter added since. Fix _with_datagen_recorder_term to merge recorders_cfg after datagen_recorders_cfg so an already-configured run's recorder settings win over datagen_recorders_cfg's inherited base-class defaults, and add a regression test asserting a pre-set field survives the merge. Change CallbackRecorderTermCfg.build_handlers's default from None to MISSING so an unset field fails with IsaacLab's own clear error instead of a TypeError deep inside RecorderTerm construction. Signed-off-by: David Tingdahl --- isaaclab_arena/evaluation/run_execution.py | 4 +- .../recording/callback_recorder_term.py | 4 +- isaaclab_arena/tests/test_run_execution.py | 301 +++++++++++++++++- 3 files changed, 301 insertions(+), 8 deletions(-) diff --git a/isaaclab_arena/evaluation/run_execution.py b/isaaclab_arena/evaluation/run_execution.py index eee31df823..8f2536649d 100644 --- a/isaaclab_arena/evaluation/run_execution.py +++ b/isaaclab_arena/evaluation/run_execution.py @@ -168,8 +168,10 @@ def _with_datagen_recorder_term( [("datagen_callback", CallbackRecorderTermCfg, CallbackRecorderTermCfg(build_handlers=build_handlers))], bases=(RecorderManagerBaseCfg,), )() + # datagen_recorders_cfg is passed last so recorders_cfg's already-configured values (not + # datagen_recorders_cfg's inherited base-class defaults) win on any field both share. return combine_configclass_instances( - "RecorderManagerCfg", recorders_cfg, datagen_recorders_cfg, bases=(RecorderManagerBaseCfg,) + "RecorderManagerCfg", datagen_recorders_cfg, recorders_cfg, bases=(RecorderManagerBaseCfg,) ) diff --git a/isaaclab_arena/recording/callback_recorder_term.py b/isaaclab_arena/recording/callback_recorder_term.py index 785c178878..b4464ba87c 100644 --- a/isaaclab_arena/recording/callback_recorder_term.py +++ b/isaaclab_arena/recording/callback_recorder_term.py @@ -13,7 +13,7 @@ from __future__ import annotations from collections.abc import Callable, Sequence -from dataclasses import dataclass +from dataclasses import MISSING, dataclass from typing import TYPE_CHECKING from isaaclab.managers import RecorderTerm, RecorderTermCfg @@ -70,7 +70,7 @@ class CallbackRecorderTermCfg(RecorderTermCfg): class_type: type[RecorderTerm] = CallbackRecorderTerm - build_handlers: Callable[[ManagerBasedEnv], CallbackRecorderTermHandlers] = None + build_handlers: Callable[[ManagerBasedEnv], CallbackRecorderTermHandlers] = MISSING """Called once, at term construction time, with the live env, to obtain the handlers. Deferred like this (rather than passing already-built handlers) because RecorderTerm diff --git a/isaaclab_arena/tests/test_run_execution.py b/isaaclab_arena/tests/test_run_execution.py index 0b0cbab615..3bc4a43bc6 100644 --- a/isaaclab_arena/tests/test_run_execution.py +++ b/isaaclab_arena/tests/test_run_execution.py @@ -1,20 +1,306 @@ -# Copyright (c) 2026, The Isaac Lab Arena Project Developers (https://github.com/isaac-sim/IsaacLab-Arena/blob/main/CONTRIBUTORS.md). +# Copyright (c) 2025-2026, The Isaac Lab Arena Project Developers (https://github.com/isaac-sim/IsaacLab-Arena/blob/main/CONTRIBUTORS.md). # All rights reserved. # # SPDX-License-Identifier: Apache-2.0 -"""Tests for run_execution's datagen recorder-term wiring. +"""Tests for run_execution's Run/Experiment orchestration and datagen recorder-term wiring. Importing run_execution needs a running SimulationApp (it imports RecorderManagerBaseCfg -from isaaclab.managers.recorder_manager) -- see isaaclab_arena/tests/test_task_registry.py -for the established _test_/test_ + run_function_with_persistent_simulation_app pattern -this mirrors. +from isaaclab.managers.recorder_manager) -- so each test case follows the established +_test_/test_ + run_function_with_persistent_simulation_app pattern from +isaaclab_arena/tests/test_task_registry.py. Helpers that only reference plain Arena +configs (no isaaclab imports) are defined once at module scope and shared across cases. """ from __future__ import annotations +from copy import deepcopy +from dataclasses import dataclass +from types import SimpleNamespace + +import pytest + +from isaaclab_arena.environments.arena_environment_factory import ArenaEnvironmentCfg +from isaaclab_arena.evaluation.arena_experiment import ArenaExperimentCfg +from isaaclab_arena.evaluation.arena_run import ArenaRunCfg, ArenaRunResult, RolloutLimitCfg, RunStatus +from isaaclab_arena.policy.policy_base import PolicyCfg from isaaclab_arena.tests.utils.persistent_simulation_app import run_function_with_persistent_simulation_app +@dataclass +class _EnvironmentCfg(ArenaEnvironmentCfg): + pass + + +@dataclass +class _PolicyCfg(PolicyCfg): + pass + + +class _Policy: + def has_length(self): + return False + + +class _EpisodeRecorder: + def set_job_name(self, name): + self.name = name + + def set_output_path(self, path): + self.path = path + + +def _environment(): + return SimpleNamespace(unwrapped=SimpleNamespace(episode_recorder=_EpisodeRecorder())) + + +def _run(**overrides): + values = { + "name": "test_run", + "environment": _EnvironmentCfg(), + "policy": _PolicyCfg(), + "rollout_limit": RolloutLimitCfg(num_episodes=5), + "num_rebuilds": 2, + } + values.update(overrides) + return ArenaRunCfg(**values) + + +def _experiment(*run_cfgs: ArenaRunCfg) -> ArenaExperimentCfg: + return ArenaExperimentCfg(runs={run_cfg.name: run_cfg for run_cfg in run_cfgs}) + + +def _test_build_and_run_splits_episode_budget_without_mutating_config(simulation_app, monkeypatch, tmp_path): + from isaaclab_arena.evaluation import run_execution + + run = _run() + rollout_limits = [] + received_run_cfgs = [] + + def make_environment(cfg, render_mode, datagen_collector_factory=None): + received_run_cfgs.append(cfg) + return _environment() + + monkeypatch.setattr(run_execution, "_build_environment_from_cfg", make_environment) + monkeypatch.setattr(run_execution, "_build_policy_from_cfg", lambda cfg: _Policy()) + monkeypatch.setattr(run_execution, "wrap_env_for_video", lambda env, video_cfg, steps, episodes: env) + monkeypatch.setattr(run_execution, "close_run_resources", lambda policy, env: None) + + def record_rollout(env, policy, num_steps, num_episodes): + rollout_limits.append((num_steps, num_episodes)) + + monkeypatch.setattr(run_execution, "rollout_policy", record_rollout) + + result = run_execution.build_and_run( + run, + output_dir=tmp_path, + ) + + base_seed = run.environment_builder.seed + run_seed_0 = deepcopy(run) # Rebuild 0 keeps the configured seed. + run_seed_1 = deepcopy(run) + run_seed_1.environment_builder.seed = base_seed + 1 + + assert result.run_name == "test_run" + assert result.status is RunStatus.COMPLETED + assert rollout_limits == [(None, 3), (None, 2)] + # Runs are the same except for their seeds. + assert received_run_cfgs == [run_seed_0, run_seed_1] + # The original config is never mutated. + assert run.rollout_limit == RolloutLimitCfg(num_episodes=5) + assert run.environment_builder.seed == base_seed + return True + + +def test_build_and_run_splits_episode_budget_without_mutating_config(monkeypatch, tmp_path): + assert run_function_with_persistent_simulation_app( + _test_build_and_run_splits_episode_budget_without_mutating_config, monkeypatch=monkeypatch, tmp_path=tmp_path + ) + + +def _test_seed_cfg_for_rebuild_offsets_seed_per_rebuild(simulation_app): + from isaaclab_arena.evaluation import run_execution + + run = _run(num_rebuilds=3) + base_seed = run.environment_builder.seed + + assert run_execution._seed_cfg_for_rebuild(run, 0).environment_builder.seed == base_seed + assert run_execution._seed_cfg_for_rebuild(run, 1).environment_builder.seed == base_seed + 1 + assert run_execution._seed_cfg_for_rebuild(run, 2).environment_builder.seed == base_seed + 2 + # The original config is never mutated. + assert run.environment_builder.seed == base_seed + return True + + +def test_seed_cfg_for_rebuild_offsets_seed_per_rebuild(): + assert run_function_with_persistent_simulation_app(_test_seed_cfg_for_rebuild_offsets_seed_per_rebuild) + + +def _test_build_and_run_raises_and_closes_resources(simulation_app, monkeypatch, tmp_path): + from isaaclab_arena.evaluation import run_execution + + closed_resources = [] + environment = _environment() + policy = _Policy() + + monkeypatch.setattr( + run_execution, + "_build_environment_from_cfg", + lambda cfg, render_mode, datagen_collector_factory=None: environment, + ) + monkeypatch.setattr(run_execution, "_build_policy_from_cfg", lambda cfg: policy) + monkeypatch.setattr(run_execution, "wrap_env_for_video", lambda env, video_cfg, steps, episodes: env) + monkeypatch.setattr( + run_execution, + "close_run_resources", + lambda closed_policy, closed_environment: closed_resources.append((closed_policy, closed_environment)), + ) + monkeypatch.setattr( + run_execution, + "rollout_policy", + lambda *args, **kwargs: (_ for _ in ()).throw(RuntimeError("rollout failed")), + ) + + with pytest.raises(RuntimeError, match="rollout failed"): + run_execution.build_and_run( + _run(rollout_limit=RolloutLimitCfg(num_steps=2), num_rebuilds=1), + output_dir=tmp_path, + ) + + assert closed_resources == [(policy, environment)] + return True + + +def test_build_and_run_raises_and_closes_resources(monkeypatch, tmp_path): + assert run_function_with_persistent_simulation_app( + _test_build_and_run_raises_and_closes_resources, monkeypatch=monkeypatch, tmp_path=tmp_path + ) + + +def _test_build_and_run_requires_a_limit_for_an_unbounded_policy(simulation_app, monkeypatch, tmp_path): + from isaaclab_arena.evaluation import run_execution + + closed_resources = [] + environment = _environment() + policy = _Policy() + + monkeypatch.setattr( + run_execution, + "_build_environment_from_cfg", + lambda cfg, render_mode, datagen_collector_factory=None: environment, + ) + monkeypatch.setattr(run_execution, "_build_policy_from_cfg", lambda cfg: policy) + monkeypatch.setattr( + run_execution, + "close_run_resources", + lambda closed_policy, closed_environment: closed_resources.append((closed_policy, closed_environment)), + ) + + with pytest.raises(AssertionError, match="must configure num_steps or num_episodes"): + run_execution.build_and_run( + _run(rollout_limit=RolloutLimitCfg(), num_rebuilds=1), + output_dir=tmp_path, + ) + + assert closed_resources == [(policy, environment)] + return True + + +def test_build_and_run_requires_a_limit_for_an_unbounded_policy(monkeypatch, tmp_path): + assert run_function_with_persistent_simulation_app( + _test_build_and_run_requires_a_limit_for_an_unbounded_policy, monkeypatch=monkeypatch, tmp_path=tmp_path + ) + + +def _test_execute_experiment_runs_in_declaration_order(simulation_app, monkeypatch, tmp_path): + from isaaclab_arena.evaluation import run_execution + + received = [] + + def build_and_run(run_cfg, output_dir, video_cfg, datagen_collector_factory=None): + received.append((run_cfg.name, output_dir, video_cfg.video_base_dir)) + return ArenaRunResult(run_name=run_cfg.name, status=RunStatus.COMPLETED) + + monkeypatch.setattr(run_execution, "build_and_run", build_and_run) + + results = run_execution.execute_experiment( + _experiment(_run(name="first"), _run(name="second")), + output_dir=tmp_path, + ) + + assert [result.run_name for result in results] == ["first", "second"] + assert received == [ + ("first", tmp_path / "first", str(tmp_path / "first")), + ("second", tmp_path / "second", str(tmp_path / "second")), + ] + return True + + +def test_execute_experiment_runs_in_declaration_order(monkeypatch, tmp_path): + assert run_function_with_persistent_simulation_app( + _test_execute_experiment_runs_in_declaration_order, monkeypatch=monkeypatch, tmp_path=tmp_path + ) + + +def _test_execute_experiment_records_failure_and_continues(simulation_app, monkeypatch, tmp_path): + from isaaclab_arena.evaluation import run_execution + + attempted = [] + + def build_and_run(run_cfg, output_dir, video_cfg, datagen_collector_factory=None): + attempted.append(run_cfg.name) + if run_cfg.name == "failing": + raise RuntimeError("rollout failed") + return ArenaRunResult(run_name=run_cfg.name, status=RunStatus.COMPLETED) + + monkeypatch.setattr(run_execution, "build_and_run", build_and_run) + + results = run_execution.execute_experiment( + _experiment(_run(name="failing"), _run(name="passing")), + output_dir=tmp_path, + continue_on_error=True, + ) + + assert attempted == ["failing", "passing"] + assert [(result.run_name, result.status) for result in results] == [ + ("failing", RunStatus.FAILED), + ("passing", RunStatus.COMPLETED), + ] + return True + + +def test_execute_experiment_records_failure_and_continues(monkeypatch, tmp_path): + assert run_function_with_persistent_simulation_app( + _test_execute_experiment_records_failure_and_continues, monkeypatch=monkeypatch, tmp_path=tmp_path + ) + + +def _test_execute_experiment_stops_on_failure_by_default(simulation_app, monkeypatch, tmp_path): + from isaaclab_arena.evaluation import run_execution + + attempted = [] + + def build_and_run(run_cfg, output_dir, video_cfg, datagen_collector_factory=None): + attempted.append(run_cfg.name) + raise RuntimeError("rollout failed") + + monkeypatch.setattr(run_execution, "build_and_run", build_and_run) + + with pytest.raises(RuntimeError, match="rollout failed"): + run_execution.execute_experiment( + _experiment(_run(name="failing"), _run(name="not_attempted")), + output_dir=tmp_path, + ) + + assert attempted == ["failing"] + return True + + +def test_execute_experiment_stops_on_failure_by_default(monkeypatch, tmp_path): + assert run_function_with_persistent_simulation_app( + _test_execute_experiment_stops_on_failure_by_default, monkeypatch=monkeypatch, tmp_path=tmp_path + ) + + def _test_merges_datagen_term_into_none_recorders_cfg(simulation_app): from isaaclab.managers.recorder_manager import RecorderManagerBaseCfg @@ -43,10 +329,15 @@ def _test_preserves_existing_recorder_terms_alongside_datagen_term(simulation_ap "ExistingRecordersCfg", [("existing_term", object, "sentinel")], bases=(RecorderManagerBaseCfg,) ) existing = ExistingRecordersCfg() + # A pre-set value on a field RecorderManagerBaseCfg also declares (not just existing_term, + # which only datagen_recorders_cfg lacks): merging must not let datagen_recorders_cfg's + # inherited base-class default for this field silently overwrite it. + existing.dataset_filename = "already_configured_dataset" merged = _with_datagen_recorder_term(existing, build_handlers=lambda env: CallbackRecorderTermHandlers()) assert merged.existing_term == "sentinel" + assert merged.dataset_filename == "already_configured_dataset" assert hasattr(merged, "datagen_callback") return True