Skip to content

Conversation

@rodrigodelazcano
Copy link
Contributor

Changes made:

  • Create Viewer() class to render window in "human" mode with dm_viewer and glfw
  • Modified the default viewer_setup() method for all mujoco_environments (only for v3 envs)

def _get_obs(self):
position = self.sim.data.qpos.flat.copy()
velocity = self.sim.data.qvel.flat.copy()
position = self.sim.position().flat.copy()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This are minor changes. I felt the attributes are more accessible

@jkterry1
Copy link
Collaborator

jkterry1 commented Feb 4, 2022

@ikostrikov could you please take a look at this?

@ikostrikov
Copy link

@jkterry1 @rodrigodelazcano

I ran the tests here:
https://github.com/ikostrikov/gym_dmc

It passes all of them. But I would also suggest running deterministic training to verify that results are identical (or at least similar enough). Right now, my training code is non-deterministic. It will be very useful if you know someone who could test that.

However, the viewer doesn't work for me on v2 versions of the environments. It works on v3, though. Tested on mac with m1 and ubuntu. Here is a script to reproduce the issue:

import gym

env = gym.make('HalfCheetah-v2')

env.reset()

env.render()

Error:

Traceback (most recent call last):
  File "/Users/kostrikov/GitHub/gym_dmc/tmp.py", line 16, in <module>
    env.render()
  File "/Users/kostrikov/miniconda3/envs/py39/lib/python3.9/site-packages/gym/core.py", line 269, in render
    return self.env.render(mode, **kwargs)
  File "/Users/kostrikov/miniconda3/envs/py39/lib/python3.9/site-packages/gym/core.py", line 269, in render
    return self.env.render(mode, **kwargs)
  File "/Users/kostrikov/miniconda3/envs/py39/lib/python3.9/site-packages/gym/envs/mujoco/mujoco_env.py", line 161, in render
    self.viewer_setup()
  File "/Users/kostrikov/miniconda3/envs/py39/lib/python3.9/site-packages/gym/envs/mujoco/half_cheetah.py", line 39, in viewer_setup
    self.viewer.cam.distance = self.model.stat.extent * 0.5
AttributeError: 'WindowViewer' object has no attribute 'cam'

One minor nitpick (feel free to ignore):
position/velocity naming might be confusing.

qpos/qvel in mujoco corresponds to joint position/velocity. At the same time, xpos/xvel corresponds to global cartesian position/velocity.

@rodrigodelazcano
Copy link
Contributor Author

@jkterry1 @rodrigodelazcano

I ran the tests here: https://github.com/ikostrikov/gym_dmc

It passes all of them. But I would also suggest running deterministic training to verify that results are identical (or at least similar enough). Right now, my training code is non-deterministic. It will be very useful if you know someone who could test that.

However, the viewer doesn't work for me on v2 versions of the environments. It works on v3, though. Tested on mac with m1 and ubuntu. Here is a script to reproduce the issue:

import gym

env = gym.make('HalfCheetah-v2')

env.reset()

env.render()

Error:

Traceback (most recent call last):
  File "/Users/kostrikov/GitHub/gym_dmc/tmp.py", line 16, in <module>
    env.render()
  File "/Users/kostrikov/miniconda3/envs/py39/lib/python3.9/site-packages/gym/core.py", line 269, in render
    return self.env.render(mode, **kwargs)
  File "/Users/kostrikov/miniconda3/envs/py39/lib/python3.9/site-packages/gym/core.py", line 269, in render
    return self.env.render(mode, **kwargs)
  File "/Users/kostrikov/miniconda3/envs/py39/lib/python3.9/site-packages/gym/envs/mujoco/mujoco_env.py", line 161, in render
    self.viewer_setup()
  File "/Users/kostrikov/miniconda3/envs/py39/lib/python3.9/site-packages/gym/envs/mujoco/half_cheetah.py", line 39, in viewer_setup
    self.viewer.cam.distance = self.model.stat.extent * 0.5
AttributeError: 'WindowViewer' object has no attribute 'cam'

