Refactor rllab from rllab.mujoco_py to mujoco_py (v1.5.0) #91

jonashen · 2018-05-25T01:46:34Z

When rllab was first developed, the mujoco_py package written by MuJoCo was barebones and sorely lacking in functionality. As such, the original creators of rllab decided to copy in a local folder called rllab.mujoco_py, in which they built upon the mujoco_py package with custom functions they felt were necessary to implement rllab.

As MuJoCo has now upgraded to a new version (i.e. v1.5.0), many of the functions in this folder are now obsolete, and can be replaced. Thus, there is no need to keep this folder; instead, we can utilize the functionality of mujoco_py directly. This PR will replace all usage of rllab.mujoco_py with mujoco_py instead, as well as remove the rllab.mujoco_py folder itself.

P.S. this PR will also update the setup_mujoco.sh script to support installation of MuJoCo v1.5.0 in lieu of v1.3.0.

P.P.S. I have also inadvertently built upon a commit that supports asynchronous support for plot rollout.

See: #1, #4, #55, #90

ryanjulian

Looks good but this PR definitely looks like a WIP.

Please remove all commented-out code
If you're unsure whether an API maps 1:1, test it.

ryanjulian · 2018-05-25T19:51:00Z

rllab/algos/batch_polopt.py

-            plotter.init_plot(self.env, self.policy)
+            plotter.init_plot(self.env, self.policy, self.max_path_length)
+            # Call rollout once to display glfw window
+            rollout(self.env, self.policy, animated=True, max_path_length=self.max_path_length)


Make a TODO/github issue to make this API better. Shouldn't this be part of init_plot()?

ryanjulian · 2018-05-25T19:51:22Z

rllab/algos/batch_polopt.py

@@ -4,6 +4,7 @@
 import rllab.misc.logger as logger
 import rllab.plotter as plotter
 from rllab.policies.base import Policy
+from rllab.sampler.utils import rollout


PEP8: alphabetical imports

Can you explain which part isn't in alphabetical order? I don't see it

ryanjulian · 2018-05-25T19:52:37Z

rllab/envs/mujoco/gather/embedded_viewer.py

-import rllab.mujoco_py.mjconstants as C
-from rllab.mujoco_py.mjlib import mjlib
+from mujoco_py import MjRenderContext, MjSim, functions, const as C
+from mujoco_py.generated.const import CAT_ALL


PEP8 imports

https://www.python.org/dev/peps/pep-0008/#imports

"MuJoCo comes with its own version of GLFW, so it's preferable to use that one. The easy solution is to import mujoco_py before import glfw."

This comment is specified by the creators of mujoco_py; should I make a note of this fact using a comment? If so, should I make a note of this in every file that contains these two import statements?

See: https://github.com/openai/mujoco-py/blob/6ac6ac203a875ef35b1505827264cadccbfd9f05/mujoco_py/builder.py

ryanjulian · 2018-05-25T19:52:58Z

rllab/envs/mujoco/gather/embedded_viewer.py

-        self.con = mjcore.MJRCONTEXT()
+        # self.objects = mjcore.MJVOBJECTS()
+        # self.cam = mjcore.MJVCAMERA()
+        # self.vopt = mjcore.MJVOPTION()


Please don't commit commented-out code.

ryanjulian · 2018-05-25T19:53:25Z

rllab/envs/mujoco/gather/embedded_viewer.py

@@ -35,52 +39,67 @@ def __init__(self):
    def set_model(self, model):
        self.model = model
        if model:
-            self.data = model.data
+            # self.data = model.data


please don't commit commented-out code.

ryanjulian · 2018-05-25T19:57:30Z

rllab/envs/mujoco/gather/gather_env.py

@@ -401,9 +410,9 @@ def get_ori(self):
            obj = obj.wrapped_env
        try:
            return obj.get_ori()
-        except (NotImplementedError, AttributeError) as e:
+        except (NotImplementedError, AttributeError):


If this is anything but completely meaningless, can you please at least omit a warning in the logger?

If the program cannot meaningfully continue when this error is raised, then do not suppress it.

This was an implementation of the original contributors. As specified in this function's header,

First it tries to use a get_ori from the wrapped env. If not successful, falls back to the default based on the ORI_IND specified in Maze (not accurate for quaternions)

Thus, the program will continue without fault even if this exception is thrown; it simply falls back to the default implementation.

