-
Notifications
You must be signed in to change notification settings - Fork 237
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
Bypass out-of-sync Gym registry in SubprocVecEnv by resolving EnvSpec #160
Changes from 3 commits
a1b9c03
0576bc5
f527f0f
a442b00
560c068
3f8d29f
e7fc11d
365c177
e8061a6
902fe96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,15 +49,29 @@ def make_vec_env(env_name: str, | |
max_episode_steps: If specified, wraps VecEnv in TimeLimit wrapper with | ||
this episode length before returning. | ||
""" | ||
# Resolve the spec outside of the subprocess first, so that it is available to | ||
# subprocesses via automatic pickling. | ||
spec = gym.spec(env_name) | ||
|
||
def make_env(i, this_seed): | ||
env = gym.make(env_name) | ||
# Previously, we directly called `gym.make(env_name)`. | ||
# | ||
# That direct approach was problematic (especially in combination with Ray) | ||
# because the forkserver from which subprocesses are forked might have been | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure this is what's going on? It's fine as a hypothesis but I don't want to immortalize in a comment something we're not sure about. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm 80% confident the
With that said, there does seem to be some logic to preload modules (by default, I think, the Seems plausible that starting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we can't get to the bottom of this, may be better to leave the comment vague and cite this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree that we should be vague about the cause in the comments and note the PR, I've made these changes. |
||
# spawned before `env_name` was registered in the main process, | ||
# causing `env_name` to never exist in the Gym registry of the forked | ||
# subprocess that is running `make_env(env_name)`. | ||
env = spec.make() | ||
|
||
# Seed each environment with a different, non-sequential seed for diversity | ||
# (even if caller is passing us sequentially-assigned base seeds). int() is | ||
# necessary to work around gym bug where it chokes on numpy int64s. | ||
env.seed(int(this_seed)) | ||
|
||
if max_episode_steps is not None: | ||
env = TimeLimit(env, max_episode_steps) | ||
elif (spec.max_episode_steps is not None) and not spec.tags.get('vnc'): | ||
shwang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
env = TimeLimit(env, max_episode_steps=spec.max_episode_steps) | ||
|
||
# Use Monitor to record statistics needed for Baselines algorithms logging | ||
# Optionally, save to disk | ||
|
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.
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 expanded this comment a bit more in 902fe96. Wanted to note that the gym registry total timesteps thing is default behavior for
gym.make
.