One minor nitpick (feel free to ignore): position/velocity naming might be confusing.

qpos/qvel in mujoco corresponds to joint position/velocity. At the same time, xpos/xvel corresponds to global cartesian position/velocity.

Hi @ikostrikov. Thank you for your feedback. I actually haven't updated the v2 environments for the new render since I though they will be deprecated. Should I update them? Won't take me long

@ikostrikov
Copy link

@rodrigodelazcano if it doesn't take too much time, it will be very useful. Many codebases still use v2 by default even though they are deprecated.

@rodrigodelazcano
Copy link
Contributor Author

@rodrigodelazcano if it doesn't take too much time, it will be very useful. Many codebases still use v2 by default even though they are deprecated.

Done :)

self.viewer.cam.elevation = -20
self.viewer.set_free_camera_settings(
trackbodyid=2,
distance=self.model.stat.extent * 1.0,

Choose a reason for hiding this comment

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

Should it be trackbodyid=1 here?

)

DEFAULT_SIZE = 500
DEFAULT_SIZE = 480

Choose a reason for hiding this comment

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

Is the change from 500 to 480 necessary?

@ikostrikov
Copy link

@rodrigodelazcano, thanks again for your work on the viewer! I did another pass on the commits. Please take a look.

Also, could you squash all commits into one after reviewing my comments?

Thanks!

@rodrigodelazcano
Copy link
Contributor Author

Thanks for helping with the review @ikostrikov. I made all the changes, if you can have a final look at it that will be great.

@ikostrikov
Copy link

@rodrigodelazcano it looks great now! Thanks a lot!

@jkterry1
Copy link
Collaborator

@ikostrikov can you please review this PR again rodrigo made a bunch of changes

@jkterry1
Copy link
Collaborator

@JamesKCS could you please make sure your issues with ant are properly resolved here too?

@JamesKCS
Copy link

@JamesKCS could you please make sure your issues with ant are properly resolved here too?

@jkterry1 Thank you for looking into this. I tried to check, but I think I am doing something wrong. I made a new conda environment named test_mujoco_change, activated the environment and ran
~/anaconda3/envs/test_mujoco_change/bin/pip install git+git://github.com/rodrigodelazcano/gym#egg=gym[mujoco] as directed in the other issue,
but the line env = gym.make('Ant-v2') results in the no module named 'mujoco_py' error below (and Ant-v4 gives a similar error). I think I am maybe installing/testing the wrong thing? Thank you again.

/home/james/anaconda3/envs/test_mujoco_change/lib/python3.7/site-packages/gym/envs/registration.py:506: UserWarning: WARN: The environment Ant-v2 is out of date. You should consider upgrading to version `v4` with the environment ID `Ant-v4`.
  f"The environment {path} is out of date. You should consider "
Traceback (most recent call last):
  File "main.py", line 2, in <module>
    env = gym.make('Ant-v2')
  File "/home/james/anaconda3/envs/test_mujoco_change/lib/python3.7/site-packages/gym/envs/registration.py", line 676, in make
    return registry.make(id, **kwargs)
  File "/home/james/anaconda3/envs/test_mujoco_change/lib/python3.7/site-packages/gym/envs/registration.py", line 520, in make
    return spec.make(**kwargs)
  File "/home/james/anaconda3/envs/test_mujoco_change/lib/python3.7/site-packages/gym/envs/registration.py", line 139, in make
    cls = load(self.entry_point)
  File "/home/james/anaconda3/envs/test_mujoco_change/lib/python3.7/site-packages/gym/envs/registration.py", line 55, in load
    mod = importlib.import_module(mod_name)
  File "/home/james/anaconda3/envs/test_mujoco_change/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/james/anaconda3/envs/test_mujoco_change/lib/python3.7/site-packages/gym/envs/mujoco/__init__.py", line 15, in <module>
    from gym.envs.mujoco.pusher import PusherEnv
  File "/home/james/anaconda3/envs/test_mujoco_change/lib/python3.7/site-packages/gym/envs/mujoco/pusher.py", line 5, in <module>
    import mujoco_py
ModuleNotFoundError: No module named 'mujoco_py'

@ikostrikov
Copy link

@jkterry1 I apologize for the slow reply. I'm getting the same error as above.

  File "/home/kostrikov/miniconda3/envs/test_mujoco39/lib/python3.9/site-packages/gym/envs/mujoco/__init__.py", line 15, in <module>
    from gym.envs.mujoco.pusher import PusherEnv
  File "/home/kostrikov/miniconda3/envs/test_mujoco39/lib/python3.9/site-packages/gym/envs/mujoco/pusher.py", line 5, in <module>
    import mujoco_py
ModuleNotFoundError: No module named 'mujoco_py'

@rodrigodelazcano
Copy link
Contributor Author

@JamesKCS , @ikostrikov and @jkterry1, sorry for that. I removed the dependency for mujoco_py since it will be deprecated in the future. In the last commit I recovered it, please try now. Also, @JamesKCS, the contact force issue has only been solved for version 4 environments (Ant-v4)

@ikostrikov
Copy link

@rodrigodelazcano

PusherEnv does not seem to depend on mujoco_py in any way

import mujoco_py

Is it possible to remove this line (and mujoco_py dependency as well)?

@rodrigodelazcano
Copy link
Contributor Author

Should mujoco_py dependency be removed despite keeping the past versions of mujoco environments?

@ikostrikov
Copy link

@rodrigodelazcano one potential solution is to have both gym[mujoco] and gym[dm_control], and provide an explanation in the readme + add a warning that gym[mujoco] is deprecated. @jkterry1 what do you think?

@jkterry1
Copy link
Collaborator

jkterry1 commented Mar 2, 2022

@rodrigodelazcano I like ikostrikov's proposal, lets do that

@jkterry1
Copy link
Collaborator

jkterry1 commented Mar 2, 2022

Instead of saying mujoco-py is deprecated, just say it's unmaintained. We aren't going to/can't drop support for older mujoco-py based versions for the foreseeable future, it's deprecated may not be the best description.

@jkterry1
Copy link
Collaborator

jkterry1 commented Mar 2, 2022

Also @rodrigodelazcano , this PR doesn't overwrite the removals in this PR right? #2651

@jkterry1
Copy link
Collaborator

jkterry1 commented Mar 2, 2022

Costa ran some basic experiments:

https://wandb.ai/costa-huang/cleanRL/reports/MuJoCo-v2-vs-v4-environments--VmlldzoxNjM1OTAx

The changes appear to be within acceptable deviations for a version bump

@vwxyzjn
Copy link
Contributor

vwxyzjn commented Mar 2, 2022

Per @jkterry1's suggestion, I started running some benchmarks to see if there are breaking performance changes.

Report here: https://wandb.ai/costa-huang/cleanRL/reports/MuJoCo-v2-vs-v4-environments--VmlldzoxNjM1OTAx

Code here: https://github.com/vwxyzjn/validate-new-gym-mujoco-envs

HalfCheetah-v4 Walker2d-v4
Hopper-v4

So... ya they may be different (I personally find mujoco envs shows greater variance with random seeds, and that the means of the curve roughly fall into the min/max range of other curves), c'est la vie" @jkterry1

@ikostrikov
Copy link

@jkterry1 @vwxyzjn this is unexpected. Can we chat via some messenger?

@jkterry1
Copy link
Collaborator

jkterry1 commented Mar 2, 2022

@ikostrikov email me at [email protected], I'll give you my discord information. Basically the people involved in gym are coordinated over discord.

@jkterry1
Copy link
Collaborator

For future travelers, after some experimentation between cost and rodrigo, the cause for the performance changes was that we fixed the very long standing bugs with contact forces being zero in ant and walker2d.

younik and others added 4 commits April 15, 2022 11:03
* fix impulse sample bug and add adjustable gravity

* revert weird sample thing

* assertation

* add wind

* fix wind to use applyforce

* black

* add tests for wind

