Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove auto close feature in VideoRecorder #42

Merged
merged 6 commits into from
Nov 1, 2022

Conversation

younik
Copy link
Member

@younik younik commented Oct 8, 2022

If user forgets to call close, and the VideoRecorder is deleted by the garbage collector, the class tries to save the video, but it raises an error (and it is not easy to understand the reason). With this PR, RecordVideo just warns the user that the last video is not saved and to call close.

It also adds a parameter disable_logger to disable MoviePy messages.
Screenshot from 2022-10-08 13-33-51

@pseudo-rnd-thoughts
Copy link
Member

pseudo-rnd-thoughts commented Oct 8, 2022

Do we want this as a debug message when a video is saved as depending on the video length then it can take some time I believe?

@younik
Copy link
Member Author

younik commented Oct 9, 2022

Do we want this a debug message when a video is saved as depending on the video length then can take some time I believe?

Good point; on my laptop, it takes ~5 seconds for 1000 frames.

There is already line 156:

logger.debug(f"Closing video encoder: path={self.path}")

I can add a Done! message after line 158.
Alternatively, we can set a threshold on the number of frames to turn on logger.

@pseudo-rnd-thoughts
Copy link
Member

We could do with a parameter disable_logger: bool = False? This should allow people to disable if they want but it is enabled by default

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

Is it possible to save multiple videos?

@younik
Copy link
Member Author

younik commented Oct 25, 2022

Is it possible to save multiple videos?

The close method of VideoRecorder is called every time the recording of the current video has finished and the video is saved. This can happen multiple times (depending on the parameters of RecordVideo). Does this answer your question?

@pseudo-rnd-thoughts
Copy link
Member

Yes, but I dislike the answer in the sense that to me close should only be called once. But I guess users can now use the function version of the wrappers for cases like that if they care

@younik
Copy link
Member Author

younik commented Oct 25, 2022

Yes, but I dislike the answer in the sense that to me close should only be called once. But I guess users can now use the function version of the wrappers for cases like that if they care

Yes, I agree; we decided to keep VideoRecorder after discussion here: openai/gym#2894
It is also used by wandb

@younik younik changed the title ENH: remove moviepy logger fix: remove auto close feature in VideoRecorder Oct 25, 2022
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