-
-
Notifications
You must be signed in to change notification settings - Fork 96
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 MaMuJoCo (Multi-agent mujoco) Environments #53
Add MaMuJoCo (Multi-agent mujoco) Environments #53
Conversation
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.
This is a quick review, could you fix the pre-commit and there seems to be a number of type hint issues.
For the implementation, I believe in PettingZoo, each environment needs a versioned class as a pettingzoo.make
doesn't exist currently. Therefore, Im uncertain of the current implementation using MaMujoco(scenario="...")
won't make sense.
Additionally, could you provide a class structure as Im uncertain of how the many agent swimmer environment relates to the MaMujoco
class
gymnasium_robotics/envs/multiagent_mujoco/assets/coupled_half_cheetah.xml
Show resolved
Hide resolved
There is one class for The single agent MuJoCo Environment may be either a member of
Also, how should I name the agents in |
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.
Thanks for the changes, this is looking good. I don't understand the PettingZoo environment structure so we will need @jjshoots or @WillDudley to review.
In addition, there are several PettingZoo API tests that exist which we could use
gymnasium_robotics/envs/multiagent_mujoco/coupled_half_cheetah.py
Outdated
Show resolved
Hide resolved
gymnasium_robotics/envs/multiagent_mujoco/coupled_half_cheetah.py
Outdated
Show resolved
Hide resolved
if self.agent_obsk is None: | ||
return {self.possible_agents[0]: global_state} | ||
|
||
class data_struct: |
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 would move this outside the MaMujoco
class to be a global class
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.
Why? This is a function specific data struct,
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.
Ok, looking at the function in more detail. Why does the class exist? Can we not have the class attributes as normal variables?
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.
No we, can not pass all 122 arguments normally ;-)
obsk.build_obs
is written this way, to be extensible, it supports all possible mujoco
environments this way (and potentially be able to support future brax
based environments with some modifications)
I would like to request a review of the DOCs, I currently have written the index page for If you are satisfied with the
|
Could you fix the testing such that it passes all CI checks then I will make another pass over the code |
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.
From a pettingzoo perspective, LGTM. Seems like it's taking the single agent MJC envs and splitting the robot up into multiple segments as different agents and representing it as a ParallelEnv. From that perspective everything looks sound to me.
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.
This looks like it is very close to being done.
Couple of last comments
- I think you have already answered this but why are two of the
.xml
have.xml.template
. - Do we have tests that the new mujoco environments work as standard gym mujoco envs?
- Do we need copyright comments as listed in the PR todo list. If so, the comment should exist at the top of every file copied. https://github.com/Farama-Foundation/Gymnasium/blob/5d67eae4fbf699b84a20ec056f98802dd30268f3/gymnasium/utils/env_checker.py#L3 and https://github.com/Farama-Foundation/Gymnasium/blob/5d67eae4fbf699b84a20ec056f98802dd30268f3/gymnasium/utils/env_checker.py#L14
@rodrigodelazcano Do you have any last comments?
It looks great, thank you @Kallinteris-Andreas. We need to add the License as Mark mentioned. They use Apache 2.0 so you'll have to add the original Author in each file and a small description of any changes. Also, add the Other thing I would like to do in a future PR is to get rid of the additional .xml and .template files so that we don't add the extra jinja dependency. We can do this by importing the original Gymnasium .xml and editing it with ElementTree or dm_control pymjcf In 2. @pseudo-rnd-thoughts, I think these tests are already being done by https://github.com/Farama-Foundation/Gymnasium-Robotics/blob/main/tests/test_envs.py |
@rodrigodelazcano |
@Kallinteris-Andreas. I'm suggesting something similar to this:
|
Also, this is ready to be merged, thank you for hanging there @Kallinteris-Andreas . It will be done after a new release is made with the D4RL environments soon :) |
I have fixed all the Also, I have added all the required license things. |
@Kallinteris-Andreas can you also rebase to the latest commit in the main repo? |
@Kallinteris-Andreas nice job on this, sorry for it taking a while. I'm looking forward to your next contributions |
MaMuJoCo was first introduced in "FACMAC: Factored Multi-Agent Centralised Policy Gradients"
I consider this version of the code to be:
almostfeature completebut I still not sure how the Docs should be structured, e.g. do we need 1 page per task, or 1 for the Gymnasium/MuJoCo Tasks and 1 per new taskrequirements: (I have not added them tosetup.py
, because it is obvious to me, how it should be packaged, should it work with justpip install gymansium-robotics[MaMuJoCo]
for example)demo (feel free to try other scenarios/agent_configurations)
Notes:
Does not include versioning (-v0) this will be added right before it is ready for inclusion in the projectI need some help with deciding the structure (Since there are effectively a lot of domains)Not sure if it belongs in this repo, or it would be better as part of PettingZoo (your call)TODO (not by me)