refactor(recording): make filename and required_context required

The previous defaults (`filename="{task_name}_ep{episode_idx}_{status}.mp4"`
and `required_context=("episode_idx",)`) were a footgun:

- `task_name` is not a universal key. RoboMME exposes `env_id`, LIBERO
  uses `suite`/`task_id`, SimplerEnv uses `task` — the default name
  doesn't fit any of them out of the box.
- The default `required_context` only covered one of the two keys the
  default template referenced. A caller passing `episode_idx` but
  forgetting `task_name` would pass `start()` validation, succeed at
  every `record()`, then silently lose the mp4 at `save()` when
  `str.format` raised `KeyError`.

Make both required so each benchmark spells out its naming scheme and
its context contract once at construction. Mismatches surface at
`start()` rather than as a dropped mp4 at the end of the episode.

RoboMME wiring updated to pass `{env_id}_ep{episode_idx}_{status}.mp4`
with `required_context=("env_id", "episode_idx")`.

Author: claude

src/vla_eval/benchmarks/recording.py

@@ -21,9 +21,11 @@
 * **Logging-style filename templating**: the filename is a ``str.format``
   template (or callable) over a context dict that the caller passes at
   ``start()`` time, plus a ``status`` key injected at ``save()`` time.
-  Default template ``"{task_name}_ep{episode_idx}_{status}.mp4"``;
-  benchmarks with richer identifiers can use any context they like, e.g.
-  ``"{suite}/{task}/{episode_idx:04d}_seed{seed}_{status}.mp4"``.
+  ``filename`` and ``required_context`` are required at construction time
+  — there is no universal default because every benchmark names tasks
+  differently (``env_id``, ``task_id``, ``suite/task`` …).  Forcing the
+  caller to spell it out catches mismatched context keys at ``start()``
+  rather than as a silent dropped mp4 at ``save()``.
 * **Best-effort**: every encode-side failure logs a warning with the
   context for debuggability and clears state — a corrupted video should
   never bring down an otherwise good eval episode.
@@ -34,13 +36,13 @@
 
     recorder = EpisodeVideoRecorder(
         output_dir="/workspace/results/videos",
-        # filename can stay default, or e.g.
-        # filename="{suite}/{task}_ep{episode_idx}_{status}.mp4",
+        filename="{env_id}_ep{episode_idx}_{status}.mp4",
+        required_context=("env_id", "episode_idx"),
         fps=20,
     )
 
     # In benchmark.reset(task):
-    recorder.start({"task_name": task["env_id"], "episode_idx": task["episode_idx"]})
+    recorder.start({"env_id": task["env_id"], "episode_idx": task["episode_idx"]})
     recorder.record(initial_frame)
 
     # In benchmark.step(action):
@@ -100,9 +102,9 @@ class EpisodeVideoRecorder:
     def __init__(
         self,
         output_dir: str | os.PathLike[str],
-        filename: FilenameSpec = "{task_name}_ep{episode_idx}_{status}.mp4",
+        filename: FilenameSpec,
+        required_context: Sequence[str],
         fps: int = 20,
-        required_context: Sequence[str] = ("episode_idx",),
         writer_kwargs: Mapping[str, Any] | None = None,
     ) -> None:
         """
@@ -112,13 +114,17 @@ def __init__(
                 ``"{suite}/{task}_..."``); intermediate dirs are also created.
             filename: ``str.format`` template or callable producing the
                 filename relative to ``output_dir``.  Resolved at ``save()``
-                time over ``{**start_context, "status": status}``.
-            fps: Output framerate.
+                time over ``{**start_context, "status": status}``.  Required
+                because every benchmark identifies tasks differently
+                (``env_id``, ``task_id``, ``suite/task``) — there is no
+                universally safe default.
             required_context: Keys that must be present in the dict passed to
                 ``start()``.  ``ValueError`` is raised at ``start()`` if any
-                are missing.  Default ``("episode_idx",)`` because without an
-                episode index, multi-episode runs of the same task collide
-                on a single ``_ep0_`` filename.
+                are missing.  Required so callers explicitly declare the
+                template's expectations; failing fast at ``start()`` avoids
+                a silent ``KeyError`` -> dropped mp4 at ``save()`` time.
+                Should include every key the ``filename`` template references.
+            fps: Output framerate.
             writer_kwargs: Extra kwargs forwarded to ``imageio.get_writer``
                 (e.g. ``{"codec": "libx264", "quality": 8}``).
         """

src/vla_eval/benchmarks/robomme/benchmark.py

