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

Make NormalizeObservation compatible with pybullet #2638

Merged
merged 5 commits into from
Feb 23, 2022

Conversation

vwxyzjn
Copy link
Contributor

@vwxyzjn vwxyzjn commented Feb 22, 2022

This PR follows up with #2531 (comment). Basically the following script errors out:

# pip install pybullet==3.1.8
import gym
import numpy as np
import pybullet_envs

env = gym.make("HopperBulletEnv-v0")
env = gym.wrappers.ClipAction(env)
env = gym.wrappers.NormalizeObservation(env)
env.reset()
pybullet build time: Sep  3 2021 23:59:09
/home/costa/.cache/pypoetry/virtualenvs/cleanrl-ghSZGHE3-py3.9/lib/python3.9/site-packages/gym/spaces/box.py:78: UserWarning: WARN: Box bound precision lowered by casting to float32
  logger.warn(f"Box bound precision lowered by casting to {self.dtype}")
argv[0]=
argv[0]=
Traceback (most recent call last):
  File "/home/costa/Documents/go/src/github.com/cleanrl/cleanrl/test.py", line 14, in <module>
    env.reset()
  File "/home/costa/.cache/pypoetry/virtualenvs/cleanrl-ghSZGHE3-py3.9/lib/python3.9/site-packages/gym/core.py", line 324, in reset
    return self.env.reset(**kwargs)
  File "/home/costa/.cache/pypoetry/virtualenvs/cleanrl-ghSZGHE3-py3.9/lib/python3.9/site-packages/gym/core.py", line 283, in reset
    return self.env.reset(**kwargs)
  File "/home/costa/.cache/pypoetry/virtualenvs/cleanrl-ghSZGHE3-py3.9/lib/python3.9/site-packages/gym/core.py", line 311, in reset
    return self.observation(self.env.reset(**kwargs))
  File "/home/costa/.cache/pypoetry/virtualenvs/cleanrl-ghSZGHE3-py3.9/lib/python3.9/site-packages/gym/wrappers/normalize.py", line 75, in reset
    obs = self.env.reset(seed=seed, options=options)
  File "/home/costa/.cache/pypoetry/virtualenvs/cleanrl-ghSZGHE3-py3.9/lib/python3.9/site-packages/gym/core.py", line 337, in reset
    return self.env.reset(**kwargs)
  File "/home/costa/.cache/pypoetry/virtualenvs/cleanrl-ghSZGHE3-py3.9/lib/python3.9/site-packages/gym/wrappers/time_limit.py", line 26, in reset
    return self.env.reset(**kwargs)
  File "/home/costa/.cache/pypoetry/virtualenvs/cleanrl-ghSZGHE3-py3.9/lib/python3.9/site-packages/gym/wrappers/order_enforcing.py", line 18, in reset
    return self.env.reset(**kwargs)
TypeError: reset() got an unexpected keyword argument 'seed'

And the reason is that

obs, info = self.env.reset(seed=seed, return_info=True, options=options)
forces to use the seed during reset.

This PR fixes this breaking change and makes gym compatible with past gym environments that don't implement env.reset(seed=seed)

@vwxyzjn
Copy link
Contributor Author

vwxyzjn commented Feb 22, 2022

Hey @RedTachyon would you mind helping review this?

@RedTachyon
Copy link
Contributor

It seems kinda contrived in the way it's implemented? I'm also not confident it will actually normalize stuff, in lines 74/76 you're basically just overwriting the variable obs, so I think output will be returned unchanged.

What I'd suggest doing instead is doing the same thing as it was in the previous version (although with the current signature), except you do something like

return_info = kwargs.get("return_info", False)
if return_info:
    obs, info = env.reset(**kwargs)
else:
    obs = env.reset(**kwargs)
...

Does that make sense?

@vwxyzjn
Copy link
Contributor Author

vwxyzjn commented Feb 22, 2022

Hey @RedTachyon, the current implementation basically uses reference, but I think your proposed approach is cleaner. I have incorporated your suggestion.

@RedTachyon
Copy link
Contributor

Looks good now

@jkterry1 jkterry1 merged commit c8321e6 into openai:master Feb 23, 2022
@vwxyzjn vwxyzjn deleted the hot-fix-normalize branch February 25, 2022 01:17
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.

3 participants