-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
Make RecordEpisodeStatistics work with VectorEnv #2296
Make RecordEpisodeStatistics work with VectorEnv #2296
Conversation
Could you please add tests before I merge? |
@jkterry1 Could you approve workflows? |
Hey @jkterry1 can you approve the workflows again |
self.episode_length = 0 | ||
return observation | ||
observations = super(RecordEpisodeStatistics, self).reset(**kwargs) | ||
self.episode_returns = np.zeros(self.num_envs, dtype=np.float32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.num_envs
is not defined here if env
is a VectorEnv
instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t follow. VectorEnv
has a num_envs
attribute, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right the wrapper inherits the properties from env
, sorry my bad!
@pytest.mark.parametrize("env_id", ["CartPole-v0"]) | ||
def test_record_episode_statistics_with_vectorenv(env_id): | ||
envs = gym.vector.make(env_id, asynchronous=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the corresponding imports
from gym.vector import AsyncVectorEnv, SyncVectorEnv
from gym.vector.tests.utils import make_env
@pytest.mark.parametrize("env_id", ["CartPole-v0"]) | |
def test_record_episode_statistics_with_vectorenv(env_id): | |
envs = gym.vector.make(env_id, asynchronous=False) | |
@pytest.mark.parametrize("klass", [SyncVectorEnv, AsyncVectorEnv]) | |
@pytest.mark.parametrize("num_envs", [1, 4]) | |
def test_record_episode_statistics_with_vectorenv(klass, num_envs): | |
env_fns = [make_env("CartPole-v0", i) for i in range(num_envs)] | |
envs = klass(env_fns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it’s gonna fail with AsyncVectorEnv
because the envs.env.envs[0].spec.max_episode_steps
is inaccessible. Maybe I should just hardcore a 201 instead of envs.env.envs[0].spec.max_episode_steps
? Do we really need the test case with AsyncVectorEnv
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry I didn't see that you were looping over that later. Then you can ignore this (maybe keeping the parametrization for num_envs
?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Would you mind Allowing the GitHub action workflow runs? I have some weird setup That makes it difficult to run test cases locally….
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Tristan Deleu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @tristandeleu so much for the review. I just have one comment regarding the test cases.
@pytest.mark.parametrize("env_id", ["CartPole-v0"]) | ||
def test_record_episode_statistics_with_vectorenv(env_id): | ||
envs = gym.vector.make(env_id, asynchronous=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it’s gonna fail with AsyncVectorEnv
because the envs.env.envs[0].spec.max_episode_steps
is inaccessible. Maybe I should just hardcore a 201 instead of envs.env.envs[0].spec.max_episode_steps
? Do we really need the test case with AsyncVectorEnv
?
self.episode_length = 0 | ||
return observation | ||
observations = super(RecordEpisodeStatistics, self).reset(**kwargs) | ||
self.episode_returns = np.zeros(self.num_envs, dtype=np.float32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t follow. VectorEnv
has a num_envs
attribute, right?
…s://github.com/vwxyzjn/gym into make-RecordEpisodeStatistics-work-with-vec-env
* Make RecordEpisodeStatistics work with VectorEnv * fix test cases * fix lint * add test cases * fix linting * fix tests * fix test cases... * Update gym/wrappers/record_episode_statistics.py Co-authored-by: Tristan Deleu <[email protected]> * fix test cases * fix test cases again Co-authored-by: Tristan Deleu <[email protected]>
Following up with #2279, this PR makes
RecordEpisodeStatistics
work withVectorEnv
as well. That iswill produce the same results as
The reason why this PR is important is that some envs don't allow you to insert a
RecordEpisodeStatistics
in themake_env
function. The procgen environment is one such example. So this PR will allow you to do something like this: