Centralize video-recording env wrapping by alexmillane · Pull Request #801 · isaac-sim/IsaacLab-Arena

alexmillane · 2026-06-16T12:56:53Z

Summary

Move the inline video-recorder wrapping out of the two rollout runners into a single video_recording module.

Details

Goals:
- simplify (i.e. shorten) eval_runner.py and policy_runner.py
- remove duplicated wrapping code
Method:
- move wrapping code into a function.

…ecording_per_episode

Move the inline RecordVideo / CameraObsVideoRecorder wrapping out of policy_runner and eval_runner into a single video_recording module (VideoRecordingCfg + wrap_env_for_video), so the gym-wrapper plumbing lives in one place instead of being duplicated across the two runners. Signed-off-by: alex <amillane@nvidia.com>

greptile-apps · 2026-06-16T12:59:54Z

Greptile Summary

This PR extracts the duplicated RecordVideo / CameraObsVideoRecorder wrapping logic from policy_runner.py and eval_runner.py into a new isaaclab_arena/video/video_recording.py module, and simultaneously renames the CLI flags (--video → --record_viewport_video, --camera_video → --record_camera_video, --video_dir → --video_base_dir) and moves camera_video.py to isaaclab_arena/video/camera_observation_video_recorder.py.

New video_recording.py introduces VideoRecordingCfg, timestamped_run_dir, and wrap_env_for_video as the single authoritative place for video-wrapper plumbing; eval_runner calls timestamped_run_dir once and shares the result across all jobs (correct), while policy_runner calls it once per rank process (potential divergence in distributed runs).
CLI breaking change: the old --video, --camera_video, and --video_dir flags no longer exist; any external scripts relying on them will silently stop recording without error.
Test file rename (test_camera_video.py → test_camera_observation_video_recorder.py) keeps all patch targets in sync with the new module path.

Confidence Score: 4/5

Safe to merge for single-rank runs; distributed video output directories may diverge across ranks if processes cross the timestamped_run_dir call on different clock seconds.

The refactor is clean and the eval_runner path is correct (one shared timestamp for all jobs). The concern is in policy_runner.py: each rank process independently computes its own timestamped subdirectory, so in a distributed run where two processes differ by even one second they'll write to separate folders. The old code shared the single CLI-specified path across all ranks, making this a genuine behavioral regression for distributed video recording.

isaaclab_arena/evaluation/policy_runner.py — the timestamped_run_dir call at line 194 runs independently in each rank process.

Important Files Changed