@@ -193,7 +193,12 @@ def __init__(
         self.subgoal_mode = subgoal_mode
 
         self._recorder: EpisodeVideoRecorder | None = (
-            EpisodeVideoRecorder(output_dir=video_dir or "/workspace/results/videos", fps=20)
+            EpisodeVideoRecorder(
+                output_dir=video_dir or "/workspace/results/videos",
+                filename="{env_id}_ep{episode_idx}_{status}.mp4",
+                required_context=("env_id", "episode_idx"),
+                fps=20,
+            )
             if save_episode_video
             else None
         )
@@ -385,8 +390,12 @@ def reset(self, task: Task) -> Any:
             self._current_subgoal = self._extract_subgoal(info_flat)
 
         if self._recorder is not None:
-            task_name = task.get("name") or task.get("env_id", "unknown")
-            self._recorder.start({"task_name": task_name, "episode_idx": task.get("episode_idx", 0)})
+            self._recorder.start(
+                {
+                    "env_id": task.get("env_id") or task.get("name") or "unknown",
+                    "episode_idx": task.get("episode_idx", 0),
+                }
+            )
             front_list = obs_batch.get("front_rgb_list", [])
             if front_list:
                 self._recorder.record(front_list[-1])

tests/test_recording.py

@@ -37,13 +37,31 @@ def _count_frames(path: Path) -> int:
         return sum(1 for _ in r)  # ty: ignore[not-iterable]
 
 
+def _rec(tmp_path: Path, **overrides: Any) -> EpisodeVideoRecorder:
+    """Construct a recorder with stable test-suite defaults.
+
+    The recorder itself has no defaults for ``filename`` /
+    ``required_context`` (every benchmark spells them out explicitly).
+    Tests don't need to repeat that boilerplate, so this helper picks
+    the same ``task_name``/``episode_idx`` template the original
+    test data assumed.
+    """
+    kwargs: dict[str, Any] = {
+        "output_dir": tmp_path,
+        "filename": "{task_name}_ep{episode_idx}_{status}.mp4",
+        "required_context": ("task_name", "episode_idx"),
+    }
+    kwargs.update(overrides)
+    return EpisodeVideoRecorder(**kwargs)
+
+
 # ---------------------------------------------------------------------------
 # Happy path
 # ---------------------------------------------------------------------------
 
 
 def test_save_writes_mp4_with_correct_framecount(tmp_path: Path) -> None:
-    rec = EpisodeVideoRecorder(output_dir=tmp_path, fps=10)
+    rec = _rec(tmp_path, fps=10)
     rec.start({"task_name": "PickCube", "episode_idx": 0})
     for _ in range(5):
         rec.record(_frame())
@@ -56,7 +74,7 @@ def test_save_writes_mp4_with_correct_framecount(tmp_path: Path) -> None:
 
 
 def test_save_uses_status_in_filename(tmp_path: Path) -> None:
-    rec = EpisodeVideoRecorder(output_dir=tmp_path)
+    rec = _rec(tmp_path)
     rec.start({"task_name": "T", "episode_idx": 7})
     rec.record(_frame())
     final = rec.save(status="fail")
@@ -66,7 +84,7 @@ def test_save_uses_status_in_filename(tmp_path: Path) -> None:
 
 
 def test_active_flag_tracks_lifecycle(tmp_path: Path) -> None:
-    rec = EpisodeVideoRecorder(output_dir=tmp_path)
+    rec = _rec(tmp_path)
     assert rec.active is False
     rec.start({"task_name": "T", "episode_idx": 0})
     assert rec.active is True
@@ -75,7 +93,7 @@ def test_active_flag_tracks_lifecycle(tmp_path: Path) -> None:
 
 
 def test_consecutive_episodes_each_produce_their_own_file(tmp_path: Path) -> None:
-    rec = EpisodeVideoRecorder(output_dir=tmp_path)
+    rec = _rec(tmp_path)
     for ep in range(3):
         rec.start({"task_name": "T", "episode_idx": ep})
         rec.record(_frame())
@@ -125,18 +143,20 @@ def naming(ctx: Mapping[str, Any]) -> str:
     assert final == tmp_path / "abc-3-ok.mp4"
 
 
-def test_save_with_missing_template_key_is_handled(tmp_path: Path) -> None:
+def test_save_with_template_key_not_in_required_context_is_handled(tmp_path: Path) -> None:
+    # required_context is the caller's contract for what must be present at
+    # start(); it's permitted to be a subset of the keys the template uses
+    # (e.g. an optional `seed`).  When a template key is genuinely missing
+    # at save() time, resolution should fail gracefully rather than raise.
     rec = EpisodeVideoRecorder(
         output_dir=tmp_path,
         filename="{task_name}_{seed}_{status}.mp4",
+        required_context=("task_name",),
     )
-    rec.start({"task_name": "T", "episode_idx": 0})  # `seed` missing
+    rec.start({"task_name": "T"})  # `seed` missing
     rec.record(_frame())
-    # Resolution happens at save() time; a missing key logs and returns None
-    # rather than raising.
     final = rec.save(status="success")
     assert final is None
-    # Tempfile must have been cleaned up.
     assert list(tmp_path.glob(".recorder-*.mp4")) == []
 
 
@@ -146,31 +166,31 @@ def test_save_with_missing_template_key_is_handled(tmp_path: Path) -> None:
 
 
 def test_start_missing_required_context_raises(tmp_path: Path) -> None:
-    rec = EpisodeVideoRecorder(output_dir=tmp_path)
+    rec = _rec(tmp_path)
     with pytest.raises(ValueError, match="missing required context keys"):
         rec.start({"task_name": "T"})  # episode_idx missing
     assert rec.active is False
 
 
 def test_record_before_start_is_noop(tmp_path: Path) -> None:
-    rec = EpisodeVideoRecorder(output_dir=tmp_path)
+    rec = _rec(tmp_path)
     rec.record(_frame())  # must not raise
     assert rec.active is False
 
 
 def test_save_before_start_returns_none(tmp_path: Path) -> None:
-    rec = EpisodeVideoRecorder(output_dir=tmp_path)
+    rec = _rec(tmp_path)
     assert rec.save() is None
 
 
 def test_discard_before_start_is_noop(tmp_path: Path) -> None:
-    rec = EpisodeVideoRecorder(output_dir=tmp_path)
+    rec = _rec(tmp_path)
     rec.discard()  # must not raise
     assert rec.active is False
 
 
 def test_writer_open_failure_leaves_recorder_inactive(tmp_path: Path) -> None:
-    rec = EpisodeVideoRecorder(output_dir=tmp_path)
+    rec = _rec(tmp_path)
     with patch("imageio.get_writer", side_effect=RuntimeError("nope")):
         rec.start({"task_name": "T", "episode_idx": 0})
     assert rec.active is False
@@ -187,13 +207,14 @@ def test_writer_open_failure_leaves_recorder_inactive(tmp_path: Path) -> None:
 
 
 def test_start_again_without_save_discards_prior_episode(tmp_path: Path) -> None:
-    rec = EpisodeVideoRecorder(output_dir=tmp_path)
+    rec = _rec(tmp_path)
     rec.start({"task_name": "T", "episode_idx": 0})
     rec.record(_frame())
     # Simulate orchestrator skipping save() / discard() and starting next ep:
     rec.start({"task_name": "T", "episode_idx": 1})
     rec.record(_frame())
     final = rec.save(status="success")
+    assert final is not None
     assert final == tmp_path / "T_ep1_success.mp4"
     # Only ep1 mp4 should exist; ep0's tempfile was cleaned up.
     mp4s = sorted(p.name for p in tmp_path.glob("*.mp4"))
@@ -202,7 +223,7 @@ def test_start_again_without_save_discards_prior_episode(tmp_path: Path) -> None
 
 
 def test_discard_cleans_up_tempfile(tmp_path: Path) -> None:
-    rec = EpisodeVideoRecorder(output_dir=tmp_path)
+    rec = _rec(tmp_path)
     rec.start({"task_name": "T", "episode_idx": 0})
     rec.record(_frame())
     rec.discard()
@@ -219,7 +240,7 @@ def test_discard_cleans_up_tempfile(tmp_path: Path) -> None:
 def test_output_dir_created_lazily(tmp_path: Path) -> None:
     target = tmp_path / "nested" / "videos"
     assert not target.exists()
-    rec = EpisodeVideoRecorder(output_dir=target)
+    rec = _rec(target)
     rec.start({"task_name": "T", "episode_idx": 0})
     rec.record(_frame())
     final = rec.save()
@@ -229,7 +250,7 @@ def test_output_dir_created_lazily(tmp_path: Path) -> None:
 
 
 def test_str_path_accepted(tmp_path: Path) -> None:
-    rec = EpisodeVideoRecorder(output_dir=str(tmp_path))
+    rec = _rec(tmp_path, output_dir=str(tmp_path))
     rec.start({"task_name": "T", "episode_idx": 0})
     rec.record(_frame())
     final = rec.save()