Feature pybullet

[dtch1997]

Continuation of incomplete PR from #87
This is my first time contributing to an open-source project so any advice is welcome, technical or otherwise

Types of changes

• Docs change / refactoring / dependency upgrade
• 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 change)

Motivation and Context / Related issue

This adds support for PyBullet, an open-source alternative to MuJoCo. MuJoCo-compatible and RobotSchool environments are supported via pybullet-gym.

How Has This Been Tested (if it applies)

python -m pytest tests/pybullet

Checklist

• The documentation is up-to-date with the changes I made.
• I have read the CONTRIBUTING document and completed the CLA (see CONTRIBUTING).
• All tests passed, and additional code has been covered with new tests.

[facebook-github-bot]

Hi @dtch1997!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[dtch1997]

I refactored the pybullet-relevant code into a separate mujoco/util/pybullet.py as suggested in #87 .
Have verified that tests/pybullet/test_util.py works.
Currently working on making tests/pybullet/test_diagnostics.py tests work.

[dtch1997]

I need some help with understanding the config dictionary setup in tests/{mujoco,pybullet}/test_diagnostics.py.

Currently I get the following error in my tests:

FAILED tests/pybullet/test_diagnostics.py::test_eval_on_dataset - omegaconf.errors.ConfigAttributeError: Missing key seed
FAILED tests/pybullet/test_diagnostics.py::test_finetuner - omegaconf.errors.ConfigAttributeError: Missing key seed
FAILED tests/pybullet/test_diagnostics.py::test_visualizer - omegaconf.errors.ConfigAttributeError: Missing key seed

There appears to be a missing key seed but I'm not sure where it should be inserted. I made sure that the config dictionary setup in tests/pybullet/test_diagnostics.py mirrors that used in tests/mujoco/test_diagnostics.py. I am unable to verify whether this issue also exists in tests/mujoco/test_diagnostics.py because I do not have a Mujoco license.

Appreciate help with getting this to work correctly.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[dtch1997]

I resolved the missing seed error (albeit rather hackily.)
The next problem is that mbrl/diagnostics is hardcoded to use mbrl.util.mujoco functions which doesn't work well with the new pybullet code. I intend to solve this by creating an abstract class for gym env types and subclassing to get polymorphic behaviour.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[dtch1997]

Commit 6512c94 adds a proposal for a general interface to gym backends. Instead of having if-else statements in every function, we can have a single if-else statement that constructs a handler which implements polymorphic behaviour.

@luisenp Does this look good? If so I can go ahead and implement it.

[luisenp]

The proposal for EnvHandler looks good! Just let a comment about our new version of make_env, which doesn't require legacy_make_env (that's just there for backwards compatibility). Otherwise, I haven't looked at the code too deeply or ran anything, but I'll put some time for that during the week. Thanks a lot for the contribution!

[luisenp]

Ah, this is normal. This just means that black changed some of your formatting automatically and you have to stage some of your files again.

…

On Sat, Oct 2, 2021, 10:28 AM Daniel Tan ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mbrl/util/pybullet.py <#135 (comment)> : > + self._exit_method = self._exit_pybullet_gym + else: + raise RuntimeError("Tried to freeze an unsupported environment.") + + def _enter_pybullet_gym(self): + # For now, the accepted envs are limited to ease implementation and testing + from pybulletgym.envs.roboschool.robots.locomotors.walker_base import WalkerBase as RSWalkerBase + from pybulletgym.envs.mujoco.robots.locomotors.walker_base import WalkerBase as MJWalkerBase + env = self._env.env + robot = env.robot + assert isinstance(robot, (RSWalkerBase, MJWalkerBase)) + self.state_id = env._p.saveState() + self.ground_ids = env.ground_ids + self.potential = env.potential + self.reward = float(env.reward) + robot_keys = [("body_rpy", tuple), ("body_xyz", tuple), ("feet_contact", np.copy), ("initial_z", float), ("joint_speeds", np.copy), ("joints_at_limit", int), ("walk_target_dist", float), ("walk_target_theta", float), ("walk_target_x", float), ("walk_target_y", float)] I fixed the above by switching my development environment to python 3.7. Now I'm struggling to understand why this happens with black: (mbrl-lib-py37) ***@***.***:~/code/mbrl-lib$ git commit [WARNING] Unstaged files detected. [INFO] Stashing unstaged files to /home/dtch1997/.cache/pre-commit/patch1633184818-865. black....................................................................Failed - hook id: black - files were modified by this hook — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#135 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABEKAIRMV3YA7UR2N3O52EDUE4JJZANCNFSM5FGH3DOA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[dtch1997]

