Skip to content
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

[RLlib] [MultiAgentEnv Refactor #2] Change space types for BaseEnvs and MultiAgentEnvs #21063

Merged
merged 9 commits into from
Jan 6, 2022

Conversation

avnishn
Copy link
Member

@avnishn avnishn commented Dec 13, 2021

Environment's spaces should always be of the format:

{agent_id_1: space_1, agent_id_2 : space_2} in the multi-agent case, and gym.Space for non-multi-agent vectorized environments.

This differs from a change I made recently where my spaces mapped {env_id : env.space}. I misunderstood that when a base environment has multiple sub environments, this can only mean that the environment has been vectorized, and not that multiple different types of environments have been initialized in the BaseEnv.

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@avnishn avnishn requested review from sven1977 and gjoliver December 13, 2021 23:21
@avnishn avnishn force-pushed the change_env_space_types branch from 15869c0 to 5902365 Compare December 16, 2021 04:53
rllib/env/base_env.py Outdated Show resolved Hide resolved
rllib/env/base_env.py Outdated Show resolved Hide resolved
rllib/env/base_env.py Outdated Show resolved Hide resolved
@avnishn avnishn force-pushed the change_env_space_types branch from 5902365 to 321afc4 Compare December 20, 2021 22:13
@avnishn avnishn marked this pull request as draft December 21, 2021 00:06
@avnishn avnishn force-pushed the change_env_space_types branch from 9ad419e to 0571a9c Compare December 21, 2021 18:17
@avnishn avnishn force-pushed the change_env_space_types branch from 0571a9c to f96913e Compare January 3, 2022 17:15
rllib/env/multi_agent_env.py Outdated Show resolved Hide resolved
rllib/env/multi_agent_env.py Show resolved Hide resolved
rllib/env/multi_agent_env.py Outdated Show resolved Hide resolved
@avnishn avnishn force-pushed the change_env_space_types branch from f96913e to ac2fc52 Compare January 5, 2022 01:08
@avnishn avnishn marked this pull request as ready for review January 5, 2022 01:09
@avnishn avnishn changed the title [RLlib] [WIP] [MultiAgentEnv Refactor #2] Change space types for BaseEnvs [RLlib] [MultiAgentEnv Refactor #2] Change space types for BaseEnvs Jan 5, 2022
@avnishn avnishn changed the title [RLlib] [MultiAgentEnv Refactor #2] Change space types for BaseEnvs [RLlib] [MultiAgentEnv Refactor #2] Change space types for BaseEnvs and MultiAgentEnvs Jan 5, 2022
rllib/env/multi_agent_env.py Outdated Show resolved Hide resolved
if not hasattr(self, "_spaces_in_preferred_format") or \
self._spaces_in_preferred_format is None:
self._spaces_in_preferred_format = \
self._check_if_space_maps_agent_id_to_sub_space()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a pure software engineering thing, any reason we don't do this in init but here instead, in a utility function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users will inherit from this class, however they may not necessarily call this function inside of their init functions. If I added this function to init, it may get called before the user has defined their observation/action spaces.

Although calling it here is messy, it allows for a nice external functionality, where if a user defines their spaces in the preferred format, they do not need to implement any of these sampling or checking methods on their own.

Comment on lines 242 to 243
The space will either be a gym.space directly corresponding to the

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete docstring.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yerp sorry.

rllib/env/multi_agent_env.py Outdated Show resolved Hide resolved
rllib/env/multi_agent_env.py Outdated Show resolved Hide resolved
rllib/env/multi_agent_env.py Outdated Show resolved Hide resolved
rllib/env/multi_agent_env.py Show resolved Hide resolved
if not hasattr(self, "_spaces_in_preferred_format") or \
self._spaces_in_preferred_format is None:
self._spaces_in_preferred_format = \
self._check_if_space_maps_agent_id_to_sub_space()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users will inherit from this class, however they may not necessarily call this function inside of their init functions. If I added this function to init, it may get called before the user has defined their observation/action spaces.

Although calling it here is messy, it allows for a nice external functionality, where if a user defines their spaces in the preferred format, they do not need to implement any of these sampling or checking methods on their own.

rllib/env/multi_agent_env.py Outdated Show resolved Hide resolved
rllib/env/multi_agent_env.py Outdated Show resolved Hide resolved
rllib/env/multi_agent_env.py Outdated Show resolved Hide resolved
Comment on lines 242 to 243
The space will either be a gym.space directly corresponding to the

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yerp sorry.

@avnishn avnishn force-pushed the change_env_space_types branch 2 times, most recently from 4c49eae to d76a8f4 Compare January 6, 2022 18:33
@avnishn avnishn force-pushed the change_env_space_types branch from d76a8f4 to b2a6033 Compare January 6, 2022 20:17
@richardliaw richardliaw merged commit 39f8072 into ray-project:master Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants