Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Added the first version of the GymToUnityWrapper #3793
Conversation
ml-agents-envs/setup.py
Outdated
| "gym", | ||
| "cmake", | ||
| "atari-py", | ||
| "opencv-python", |
There was a problem hiding this comment.
This is a pretty heavyweight package BTW, on some platforms it will need to re-compile opencv.
There was a problem hiding this comment.
Outstanding question : Is it okay for us to have a dependency on gym and gym[atari]? One alternative would be to have a try catch in the gym_wrapper at import and raise an error if gym is not installed as well as adding the pip install gym when running our CI tests. Thoughts?
We could make it optional? We could also remove the atari test, this way, no dependency on atari or opencv
There was a problem hiding this comment.
I agree, you should NOT put these in ml-agents-envs. No preference between gym-unity and another package.
|
Wrapper itself looks great! Should we put the wrapper in gym-unity vs. ml-agents-envs? Not sure if we should have packages like |
Not sure, if the only reason is dependencies, I would say no. Make another package : ) ? |
Co-Authored-By: Chris Elion <[email protected]>
| gym_env = gym.make(name) | ||
| env = GymToUnityWrapper(gym_env, name) | ||
| assert env.get_behavior_names()[0] == name | ||
| if isinstance(gym_env.action_space, gym.spaces.Box): |
There was a problem hiding this comment.
I don't think this is a meaningful test, since you're just duplicating the logic from the code.
A better approach would be having an expected ActionType for each gym env and making sure that's set correctly.
| from mlagents_envs.gym_wrapper import GymToUnityWrapper | ||
| from mlagents_envs.base_env import ActionType | ||
|
|
||
| try: |
There was a problem hiding this comment.
Similar comment as gym_wrapper.py. Pretty sure that this file will always get imported by pytest, so mlagents-envs will not be runnable on their own.
|
Sorry, after looking more at this, I think this should not go in ml-agents-envs. gym-unity is more appropriate and keeps the gym dependency simpler. |
I disagree, gym has a very specific purpose and also a very specific directory structure (dictated by gym). All the gym code lives in the |
|
We should have |
So, create a new package ? |
|
In that case, I think either add another package, or add |
vincentpierre
changed the title
Are we using the special directory structure that gym requires? It seems they do so b/c of the registry, but we don't register the environment. It also seems that it's OK to have additional files other than Is the opencv requirement dictated by Gym? |
Only if we want to use Atari environments (removed it) |
I think |
|
@ervteng If we put this wrapper with the other gym wrapper, would we be okay having our trainers have a dependency on gym-unity? We might want to test our trainers with some gym environments latter. |
|
@vincentpierre I think that is OK. Would let us train gym envs using our trainer easily. |
|
@ervteng If you want to have a look at the latest commit. We do okay on cartpole (not perfect) but it seems montain-car is posing problems (maybe an issue with the wrapper and max_step) |
|
Closing PR as stale. |
Proposed change(s)
Need to add some tests to make sure it works.
Also need to add some tests for our trainers using gym environments (taking suggestions)
Only workd for single Box observation, Box or Discrete action.
I do not want to change the documentation yet, I would like to first make sure it is useful for our tests.
Outstanding question : Is it okay for us to have a dependency on gym and gym[atari]? One alternative would be to have a try catch in the gym_wrapper at import and raise an error if gym is not installed as well as adding the pip install gym when running our CI tests. Thoughts?
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
MLA-707
Types of change(s)
Checklist
Other comments