Add mappings from env name to env module (e.g., mpe_environments) in addition to all_environments

[elliottower]

This fixes an issue raised in facebookresearch/BenchMARL#41 where the all_environments mapping requires all of the environments' packages, but if a user wants to have a mapping of subsets of environments, it is not currently possible. This allows you to do from pettingzoo.mpe import mpe_environments which contains a mapping from name to module "mpe/simple_adversary_v3": simple_adversary_v3 etc.

Edit: used the same as in all_modules with mpe/simple_adversary instead of below

I chose to remove the prefixes for simplicity, the all_environments mapping has mpe/simple_adversary_v3 whereas mpe_environments now has just simple_advisory_v3 as the key. I feel like this is simpler as when you are using an object called mpe_environments you don't need to specify that they are mpe environments again.

This may requite some slight refactoring if these are to be used in place of all_modules but for general use and future new users this feels like it makes the most sense to me. If somebody feels strongly though it can be changed.

I thought about having all_environments be the union of these, but that would be more complicated given the above choice to remove prefixes, and I also don't think it's a huge deal to effectively re-define the mapping, as it is more clear what it is when it is all defined in that one file.

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue), Depends on # (pull request)

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.
To upload images to a PR -- simply drag and drop or copy paste.

Checklist:

• I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
• I have run pytest -v and no errors are present.
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I solved any possible warnings that pytest -v has generated that are related to my code to the best of my knowledge.
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[matteobettini]

Thanks so much for this! I really appreciate it.

I am more of the opinion that keeping the same name in these module files and in all_modules (Aka keep the prefix) makes it less bug prone as an env in the library is uniquely identified by one and only one string. Plus this allows to have all_modules be the union of these.

but i can work with any choice of convention without any problems

(in case you decide to keep as is, you can still have all_environments be the union, you just need to preappend the prefix to the dict keys. This way is less bug prone as if you change an env name in one place you won't need to remember to change it in the other)

[elliottower]

Thanks so much for this! I really appreciate it.

I am more of the opinion that keeping the same name in these module files and in all_modules (Aka keep the prefix) makes it less bug prone as an env in the library is uniquely identified by one and only one string. Plus this allows to have all_modules be the union of these.

but i can work with any choice of convention without any problems

(in case you decide to keep as is, you can still have all_environments be the union, you just need to preappend the prefix to the dict keys. This way is less bug prone as if you change an env name in one place you won't need to remember to change it in the other)

Ah okay I can see that logic actually, sure yeah I can revert and keep the unique identifiers that are used everywhere then.

And when we change the env names we always do a find and replace and replace all of them, but yeah you're right that it is slightly easier to maintain if it is a union of the other environment mappings.

[elliottower]

My only concern with this PR is that previously when you do from pettingzoo.classic import go_v5 it would only import the modules relevant to that, whereas with this, the init file imports all of the modules, so if a user is missing the chess package for example, they wouldn't be able to import go. This means that it would be a breaking change if somebody for example only bothered to add chess related dependencies to their requirements for a chess-related repo, now they would have to add all of pettingzoo[classic] dependencies. That said though, the recommended usage is pip install 'pettingzoo[classic]' so I would hope most users are doing it this way.

This is also the same as how Gymnasium does it, and I've seen it in a lot of other projects as well, so I don't think it's a huge deal. Will have to make a note of it in the release notes though.

[matteobettini]

To address that, instead of putting the dicts in the init file, we could put then in a new file (all_modules.py to follow the utils convention, for example) so that the users can import that only if they need it.

[elliottower]

To address that, instead of putting the dicts in the init file, we could put then in a new file (envs.py or all_modules.py to follow the utils convention for example) so that the users can import that only if they need it.

I was thinking about that, but I couldn't come up with anything that made much sense. All modules would be confusing to have multiple files, maybe something including the word "mapping" or something.

Okay after thinking some more, the end result I want it to be from pettingzoo.classic import classic_environments though, so I could do the same convention as is currently done for the environment names, that is, make a file called classic_environments.py and have that do the imports.

[matteobettini]

To address that, instead of putting the dicts in the init file, we could put then in a new file (envs.py or all_modules.py to follow the utils convention for example) so that the users can import that only if they need it.

I was thinking about that, but I couldn't come up with anything that made much sense. All modules would be confusing to have multiple files, maybe something including the word "mapping" or something.

Okay after thinking some more, the end result I want it to be from pettingzoo.classic import classic_environments though, so I could do the same convention as is currently done for the environment names, that is, make a file called classic_environments.py and have that do the imports.

that could work. The only thing i see is that you are repeating “classic” in the filename and it would lead to that file having different names in different modules, while something like environments.py (containing an environment_registry) could have the same name in all modules

[matteobettini]

Another option, just to throw it on the table, for you to keep it in init, is to have the dict inside a function that runs the imports only when called.
Something like we do for libs here https://github.com/pytorch/rl/blob/d7e20e1be5ee7be60db2d0632ab878c08fcef1f0/torchrl/envs/libs/vmas.py#L42

[elliottower]

To address that, instead of putting the dicts in the init file, we could put then in a new file (envs.py or all_modules.py to follow the utils convention for example) so that the users can import that only if they need it.

I was thinking about that, but I couldn't come up with anything that made much sense. All modules would be confusing to have multiple files, maybe something including the word "mapping" or something.
Okay after thinking some more, the end result I want it to be from pettingzoo.classic import classic_environments though, so I could do the same convention as is currently done for the environment names, that is, make a file called classic_environments.py and have that do the imports.

that could work. The only thing i see is that you are repeating “classic” in the filename and it would lead to that file having different names in different modules, while something like environments.py (containing an environment_registry) could have the same name in all modules

We've discussed adding a registry system, similar to https://github.com/Farama-Foundation/Gymnasium/blob/main/gymnasium/envs/registration.py for example, but if it does get added I want to do it right, so I think I'm going to wait on that. You are right about re-using the word classic though, from pettingzoo.classic import all_modules would be simpler, I don't like the idea of having a bunch of variables which are different but named the same thing, but having files which are named the same in different hierarchies is okay I think, as you always know what all_modules will contain in pettingzoo.classic.all_modules for example.

It could just be called modules but I think all_modules makes it more obvious what it does, so I just decided to go with that. Definitely appreciate the suggestion (a second set of eyes is always very useful for these kinds of things)

[elliottower]

Another option, just to throw it on the table, for you to keep it in init, is to have the dict inside a function that runs the imports only when called. Something like we do for libs here https://github.com/pytorch/rl/blob/d7e20e1be5ee7be60db2d0632ab878c08fcef1f0/torchrl/envs/libs/vmas.py#L42

This isn't a bad idea either but I think keeping things as simple as possible is probably best for now. Adding a registration system in the future definitely would be good to do, and I don't want to add too many bells and whistles right now which would have to be removed for that.

[elliottower]

Just as a note, I decided to keep the mappings named classic_environments, that way if you import all of the all_modules files you will have separate names for each variable, and can combine them (which is what I do making the utils version of all_environment)