fix: remove auto close feature in VideoRecorder (#42)

gymnasium/wrappers/monitoring/video_recorder.py

@@ -24,6 +24,7 @@ def __init__(
         metadata: Optional[dict] = None,
         enabled: bool = True,
         base_path: Optional[str] = None,
+        disable_logger: bool = False,
     ):
         """Video recorder renders a nice movie of a rollout, frame by frame.
 
@@ -33,6 +34,7 @@ def __init__(
             metadata (Optional[dict]): Contents to save to the metadata file.
             enabled (bool): Whether to actually record video, or just no-op (for convenience)
             base_path (Optional[str]): Alternatively, path to the video file without extension, which will be added.
+            disable_logger (bool): Whether to disable moviepy logger or not.
 
         Raises:
             Error: You can pass at most one of `path` or `base_path`
@@ -48,6 +50,7 @@ def __init__(
 
         self._async = env.metadata.get("semantics.async")
         self.enabled = enabled
+        self.disable_logger = disable_logger
         self._closed = False
 
         self.render_history = []
@@ -153,9 +156,9 @@ def close(self):
                     "MoviePy is not installed, run `pip install moviepy`"
                 )
 
-            logger.debug(f"Closing video encoder: path={self.path}")
             clip = ImageSequenceClip(self.recorded_frames, fps=self.frames_per_sec)
-            clip.write_videofile(self.path)
+            moviepy_logger = None if self.disable_logger else "bar"
+            clip.write_videofile(self.path, logger=moviepy_logger)
         else:
             # No frames captured. Set metadata.
             if self.metadata is None:
@@ -175,4 +178,5 @@ def write_metadata(self):
     def __del__(self):
         """Closes the environment correctly when the recorder is deleted."""
         # Make sure we've closed up shop when garbage collecting
-        self.close()
+        if not self._closed:
+            logger.warn("Unable to save last video! Did you call close()?")

gymnasium/wrappers/record_video.py

@@ -45,6 +45,7 @@ def __init__(
         step_trigger: Callable[[int], bool] = None,
         video_length: int = 0,
         name_prefix: str = "rl-video",
+        disable_logger: bool = False,
     ):
         """Wrapper records videos of rollouts.
 
@@ -56,6 +57,8 @@ def __init__(
             video_length (int): The length of recorded episodes. If 0, entire episodes are recorded.
                 Otherwise, snippets of the specified length are captured
             name_prefix (str): Will be prepended to the filename of the recordings
+            disable_logger (bool): Whether to disable moviepy logger or not.
+
         """
         super().__init__(env)
 
@@ -68,6 +71,7 @@ def __init__(
         self.episode_trigger = episode_trigger
         self.step_trigger = step_trigger
         self.video_recorder: Optional[video_recorder.VideoRecorder] = None
+        self.disable_logger = disable_logger
 
         self.video_folder = os.path.abspath(video_folder)
         # Create output folder if needed
@@ -119,6 +123,7 @@ def start_video_recorder(self):
             env=self.env,
             base_path=base_path,
             metadata={"step_id": self.step_id, "episode_id": self.episode_id},
+            disable_logger=self.disable_logger,
         )
 
         self.video_recorder.capture_frame()
@@ -205,7 +210,3 @@ def close(self):
         """Closes the wrapper then the video recorder."""
         super().close()
         self.close_video_recorder()
-
-    def __del__(self):
-        """Closes the video recorder."""
-        self.close_video_recorder()

tests/wrappers/test_video_recorder.py

@@ -1,7 +1,5 @@
-import gc
 import os
 import re
-import time
 
 import pytest
 
@@ -45,31 +43,6 @@ def test_record_simple():
     assert os.fstat(f.fileno()).st_size > 100
 
 
-def test_autoclose():
-    def record():
-        env = gym.make(
-            "CartPole-v1", render_mode="rgb_array_list", disable_env_checker=True
-        )
-        rec = VideoRecorder(env)
-        env.reset()
-        rec.capture_frame()
-
-        rec_path = rec.path
-
-        # The function ends without an explicit `rec.close()` call
-        # The Python interpreter will implicitly do `del rec` on garbage cleaning
-        return rec_path
-
-    rec_path = record()
-
-    gc.collect()  # do explicit garbage collection for test
-    time.sleep(5)  # wait for subprocess exiting
-
-    assert os.path.exists(rec_path)
-    f = open(rec_path)
-    assert os.fstat(f.fileno()).st_size > 100
-
-
 def test_no_frames():
     env = BrokenRecordableEnv()
     rec = VideoRecorder(env)