@luisenp thanks for asking! I've been quite pressed for time this week, but I am continuing to work on this. I will keep you posted when I make progress or need help

[dtch1997]

@luisenp I have enabled diagnostics/control_env.py to also work with PyBullet envs, as discussed previously. However, there are some issues:

• control_env.py implicitly requires that env states can be seamlessly transferred between different instances of the same env. There is a global env__ as well as an eval_env between which states must be transferred. I added a test to check for this functionality in tests/pybullet/test_utils.py.
• This is not true a priori in pybulletgym because each env has its own local pybullet client in which the state is saved. If we save a state in one instance of a Pybullet gym env and load it from another, we will get the pybullet.error: Couldn't restore state that you encountered.
• To get around this, I decided to save env state to a tempfile instead. This has the benefit of transferability between envs but seems to be quite slow. Running control_env.py on my own desktop gave me about 10s per step which I assume is very slow.

I am investigating whether it is possible to somehow transfer state between envs using pybullet.saveState and pybullet.restoreState. Previously it seems that getting / setting state is expected to be a very lightweight operation, so this would be a step towards that.

• This requires us to set clientServerId as described here
• However, internally, pybullet gym uses BulletClient (see here), which automatically overrides the clientServerId for all calls to pybullet functions. As a result I think it's impossible to manually override it to the serverId of a different instance of a Pybullet env, or at the very least requires a very invasive handling of Pybullet gym internal variables.

If you are alright with a somewhat hacky solution, it might be possible to skip using pybullet.saveState altogether and just manually save all relevant env variables. However this approach will become untenable for more complicated envs and also carries a high possibility of bugs.

Would like to hear if you have any recommendations on how to solve this. Thanks!

[luisenp]

Hi @dtch1997, thanks a lot for the update! I haven't taken a look but I'll try to do so today or tomorrow. I'm curious as to why it's needed to write the state to a file. Is there a reason why having a shared object in memory that can be copied for each environment wouldn't work in this case?

[dtch1997]

I'm curious as to why it's needed to write the state to a file. Is there a reason why having a shared object in memory that can be copied for each environment wouldn't work in this case?

@luisenp This is a limitation of the pybullet.saveState function. When it saves the state, it doesn't return a state object, but rather an integer which is an index into an internal table, so we do not have access to this in-memory object (which is implemened in C++ anyway). Each pybullet gym instance has its own pybullet client which has its own in-memory table. When we use pybullet.restoreState, the specified pybullet client simply restores that snapshot from its internal memory. What this means is that each pybullet gym env instance is capable of saving / restoring its own state in-memory, but this in-memory state cannot be transferred to a different instance.

Another potential idea is to use a in-memory file object to write to such as io.BytesIO. However, the pybullet.saveBullet function doesn't support writing to arbitrary Python file-like objects, it only accepts a string fileName as parameter and writes to that file on the disk. Hence the (not-ideal) use of tempfile, which I can't think of a workaround for.

[luisenp]

Thanks for the explanation, @dtch1997, that help me understand the issue better. I've been busy with other things, but I'll try to look into this today and see if I can give you any other suggestions.

[luisenp]

HI @dtch1997. I ran the code on my end and it's also quite slow for me, but at least it seems to be doing something. On the other hand, it's hard to say if the controller is functioning correctly given how slow it is.

I'm wondering if the easiest work around would be to stop relying on the state to be passed around, and instead pass the best action found after aggregating all trajectories, and update each local environment to move one step using that action. This assumes that the environment is deterministic and that setting the seed is all that's needed to set their initial state (both seem reasonable assumptions to me). It's less robust than passing the state directly, but it should work for the environments we have considered so far.

