Skip to content

Evaluation report fixing non existing video recordings - #865

Merged
cvolkcvolk merged 2 commits into
mainfrom
cvolk/fix/report-jsonl-episodes
Jul 6, 2026
Merged

Evaluation report fixing non existing video recordings#865
cvolkcvolk merged 2 commits into
mainfrom
cvolk/fix/report-jsonl-episodes

Conversation

@cvolkcvolk

@cvolkcvolk cvolkcvolk commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Evaluation reports were written empty when camera video recording --record_camera_video was disabled:

Wrote evaluation report with 0 job(s) and 0 episode(s) to: /workspaces/isaaclab_arena/output/camera_sensitivity/2026-07-06_08-13-26/index.html

The report now combines JSONL records and videos, with video recording being optional

Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Signed-off-by: Clemens Volk <cvolk@nvidia.com>
@cvolkcvolk
cvolkcvolk marked this pull request as ready for review July 6, 2026 08:34
@arena-review-bot

Copy link
Copy Markdown
Contributor

🤖 Isaac Lab-Arena Review Bot

Summary

This fixes a real bug: when --record_camera_video is off, the report scanned only mp4s and came out empty even though JSONL results existed. The fix now unions job/env/episode keys from both the results (_scan_results) and the videos (raw), so result-only episodes render with their status and metadata and empty video cells. I traced the merge logic and it is correct — episodes with both a video and a matching result collapse onto the shared (rebuild, recorder_episode) key (no double-counting), and cameras_by_job.get(job, []) correctly handles jobs that have results but no cameras. The new [WARNING]/[INFO] prints are a nice touch for the empty / video-less cases.

Findings

🟡 Warning: isaaclab_arena/visualization/report.py — This is a bug fix (empty report without videos) but ships no regression test. The module is pure Python with no sim dependency, so it's cheap to cover: a Phase-1 test that writes a temp dir containing only episode_results_rebuild0.jsonl (no mp4s), calls build_report, and asserts the resulting EvaluationReport has the expected job(s)/episode(s) would fail before this change and pass after. Worth adding so the result-only path doesn't silently regress. (Noting there are currently no tests for this module at all, so this would be the first.)

🔵 Improvement: --video_dir arg help — Not on a changed line, but now that result-only runs are supported, the flag help ("Folder of recorded rollout videos to scan…") reads as video-only. Could we tweak the wording to mention it also picks up result-only runs, so a user without videos knows they can still point at it?

Test Coverage

No tests exist for report.py. Since the whole module is sim-agnostic, a small Phase-1 (not with_cameras and not with_subprocess) test exercising the results-only and mixed cases would lock in this fix — no inner/outer run_simulation_app_function pattern needed here.

Verdict

Minor fixes needed — the fix itself is correct and well-scoped; the main gap is a regression test for the result-only path.

@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes evaluation reports being written with 0 jobs and 0 episodes when --record_camera_video is disabled. The root cause was that _scan_jobs only iterated over jobs/envs/episodes found in the MP4 scan (raw), ignoring JSONL-only entries entirely.

  • _scan_jobs now unions MP4-based and JSONL-based job/env/episode keys at every level, with safe .get(..., {}) / .get(..., []) fallbacks so result-only jobs produce correct EpisodeVideos entries with an empty video_by_camera.
  • build_report gains two informational prints: a [WARNING] when the report is fully empty, and an [INFO] when episodes were found but no videos were recorded.

Confidence Score: 4/5

The change is safe to merge; it corrects a clear gap where result-only runs produced empty reports, and all the union/fallback logic is straightforward.

The union logic at the job, env, and episode levels is implemented correctly with safe fallbacks at each layer. The only gap is that the EpisodeVideos and JobReport docstrings were not updated to reflect that videos are now optional, which could mislead future contributors.

isaaclab_arena/visualization/report.py — specifically the EpisodeVideos and JobReport class docstrings, which still describe video-centric semantics.

Important Files Changed

Filename Overview
isaaclab_arena/visualization/report.py Core logic fix: _scan_jobs now unions video-only and result-only jobs/envs/episodes, with correct fallbacks for cameras_by_job and video_recordings; minor docstring staleness on EpisodeVideos and JobReport.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[_scan_jobs] --> B[_scan_results: load JSONL]
    A --> C[Glob *.mp4: load videos]
    B --> D[Union jobs from raw and results]
    C --> D
    D --> E[For each job]
    E --> F[Build result_keys_by_env from JSONL]
    E --> G[video_envs from raw]
    F --> H[Union env indices]
    G --> H
    H --> I[For each env_index]
    I --> J[Union recording keys from videos and JSONL]
    J --> K[For each recording_key]
    K --> L[record from JSONL - empty dict if missing]
    K --> M[video_by_camera from raw - empty dict if missing]
    L --> N[Append EpisodeVideos]
    M --> N
    N --> O[JobReport with cameras - empty list if no videos]
Loading
%%{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"}}}%%
flowchart TD
    A[_scan_jobs] --> B[_scan_results: load JSONL]
    A --> C[Glob *.mp4: load videos]
    B --> D[Union jobs from raw and results]
    C --> D
    D --> E[For each job]
    E --> F[Build result_keys_by_env from JSONL]
    E --> G[video_envs from raw]
    F --> H[Union env indices]
    G --> H
    H --> I[For each env_index]
    I --> J[Union recording keys from videos and JSONL]
    J --> K[For each recording_key]
    K --> L[record from JSONL - empty dict if missing]
    K --> M[video_by_camera from raw - empty dict if missing]
    L --> N[Append EpisodeVideos]
    M --> N
    N --> O[JobReport with cameras - empty list if no videos]
Loading

Comments Outside Diff (1)

  1. isaaclab_arena/visualization/report.py, line 42-70 (link)

    P2 Stale class docstrings after the video-optional change

    EpisodeVideos is documented as "The recorded camera videos for a single (env, episode)" and JobReport as "All recorded episode videos for a single eval job", but both now represent result-only cases where video_by_camera is empty. Readers unfamiliar with the change will incorrectly assume a video is always present, which matters especially for EpisodeVideos.video_by_camera (whose field doc says "Camera name -> mp4 path … relative to the scanned root" with no mention of the empty-dict case).

    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!

Reviews (1): Last reviewed commit: "Clarify reports without rollout videos" | Re-trigger Greptile

@cvolkcvolk cvolkcvolk changed the title Include result-only episodes in evaluation reports Evaluation report fixing non existing video recordings Jul 6, 2026

@alexmillane alexmillane left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank!

@cvolkcvolk
cvolkcvolk merged commit 4c86e95 into main Jul 6, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants