Add a complete compatibility wrapper #3066
Conversation
RedTachyon
changed the title
| assert env.observation_space == Discrete(1) | ||
| assert env.action_space == Discrete(1) | ||
| assert env.reset() == (0, {}) | ||
| assert env.reset(seed=0, options={"some": "option"}) == (0, {}) |
There was a problem hiding this comment.
We don't have a test that reset actual calls seed correctly but Im not sure that matters
| @@ -146,7 +140,6 @@ def test_apply_step_compatibility(): | |||
| max_episode_steps=3, | |||
| ) | |||
| env = gym.make("testing-old-env") | |||
| assert has_wrapper(env, StepAPICompatibility) | |||
There was a problem hiding this comment.
Why is this assert removed?
There was a problem hiding this comment.
The old semantic for the compatibility layer was that it's a valid gym.Wrapper, because a gym.Env could be both in the old and new API. Now for an env to be a valid gym.Env, it has to follow the new API, which means that something written with the old API, isn't technically a gym.Env, i.e. it can't be the input to a gym.Wrapper.
The new compatibility layer is simply an implementation of a gym.Env, which happens to have a somewhat similar structure to a wrapper, but isn't one. So it used to be the case that an old-to-new environment is wrapped by StepAPICompatibility as a wrapper, which was checked in the test. Now that's not the case anymore.
There was a problem hiding this comment.
Should the environment not come before the passive environment checker?
Changed it to |
|
I talked to @younik about auto human rendering today (i.e. not having to call |
|
I put the compatibility layer before the env checker, I agree it makes more sense this way. I think? I feel like some sort of an acknowledgment of "You shouldn't be doing this" is warranted (which would have been thrown up by the env checker), but at the same time, you have to enable this explicitly, so you know what you're doing. About the rendering - I'm a bit out of loop with the details of those changes, as I understand we just want if self.render_mode == "human":
self.render()inside |
|
Yes, I think that's essentially right, though we would also want to add this to reset (and correspondingly, it should be at the very end of the step method, which it currently is :)). |
|
In the last commit I added some logic to allow doing |
|
(for anyone in the future wondering what's going on - I added an unrelated fix for #3068) |
|
LGTM |
| if sys.version_info >= (3, 8): | ||
| from typing import Protocol, runtime_checkable | ||
| elif sys.version_info >= (3, 7): | ||
| from typing_extensions import Protocol, runtime_checkable |
There was a problem hiding this comment.
typing_extension does the version check internally, is it necessary to check here? pytorch seems to simply use typing_extensions.
There was a problem hiding this comment.
Your right but this shouldn't change the implementation
This should in principle make it so that any* legacy environment can be created and then converted into a valid modern environment.
WIP, I want to see if tests work for 3.6 (ugh) since I had to do a hack around protocols. I also still need to automatically integrate it into the
makepipeline.Brief explanation of the methodology: I implemented a protocol which will type check if something is a valid legacy env according to a minimal "mandatory" set of methods and fields. In python >= 3.8, this is available by default in
typing. In 3.7, we need to usetyping_extensions, which in turn does not support 3.6, for which the protocol check will be de facto disabled.*Restrictions apply. Old environments expected to inherit the old
gym.Envand will instead inherit from the newgym.Env, so there may be issues in some cases.