That being said, for the purpose of this PR, I think the a slow but working solution is good enough, so my proposal is to start porting mujoco and dmcontrol to the new class structure, and once that's done and control_env.py works with them, we can merge this PR into main. After that, if you are still interested, we can create a new PR to explore how to optimize this code. If we try the action synchronization I proposed above, it'd be easier to test and debug with mujoco-gym and dmcontrol, which we already know that work correctly.

How does this sound?

[dtch1997]

@luisenp sounds good, I will move on to integrating Mujoco / Dmcontrol.

[dtch1997]

@luisenp can you point me to a version of dm_control that works with Mujoco 2.0.x? The latest version on the Github repository is for Mujoco 2.1.0, which is incompatible with mujoco_py at the moment.

On a side note: DeepMind has recently acquired Mujoco and made it freely available. Going forward it is still unclear whether mujoco_py will be supported: see this issue. My hunch is that dm_control will become the preferred way to access Mujoco envs. Let me know if you have opinions on what you'd like to support moving forward

[luisenp]

Hi @dtch1997, my version of dm_control is 0.0.322773188.

I saw the Mujoco announcement, which was really good news! Moving forward, we'll likely more actively support the newer versions of Mujoco, but, as long as it remains practical, we should maintain backward compatibility with mujoco_py and Mujoco 2.0. This means that people that has already ran MBRL-LIb experiments on those environments, should be able to still do so in the future.

[dtch1997]

@luisenp Is there a pip-installable version of that version of dm_control? I'm not sure how to install it

(mbrl-lib) dtch1997@DESKTOP-AR4R24K:~/code/mbrl-lib$ python -m pip install dmcontrol==0.0.322773188
ERROR: Could not find a version that satisfies the requirement dmcontrol==0.0.322773188
ERROR: No matching distribution found for dmcontrol==0.0.322773188

[luisenp]

mmm, actually, I don't think there is, now that I think about it. Are you able to do the dm_control part using 2.1.0 (if you haven't already)? Then, once you have that working, for mujoco-py you can try to put things more or less in place w/o testing, then I can run on my side and fix any remaining errors. Is that feasible?

[dtch1997]

@luisenp no worries I realized I had a typo, was able to install dm_control. As of commit 2702fcc, mbrl/diagnostics/control_env.py should work with all 3 types of env now. Right now the last remaining step is to add support for the manipulation and pendulum type envs in PyBullet, then the PR is complete.

[luisenp]

Awesome, thanks a lot for your hard work!

[dtch1997]

Hi @luisenp, I have added support for the remaining Pybullet envs.

The freezing / unfreezing of generic Pybullet envs is done by pickling the env and then restoring it from the pickled version. This is not ideal but I decided to do this because:

• There is no unified and simple way to freeze a Pybullet env the way we want and I felt that implementing an optimized freeze method for each individual env would be out of the scope of this PR
• Since the saving of state to a tempfile is already fairly expensive I figured the pickling wouldn't add much runtime in comparison.
• In the case of locomotion envs, I am reusing work done by the previous person who worked on this PR and I can't claim to understand how they derived that code or whether it is truly correct.

I have tested these changes with tests/pybullet and ensured test coverage for all 3 basic types of Pybullet env (locomotion, manipulation, pendulum). Let me know how it looks.

[luisenp]

Thanks a lot @dtch1997. I'll take a look and see if I spot anything strange in the code, but, in general, if the code runs w/o issues I'd say the PR would be good to merge, and we can look into optimizations later. Sorry it's taken me long to give more feedback, or even confirm that it works. I'm really busy with another deadline, but I'll try to find some time to finalize this PR this week. Thanks!

[luisenp]

Hi @dtch1997. I'm really sorry that is taking me so long to review this, but my other commitments are taking all of my time. It might be hard for me to take a look at this until mid-November, but rest assured that this is a really nice contribution, and I'm looking forward to merge it in.

[dtch1997]

@luisenp let me know if there's anything else I need to do for this PR.

[luisenp]

Hi @dtch1997, thanks for reaching out and apologies again for the lack of input on my part. My other commitments should clear out by next week, so you can expect some final comments to merge this in from me by Wednesday 30 or so.

[luisenp]

Hi @dtch1997, PR merged! Let me know if you are still interested in the followups we had discussed and we can discuss a bit a plan for it. Thanks again for the contribution :)