Filename	Overview
isaaclab_arena/evaluation/policy_runner.py	Correctly removes duplicated wrapping code and drops unused imports; however, `timestamped_run_dir` is called once per rank process, which can produce diverging output paths in distributed runs where the old code used a shared CLI path.
isaaclab_arena/video/video_recording.py	New centralized module for video wrapping; clean design with `VideoRecordingCfg`, `timestamped_run_dir`, and `wrap_env_for_video`. The distributed-rank timestamp divergence issue originates from how callers use `timestamped_run_dir`.
isaaclab_arena/evaluation/eval_runner.py	Correctly calls `timestamped_run_dir` once before the jobs loop and shares the result across all per-job `VideoRecordingCfg` instances; flag renames are consistent.
isaaclab_arena/evaluation/policy_runner_cli.py	CLI arguments cleanly renamed; hyphen aliases dropped intentionally.
isaaclab_arena/evaluation/eval_runner_cli.py	Parallel flag renames to match policy_runner_cli.py; updated help text accurately describes the new reverse-dated subdirectory layout.
isaaclab_arena/tests/test_camera_observation_video_recorder.py	Renamed from `test_camera_video.py`; all `patch` targets updated to the new module path; no logic changes.
isaaclab_arena/video/camera_observation_video_recorder.py	Moved from `evaluation/camera_video.py`; only the docstring reference to old CLI flag names was updated; no functional changes.
isaaclab_arena/video/init.py	New empty package init with standard Apache-2.0 header; no issues.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant CLI as CLI / args_cli
    participant PR as policy_runner.py
    participant ER as eval_runner.py
    participant VR as video_recording.py
    participant RV as RecordVideo (gym)
    participant CR as CameraObsVideoRecorder

    CLI->>PR: --record_viewport_video / --record_camera_video
    PR->>VR: timestamped_run_dir(video_base_dir)
    VR-->>PR: run_dir (per-rank timestamp ⚠️)
    PR->>VR: VideoRecordingCfg(run_dir)
    PR->>VR: wrap_env_for_video(env, cfg, num_steps, num_episodes)
    VR->>RV: RecordVideo(env, video_folder, video_length) [if record_viewport_video]
    VR->>CR: CameraObsVideoRecorder(env, video_folder) [if record_camera_video]
    VR-->>PR: wrapped env

    CLI->>ER: --record_viewport_video / --record_camera_video
    ER->>VR: timestamped_run_dir(video_base_dir) [once, shared]
    loop each job
        ER->>VR: VideoRecordingCfg(run_dir/job.name)
        ER->>VR: wrap_env_for_video(env, cfg, num_steps, num_episodes)
        VR->>RV: RecordVideo(...) [if record_viewport_video]
        VR->>CR: CameraObsVideoRecorder(...) [if record_camera_video]
        VR-->>ER: wrapped env
    end

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant CLI as CLI / args_cli
    participant PR as policy_runner.py
    participant ER as eval_runner.py
    participant VR as video_recording.py
    participant RV as RecordVideo (gym)
    participant CR as CameraObsVideoRecorder

    CLI->>PR: --record_viewport_video / --record_camera_video
    PR->>VR: timestamped_run_dir(video_base_dir)
    VR-->>PR: run_dir (per-rank timestamp ⚠️)
    PR->>VR: VideoRecordingCfg(run_dir)
    PR->>VR: wrap_env_for_video(env, cfg, num_steps, num_episodes)
    VR->>RV: RecordVideo(env, video_folder, video_length) [if record_viewport_video]
    VR->>CR: CameraObsVideoRecorder(env, video_folder) [if record_camera_video]
    VR-->>PR: wrapped env

    CLI->>ER: --record_viewport_video / --record_camera_video
    ER->>VR: timestamped_run_dir(video_base_dir) [once, shared]
    loop each job
        ER->>VR: VideoRecordingCfg(run_dir/job.name)
        ER->>VR: wrap_env_for_video(env, cfg, num_steps, num_episodes)
        VR->>RV: RecordVideo(...) [if record_viewport_video]
        VR->>CR: CameraObsVideoRecorder(...) [if record_camera_video]
        VR-->>ER: wrapped env
    end

_{Reviews (5): Last reviewed commit: "Merge branch 'main' into alex/refactor/c..." | Re-trigger Greptile}

greptile-apps · 2026-06-16T12:59:58Z

+def _resolve_video_length(env, num_steps: int | None, num_episodes: int | None) -> int:
+    """Number of env steps to record: the step budget, or one episode's worth per episode.
+
+    ``max_episode_length`` is in environment steps, which matches the rollout cadence.
+    """
+    if num_steps is not None:
+        return num_steps
+    return num_episodes * env.unwrapped.max_episode_length


Missing guard against both num_steps and num_episodes being None. When _resolve_video_length is reached with both arguments None, None * env.unwrapped.max_episode_length raises an unhelpful TypeError. This can happen in eval_runner when a job has neither a step nor episode budget and the policy doesn't report a length, while --video is enabled. Adding an explicit check here makes the failure message actionable.

Suggested change

def _resolve_video_length(env, num_steps: int | None, num_episodes: int | None) -> int:

"""Number of env steps to record: the step budget, or one episode's worth per episode.

``max_episode_length`` is in environment steps, which matches the rollout cadence.

"""

if num_steps is not None:

return num_steps

return num_episodes * env.unwrapped.max_episode_length

def _resolve_video_length(env, num_steps: int | None, num_episodes: int | None) -> int:

"""Number of env steps to record: the step budget, or one episode's worth per episode.

``max_episode_length`` is in environment steps, which matches the rollout cadence.

"""

