-
Notifications
You must be signed in to change notification settings - Fork 526
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
Add EnvPool support (as VecEnv subclass) #355
base: master
Are you sure you want to change the base?
Conversation
Here are some speed results with Breakout and PPO. I can't understand why envpool is only slightly faster than dummy vec env. @araffin, @Trinkle23897 any insight? |
@araffin what do you think about moving this to sb3 (insert it to |
not sure about it. |
Makes sense, we will need to update the sb3 doc btw. |
@araffin, before merging, we need to explain this #355 (comment) |
if self.env_kwargs: | ||
warnings.warn( | ||
"EnvPool does not support env_kwargs, it will be ignored. " | ||
"To pass keyword argument to envpool, use vec_env_kwargs instead." |
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 don't think we have vec_env_kwargs
available in the cli, but we should add it.
rl_zoo3/exp_manager.py
Outdated
if ( | ||
"Neck" in self.env_name.gym_id | ||
or self.is_robotics_env(self.env_name.gym_id) | ||
or "parking-v0" in self.env_name.gym_id | ||
and len(self.monitor_kwargs) == 0 # do not overwrite custom kwargs | ||
): |
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.
if ( | |
"Neck" in self.env_name.gym_id | |
or self.is_robotics_env(self.env_name.gym_id) | |
or "parking-v0" in self.env_name.gym_id | |
and len(self.monitor_kwargs) == 0 # do not overwrite custom kwargs | |
): | |
if ( | |
self.is_robotics_env(self.env_name.gym_id) | |
or "parking-v0" in self.env_name.gym_id | |
and len(self.monitor_kwargs) == 0 # do not overwrite custom kwargs | |
): |
"Neck" in self.env_name.gym_id
was for personal use and should be removed.
@@ -153,6 +153,13 @@ def train() -> None: | |||
default=False, | |||
help="if toggled, display a progress bar using tqdm and rich", | |||
) | |||
parser.add_argument( |
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.
Missing: a section in the readme, I will try to create a real doc soon (the readme is way too long now).
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.
doc is there
from stable_baselines3.common.vec_env.base_vec_env import VecEnvObs, VecEnvStepReturn | ||
|
||
# Used when we want to access one or more VecEnv | ||
VecEnvIndices = Union[None, int, Iterable[int]] |
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.
we don't have that type hint defined in SB3?
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.
Yes, in base_with_env
. I don't remember why I redefined it here. I probably should have imported it
So, it seems to depends on the env, but yes it is surprising. I also have envpool slower than SubProcVecEnv on my machine with python 3.7 (but not as slow as reported here). But on MuJoCo env, envpool is the fastest for me: Envpool: ~4100 fps
vs
vs Dummy: ~3600 fps
|
Can you use py-spy to have a profiling result? |
It seems like there are a lot of "importlib" calls in rl_zoo3.train with envpool. That should not be the case and I believe something is wrong. Did you accidentally call envpool with multiple python processes/threads? |
Description
Based on #307, but use VecEnv subclass instead of VecEnvWrapper.
VecEnvWrapper
assumes the wrapped env of an instance ofVecEnv
, which is not the case ofEnvPool
. IMO, using theVecEnv
class directly is therefore a better option, and the implementation is not more complex.Motivation and Context
closes #241
Types of changes
Checklist:
make format
(required)make check-codestyle
andmake lint
(required)make pytest
andmake type
both pass. (required)Note: we are using a maximum length of 127 characters per line