ryanjulian · 2018-05-25T19:57:50Z

rllab/envs/mujoco/mujoco_env.py

@@ -5,14 +5,17 @@
 from rllab import spaces
 from rllab.envs.base import Env
 from rllab.misc.overrides import overrides
-from rllab.mujoco_py import MjModel, MjViewer
+import mujoco_py


PEP8 imports

ryanjulian · 2018-05-25T19:58:51Z

rllab/mujoco_py/Gemfile.lock

@@ -1,29 +0,0 @@
-GEM


deleted code is best code.

ryanjulian · 2018-05-25T20:00:26Z

rllab/plotter/plotter.py

@@ -1,8 +1,10 @@
 import atexit


did this file get accidentally mixed with your other commit for async plotting?

see my slack announcement about rebasing.

ryanjulian · 2018-05-25T20:01:46Z

scripts/setup_mujoco.sh

@@ -1,14 +1,14 @@
 #!/bin/bash

 if [ "$(uname)" == "Darwin" ]; then
-    mujoco_file="libmujoco131.dylib"


let's remove this whole setup and just rely on the system MuJoCo.

You will need to update the .rst documentation.

I take that back. Let's make that a different issue.

ryanjulian · 2018-05-26T00:41:14Z

rllab/envs/mujoco/gather/embedded_viewer.py

+import mujoco_py
+from mujoco_py import const as C
+from mujoco_py import functions
+from mujoco_py import MjRenderContext


PEP8: import ordering and grouping

ryanjulian · 2018-05-26T00:41:40Z

rllab/envs/mujoco/gather/gather_env.py

 from ctypes import byref
+import math
+import mujoco_py
+from mujoco_py import functions


PEP8: import ordering and grouping

ryanjulian · 2018-05-26T00:41:58Z

rllab/envs/mujoco/mujoco_env.py

+from cached_property import cached_property
+import mako.template
+import mako.lookup
+import mujoco_py


PEP8: import ordering and grouping

ryanjulian · 2018-05-26T00:42:26Z

rllab/envs/mujoco/mujoco_env.py

+        #     frame_skip_id = self.data.numeric_names.index("frame_skip")
+        #     addr = self.sim.numeric_adr.flat[frame_skip_id]
+        #     self.frame_skip = int(self.sim.numeric_data.flat[addr])
+        # else:


Why all the commented code? Remove it or leave a TODO+github issue.

ryanjulian · 2018-05-26T00:42:38Z

rllab/envs/mujoco/mujoco_env.py


    def get_body_xmat(self, body_name):
-        idx = self.model.body_names.index(body_name)
-        return self.model.data.xmat[idx].reshape((3, 3))
+        # idx = self.model.body_names.index(body_name)


commented code

ryanjulian · 2018-05-26T00:42:46Z

rllab/envs/mujoco/mujoco_env.py

-        idx = self.model.body_names.index(body_name)
-        return self.model.data.com_subtree[idx]
+        # idx = self.model.body_names.index(body_name)
+        # return self.sim.data.subtree_com[idx]


commented code

ryanjulian · 2018-05-26T00:42:52Z

rllab/envs/mujoco/mujoco_env.py

-        idx = self.model.body_names.index(body_name)
-        return self.model.body_comvels[idx]
+        # idx = self.model.body_names.index(body_name)
+        # return self.model.body_comvels[idx]


commented code

ryanjulian · 2018-05-26T00:42:57Z

rllab/envs/mujoco/point_env.py

 from .mujoco_env import MujocoEnv
+import numpy as np
+from rllab.envs.base import Step


PEP8: import ordering and grouping

ryanjulian · 2018-05-26T00:43:09Z

rllab/envs/mujoco/point_env.py

@@ -1,10 +1,10 @@
-from rllab.envs.base import Step
+import glfw
+import math
 from .mujoco_env import MujocoEnv


PEP8: no relative imports

glfw is an absolute import:

/Library/Frameworks/Pyton.framework/Versions/3.6/lib/python3.6/site-packages/glfw/init.py

from .mujoco_env import MujocoEnv

ryanjulian

Just make sure to remove the changes to rllab/plotter/plotter.py before merge

ryanjulian · 2018-05-29T21:40:37Z

rllab/envs/mujoco/point_env.py

+
+import glfw
+import numpy as np
+
 from .mujoco_env import MujocoEnv