if num_steps is not None:

return num_steps

if num_episodes is None:

raise ValueError(

"wrap_env_for_video: cannot determine video length — "

"both num_steps and num_episodes are None."

)

return num_episodes * env.unwrapped.max_episode_length

greptile-apps · 2026-06-16T12:59:59Z

+        print(f"Recording {video_length}-step viewport video to: {video_cfg.video_dir}")
+
+    # --camera_video records the embodiment-mounted cameras (from obs["camera_obs"]),
+    # flushed at each episode reset rather than after a fixed number of steps.
+    if video_cfg.camera_video:
+        env = CameraObsVideoRecorder(
+            env,
+            video_folder=video_cfg.video_dir,
+            name_prefix=video_cfg.camera_name_prefix,
+        )
+        print(f"Recording per-episode per-camera videos to: {video_cfg.video_dir}")


Rank context dropped from log messages

The original policy_runner.py included [Rank {local_rank}/{world_size}] in both recording log lines, which is useful when running distributed rollouts to identify which worker is actually writing video. The new generic messages in wrap_env_for_video omit that context, making multi-rank logs harder to correlate. Consider accepting an optional log_prefix: str = "" argument so callers can inject rank context.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

alexmillane

Self review.

alexmillane · 2026-06-16T13:04:38Z

-                video_folder=args_cli.video_dir,
-            )
-            print(f"[Rank {local_rank}/{world_size}] Recording per-episode per-camera videos to: {args_cli.video_dir}")
+        # Optionally wrap with the viewport/camera video recorders (both independent).


Comment: "Optionally wrap with the viewport/camera video recorders"

alexmillane · 2026-06-16T13:04:55Z

+"""Central helpers for wrapping a rollout env with video recorders.
+
+Two independent recordings are supported and may be active at the same time:
+
+* ``video`` records the kit viewport (third-person scene view) via ``env.render()``
+  using gymnasium's ``RecordVideo``.
+* ``camera_video`` records the embodiment-mounted cameras from ``obs['camera_obs']``
+  using ``CameraObsVideoRecorder``.
+
+The runners (``policy_runner``, ``eval_runner``) build a ``VideoRecordingCfg`` from their
+CLI/job options and apply it through ``wrap_env_for_video``, so the gym-wrapper plumbing lives
+in one place instead of being duplicated inline.
+"""


Remove module level comment.

alexmillane · 2026-06-16T13:05:31Z

+class VideoRecordingCfg:
+    """Options describing which rollout video recorders to enable and where to write them."""
+
+    video: bool = False


Change variable name to record_viewport_video and corresponding flags.

alexmillane · 2026-06-16T13:06:09Z

+    video: bool = False
+    """Record the kit viewport (third-person scene view) via ``env.render()``."""
+
+    camera_video: bool = False


Change variable name to record_camera_video and corresponding flags

alexmillane · 2026-06-16T13:07:06Z

+    camera_video: bool = False
+    """Record the embodiment-mounted cameras from ``obs['camera_obs']``."""
+
+    video_dir: str = "videos"


Can we make this video_base_dir (change corresponding flags).

Then we should add a subdirectory with a reverse dated folder, like Isaac Lab does for its logs.

alexmillane · 2026-06-16T13:08:06Z

+
+    Args:
+        env: The env to wrap (already built with ``video_cfg.render_mode`` when ``video`` is set).
+        video_cfg: Which recorders to enable and where to write them.


comment: video_cfg: The video recording configuration struct.

alexmillane · 2026-06-16T13:08:18Z

+    are mutually exclusive and size the viewport video.
+
+    Args:
+        env: The env to wrap (already built with ``video_cfg.render_mode`` when ``video`` is set).


-> env: The env to wrap

alexmillane · 2026-06-16T13:09:27Z

Suggestion to move this file into isaaclab_arena/utils folder

