Deprecating the Monitor in favor of RecordEpisodeStatistics and RecordVideo

[vwxyzjn]

Kicking off a discussion thread here. Currently, the Monitor has two features: recording videos via the video_recorder.py and recording stats via the stats_recorder.py

Issues

• The stats_recorder.py does not allow the environment to reset if it's not done (see Gym Wrapper not letting me call env.reset() #599). In my experience, this causes some confusions and can be a pain to deal with, especially when the users want to do something custom.
• Because the way the Monitor is setup, it can't be easily applied to vectorized env directly.
• Monitor also has some legacy stuff such as mode (['evaluation', 'training']) and an info to use gym.upload. I am also not sure how to use the resume argument.

Potential resolution

I feel the Monitor class is doing too many things at once, and it will be a good idea to deprecating the Monitor in favor of RecordEpisodeStatistics and a newly-created VideoRecorder.

• Note the stats_recorder.py has duplicate functionality as RecordEpisodeStatistics. We can add the stats saving functionality to RecordEpisodeStatistics if desired.

[vwxyzjn]

If folks are happy with this proposal, I am happy to prepare PRs. This issue is also related to #2279

[vwxyzjn]

How do you feel about this PR @tristandeleu? Would love to get your thoughts!

[tristandeleu]

It's been a while since I've last used Monitor, but I like the idea of breaking down Monitor into RecordEpisodeStatistics and VideoRecorder for specialized use cases.

The big issue with making Monitor/VideoRecorder compatible with VectorEnv is that VideoRecorder requires a valid render() method from the environment, which is currently not implemented for VectorEnv. There is this PR #1624 which added render to VectorEnv, but this is not straightforward (see this comment #1624 (comment)).

To add to what that comment said, one thing that could be a bottleneck is the transfer of images rendered in individual environments with multiprocessing in AsyncVectorEnv back to the main process, which may slow down the environment even further. One solution could be to use a shared memory (the same way shared memory is already used by default in AsyncVectorEnv for observations). The problem is that you don't know ahead of time (i.e. when you instantiate the VectorEnv) what is the size of the images rendered by individual environments (unlike the observations, where you have the observation_space) to allocate the appropriate amount of shared memory. A workaround is not impossible, but it would probably require some work.

[jkterry1]

Hey, I just sat down to look at this and your PR in detail:

-I am in favor of making the breaking change here; please create PRs for all of this if you're willing to
-My vote would be adding these changes now, then adding the relevant vector environment support in the future after the breaking changes are made to the vector environment API. For reference on a timeline for that, I don't want to start dealing with that until ALE-Py and Lycon2 are merged and dealt with. That hopefully should be happening this week or so; I'm currently waiting on the maintainers of the respective libraries to cut releases.