PEP8: no relative imports, please.

ryanjulian

This looks good. Please fix my comment about the relative import, rebase your changes, and submit.

Don't forget to set the commit message to the authoritative one when you submit. Wrap the body to 72 chars, please.

…some cleanup

jonashen requested review from CatherineSue, hejia-zhang and ryanjulian May 25, 2018 01:48

CatherineSue approved these changes May 25, 2018

View reviewed changes

ryanjulian mentioned this pull request May 25, 2018

Support MuJoCo 1.5 #55

Closed

ryanjulian requested changes May 25, 2018

View reviewed changes

ryanjulian added this to the Week of May 21 milestone May 25, 2018

ryanjulian added big big multi-feature projects housecleaning labels May 25, 2018

ryanjulian requested changes May 26, 2018

View reviewed changes

ryanjulian assigned jonashen May 26, 2018

ryanjulian approved these changes May 29, 2018

View reviewed changes

ryanjulian reviewed May 29, 2018

View reviewed changes

ryanjulian approved these changes May 29, 2018

View reviewed changes

Jonathon Shen and others added 15 commits May 29, 2018 15:18

Added MuJoCo v1.5 support to rllab

bcfb13f

Updated environments to MuJoCo 1.5

6be4cec

Fixed CPU bug

0b5627d

Updated async support for plot rollout

c0e5f4c

Replaced rllab.mujoco_py with mujoco_py

48cff3e

Ported embedded_viewer.py to mujoco_py

5cc2e52

Transitioned mjvObjects to mjvScene

746ed3d

Finished refactoring code to utilize built-in mujoco_py library, and …

85075d4

…some cleanup

PEP8 refactoring / remove commented code

2dea5d7

Expanded import statements to 1 import per line

47b900b

Expanded import statements to 1 import per line

cb4318d

PEP8: import ordering and grouping refactoring

4cc9e15

PEP8: import ordering and grouping refactoring

0e5bf10

Undo plot rollout patch

7177bd1

Updated relative imports to absolute imports

0e7e2bc

jonashen force-pushed the mujoco_py branch from 6f4aa10 to 0e7e2bc Compare May 29, 2018 22:22

jonashen added 4 commits May 29, 2018 16:07

Commented out plot=True

826bedd

Fixed yapf errors from Travis CI

35e222b

More yapf conformities

c59687b

Removed unused dependencies

e5106a0

jonashen merged commit 2e55295 into ryanjulian:integration May 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor rllab from rllab.mujoco_py to mujoco_py (v1.5.0) #91

Refactor rllab from rllab.mujoco_py to mujoco_py (v1.5.0) #91

jonashen commented May 25, 2018 •

edited

Loading

ryanjulian left a comment

ryanjulian May 25, 2018

ryanjulian May 25, 2018

jonashen May 25, 2018 •

edited

Loading

ryanjulian May 25, 2018

jonashen May 25, 2018 •

edited

Loading

ryanjulian May 25, 2018

ryanjulian May 25, 2018

ryanjulian May 25, 2018

jonashen May 25, 2018

ryanjulian May 26, 2018

ryanjulian May 25, 2018

ryanjulian May 25, 2018

ryanjulian May 25, 2018

ryanjulian May 25, 2018

ryanjulian May 25, 2018

ryanjulian May 25, 2018

ryanjulian May 26, 2018

ryanjulian May 26, 2018

ryanjulian May 26, 2018

ryanjulian May 26, 2018

ryanjulian May 26, 2018

ryanjulian May 26, 2018

ryanjulian May 26, 2018

ryanjulian May 26, 2018

ryanjulian May 26, 2018

jonashen May 26, 2018

ryanjulian May 26, 2018

ryanjulian left a comment

ryanjulian May 29, 2018

ryanjulian left a comment

Refactor rllab from rllab.mujoco_py to mujoco_py (v1.5.0) #91

Refactor rllab from rllab.mujoco_py to mujoco_py (v1.5.0) #91

Conversation

jonashen commented May 25, 2018 • edited Loading

ryanjulian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonashen May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonashen May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanjulian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanjulian left a comment

Choose a reason for hiding this comment

jonashen commented May 25, 2018 •

edited

Loading

jonashen May 25, 2018 •

edited

Loading

jonashen May 25, 2018 •

edited

Loading