- Move video_recording.py to isaaclab_arena/utils/ and drop its module docstring. - Rename VideoRecordingCfg fields and the corresponding CLI flags: --video -> --record_viewport_video, --camera_video -> --record_camera_video, --video_dir -> --video_base_dir. - Write videos into a reverse-dated run subdirectory (timestamped_run_dir), shared across all jobs in an eval run, mirroring Isaac Lab's log layout. - Guard _resolve_video_length when both num_steps and num_episodes are None. - Tidy wrap_env_for_video docstring args. Signed-off-by: alex <amillane@nvidia.com>

alexmillane · 2026-06-16T13:21:16Z

Addressed the review feedback in 0920eb0:

Move to utils/ — video_recording.py moved to isaaclab_arena/utils/; module-level docstring removed.
Field/flag renames — record_viewport_video, record_camera_video, video_base_dir, and the corresponding CLI flags (--video→--record_viewport_video, --camera_video→--record_camera_video, --video_dir→--video_base_dir, incl. dash aliases and the early enable_cameras detection).
Reverse-dated subdir — added timestamped_run_dir(); videos now land in <video_base_dir>/<YYYY-MM-DD_HH-MM-SS>/, computed once per run and shared across all jobs in an eval run (like Isaac Lab logs). Note: with --chunk_size, each chunk subprocess gets its own dated dir since chunks are independent processes.
Docstrings — env: The env to wrap / video_cfg: The video recording configuration struct.
_resolve_video_length guard (greptile P2) — now asserts when both num_steps and num_episodes are None instead of raising a bare TypeError.
Rank log prefix (greptile P2) — left generic for now to keep the wrapper's API surface small.

Use underscore-only flag names (--record_viewport_video, --record_camera_video, --video_base_dir) to match the convention used by every other CLI argument. Signed-off-by: alex <amillane@nvidia.com>

- Move camera_video.py -> video/camera_observation_video_recorder.py. - Move video_recording.py from utils/ -> video/. - Rename the test file to match (test_camera_observation_video_recorder.py). - Update all imports and mock patch targets to the new module paths. Signed-off-by: alex <amillane@nvidia.com>

Signed-off-by: alex <amillane@nvidia.com> # Conflicts: # isaaclab_arena/evaluation/camera_video.py # isaaclab_arena/evaluation/eval_runner.py # isaaclab_arena/evaluation/eval_runner_cli.py # isaaclab_arena/evaluation/policy_runner.py

aiguldzh-nvidia and others added 11 commits June 16, 2026 09:17

split camera recordings per episode instead of one long video

36d49b3

apply pre-commit fixes

cf8e7e3

fix: skip episode counter increment when no frames were written

c70baae

fix: scope camera video filenames per rebuild to avoid overwrites

d41f7a8

fix: cover --camera-video dash alias in pre-sim camera-enable guard

2cb3de9

drop partial episodes on close instead of flushing them

0151399

address code review comments on camera_video.py

bdac7ba

add unit tests for CameraObsVideoRecorder

31694b3

Add pandas as a dep to arena

5cf0543

Merge branch 'alex/fix/missing_pandas_dep' into adzhumamurat/camera_r…

cd12f67

…ecording_per_episode

alexmillane requested review from cvolkcvolk, peterd-NV, qianl-nv, viiik-inside, xyao-nv and zhx06 as code owners June 16, 2026 12:56

greptile-apps Bot reviewed Jun 16, 2026

View reviewed changes

alexmillane commented Jun 16, 2026

View reviewed changes

Drop dash-aliased video flag variants

def4875

Use underscore-only flag names (--record_viewport_video, --record_camera_video, --video_base_dir) to match the convention used by every other CLI argument. Signed-off-by: alex <amillane@nvidia.com>

alexmillane changed the title ~~Centralize rollout env video-recording wrapping~~ Centralize video-recording env wrapping Jun 16, 2026

alexmillane changed the base branch from adzhumamurat/camera_recording_per_episode to main June 16, 2026 14:32

xyao-nv approved these changes Jun 16, 2026

View reviewed changes

Conversation

alexmillane commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Uh oh!

greptile-apps Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps Bot Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

alexmillane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexmillane commented Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexmillane commented Jun 16, 2026 •

edited

Loading

greptile-apps Bot commented Jun 16, 2026 •

edited

Loading