[RLlib] Make MultiAgentEnv inherit gym.Env to avoid direct class type manipulation #18156
Conversation
sven1977
left a comment
sven1977
left a comment
There was a problem hiding this comment.
Looks great! We should probably add some tests for rendering and recording for different env types (simple gym.Env -> BaseEnv; MultiAgentEnv -> BaseEnv).
|
|
||
| @PublicAPI | ||
| class MultiAgentEnv: | ||
| class MultiAgentEnv(gym.Env): |
There was a problem hiding this comment.
Wow! This is great! I tried this a few moths ago and one of the gym.Env parent methods would complain due to the rewards not being floats anymore (but multi-agent Dict[str, float]s).
…i-agent-gym-env # Conflicts: # rllib/evaluation/tests/test_rollout_worker.py
gjoliver
force-pushed
the
multi-agent-gym-env
branch
from
dedeb7d to
5f4da39
Compare
…i-agent-gym-env # Conflicts: # rllib/env/base_env.py
sven1977
changed the title
|
@sven1977 I know I'm a few versions late to the discussion, but this change imposes some additional requirements that weren't there before, and many of my simulations don't use all everything from the gym.Env API. The old rllib.MultiAgentEnv only required step and reset, which worked very well for me. I can probably create a wrapper that adapts my simulations to this new requirement, but I also wanted to ask if there's another Env class I can target now that MultiAgentEnv requires gym.Env interface support? |
|
does your env have observation and action spaces? |
I can put them in here.
Would still like to hear about this. |
|
Actually, this change didn't effect my environments. It was #21063 that refactored MultiAgentEnvs. I'll update my code to connect with this better. |
3 participants
Why are these changes needed?
This is a more exploratory PR. I am not saying we should commit this for sure.
When I was reading the monitoring wrapper related code, I noticed we have this logic to modify the class type of a MultiAgentEnv instance back & forth directly. I was a bit confused about the code.
So I made this change and mostly just want to double check with you to see why we need to do that instead of just having MultiAgentEnv and VideoMonitor classes inherit the right type.
Don't know if I am breaking anything somewhere actually :)
Thanks.
Checks
scripts/format.shto lint the changes in this PR.