Forward environment properties to the wrapper

[tristandeleu]

• Do not set the wrapper properties in __init__, but forward the environment properties by default.
• Fixes Base class Wrapper(env) does not allow action_space updates from within original environment #2175

class CustomEnv(gym.Env):
    observation_space = None
    action_space = None

    def reset(self):
        self.observation_space = gym.spaces.Box(low=0., high=1., shape=(2,))
        self.action_space = gym.spaces.Discrete(2)
        return self.observation_space.sample()

    def step(self, action):
        return (self.observation_space.sample(), 0., False, {})

class CustomWrapper(gym.Wrapper):
    pass

env = CustomWrapper(CustomEnv())
env.reset()
print(f'Action space: {env.action_space}')
# Before: Action space: None
# After: Action space: Discrete(2)

[jkterry1]

@tristandeleu can you add tests? Otherwise I believe this looks good.

[tristandeleu]

What is the status of this PR?

[RedTachyon]

Overall this looks solid and solves what it's supposed to, just one minor doubt about a variable name (see review)

One thing I'm thinking about now is whether it would be useful for wrappers to forward all properties' getters by default (which would also solve this issue as a special case). This way if someone has e.g. env.current_state defined, it would be forwarded through all the wrappers. But it'd probably require too much effort, with the only benefit being sometimes not having to write .unwrapped

[RedTachyon]

I'm assuming it's intended that the setter only affects the wrapper, and does not get propagated to the inner env. I'm not sure if this makes any difference in the end, but I'm also not fully immediately convinced which option is better? Is there any use case in which either option is better?

[tristandeleu]

I don't think mutating observation_space/action_space in the inner envs is a good idea. There may be cases where you'd like to have access to the space of the original environment (e.g. through env.unwrapped.action_space). If the setter was propagated to the inner env, then you'd always have env.action_space == env.unwrapped.action_space.

[RedTachyon]

Minor nitpick - is klass often used instead of class? I'd intuitively go rather for cls

[tristandeleu]

By convention, cls is reserved for classmethods. PEP8 suggests using class_ instead, but klass is also a fairly common name, even CPython uses it.

[tristandeleu]

One thing I'm thinking about now is whether it would be useful for wrappers to forward all properties' getters by default (which would also solve this issue as a special case). This way if someone has e.g. env.current_state defined, it would be forwarded through all the wrappers. But it'd probably require too much effort, with the only benefit being sometimes not having to write .unwrapped

That's already the case: #1347

class CustomEnv(gym.Env):
    g = 9.81

class CustomWrapper(gym.Wrapper):
    pass

env = CustomWrapper(CustomEnv())
print(env.g)  # 9.81
env.g = 1.62
print(env.g)  # 1.62

The reason why we can't just rely on __getattr___ for observation_space, action_space, reward_range and metadata is that since Wrapper inherits from Env, those properties will get set in every wrapper, and so forwarding will not work properly: for example, if you don't set observation_space in your wrapper, because you'd like it to be the same as the wrapped environment, then you'd always get wrapped_env.observation_space == None.