-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Update EnvSpec to improve backward compatibility
#355
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
Update EnvSpec to improve backward compatibility
#355
Conversation
|
Note, this breaks backward compatibility for |
rodrigodelazcano
left a comment
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.
The PR looks ready to be merged. I've been testing it with Minari to recreate environments from json environment specs and it works perfectly. Thanks @pseudo-rnd-thoughts !
RedTachyon
left a comment
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.
Two minor comments, but I wasn't able to look at this in proper depth. If it works for "normal" usage (i.e. with the branch installed you can create an env, get the spec, recreate it and do regular stuff without any obvious bugs or redundant warnings), and it works for Minari purposes, then looks good to me as well
| assert env_spec == recreated_env_spec | ||
|
|
||
|
|
||
| def test_wrapped_env_entry_point(): |
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.
Why is this test removed?
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 moved the tests to test_make.py I believe
gymnasium/core.py
Outdated
| # to avoid reference issues we deepcopy the prior environments spec and add the new information | ||
| env_spec = deepcopy(env_spec) | ||
| env_spec.applied_wrappers += (wrapper_spec,) | ||
| if self._cached_spec is None: |
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.
Minor comment for slight unspagghettification - it might be better to do
if self._cached_spec is not None:
return self._cached_spec
env_spec = self.env.spec
...Basically just flip the condition and do an early return to remove one level of indentation from the whole code block.
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.
Updated
Original PR #355 by pseudo-rnd-thoughts Original: Farama-Foundation/Gymnasium#355
…lity Merged from original PR #355 Original: Farama-Foundation/Gymnasium#355
Description
As noted by DLR-RM/stable-baselines3#1327 (comment), there are significant backward compatibility issues with #292
This PR updates
EnvSpecsuch that it is a "full description of the whole env". Using theEnvSpecinmakeis now identical tomake(str)as before. Critically, the difference withmakein v0.27 is that theEnvSpecchanges with each wrapper applied, including the default wrappers. As a result,env.unwrapped.spec'sEnvSpechas all parameters disabled such thatgym.make(env.unwrapped.spec)is equivalent to just the base environment.The advantage of this implementation is that users can define an environment,
envand perfectly replicate each stageAdditionally, this PR updates the testing for
makesuch that we check for both strings andEnvSpec.