* fix test bug

* run lunar lander tests over 10 runs

* reduce wind for heuristic landing

* made wind power kwarg

* reduce wind power for heuristic landing

* update doc

* remove wind power from kwarg

* fix tests by deleting tests ;)

* Add back wind power as kwarg

* the black sheep

* remove rogue plus
* first commit domain randomize

* black

* update doc

* add some type hints and internalized some functions

* we were told, the black bear is innocent; but I should not like to trust
myself wit him

* Don't need two color conventions

* don't multiply twice

* hardcore -> domain_randomize & register

* remove rogue decorator
@rodrigodelazcano rodrigodelazcano changed the title Use dm_control instead of mujoco_py Use mujoco bindings instead of mujoco_py Apr 15, 2022
* fix impulse sample bug and add adjustable gravity

* revert weird sample thing

* assertation

* add wind

* fix wind to use applyforce

* black

* add tests for wind

* fix test bug

* run lunar lander tests over 10 runs

* reduce wind for heuristic landing

* made wind power kwarg

* reduce wind power for heuristic landing

* update doc

* remove wind power from kwarg

* fix tests by deleting tests ;)

* Add back wind power as kwarg

* the black sheep

* remove rogue plus

* slight doc fix and variable rename

* fix bug with track turn indicators

* black
@pseudo-rnd-thoughts
Copy link
Contributor

Looking ahead, there are two other large PR (#2671 and #2752) happening that will affect this PR as those updates will have to be replicated over to this PR, i.e. render API and two done API
@rodrigodelazcano Would you prefer to try and merge this PR first then make a subsequent PR to make the changes
Or make the changes in this PR and merge it after #2671 and #2752?

@vwxyzjn
Copy link
Contributor

vwxyzjn commented Apr 17, 2022

I would prefer to merge this PR and make a release if possible. #2671 and #2752 are much larger refactoring and could break existing code.

@rodrigodelazcano let me know if you have finalized the changes. I would be happy to do another round of benchmarks since contact force setting is changed.

* refactoring play function. Tests for keys to action mapping.

* Add mocking pygame events.

* partial event processing in class.

* pre-commit.

* quit pygame after tests.

* fix typos in functions names.

* Add type hint.

* Add test for play function.

* remove mockKeyEvent.

* remove unused main code.

* Adding type hints.

* catch custom exception in tests.

* Fix magic numbers.

* Add test with an actual environment.

* fix comment.

* Add TODO memo on env.render.

* change map with list comprehension.

* remove unused imports.

* Add type hint.

* typo.

* docstring.
@rodrigodelazcano
Copy link
Contributor Author

@vwxyzjn the past benchmarks should be enough. I'm trying to fix the CI to allow mujoco envs to be tested

mujoco_bindings

mujoco bindings

mujoco bindings

mujoco bindings

dm_control for mujoco envs

dm for mj_py

mujoco v4

move viewer

lint

mujoco dependency

remove mujoco_py pusherEnv

setup

dm_control contact forces

remove print setup

mujoco bindings no rendering

rendering

viewer_setup

dependencies

rebase fix

dependencies

pre-commit fix

ping mujoco version

fix mujoco test
@rodrigodelazcano
Copy link
Contributor Author

@jkterry1 and @vwxyzjn, just finished updating the CI. I had to skip the rendering test of the old mujoco_py env versions, the past error was deriving from mujoco_py. Please let me know if anymore changes are required

@pseudo-rnd-thoughts
Copy link
Contributor

@rodrigodelazcano Does that mean that we don't test the old environment can render or can old environment just not render anymore. I just want to check for backward compatibility

@rodrigodelazcano
Copy link
Contributor Author

@pseudo-rnd-thoughts it only means that the old environments are not being tested for rendering (new v4 envs are), because they were being also skipped on past releases and I not able to find the issue of them not passing test in headless systems .

However, the rendering implementation for old mujoco versions is still the same and should work as usual.

@rodrigodelazcano
Copy link
Contributor Author

This PR has been moved to #2762

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.