Feature pybullet

[dtch1997]

Continuation of incomplete PR from https://github.com/facebookresearch/mbrl-lib/pull/87 This is my first time contributing to an open-source project so any advice is welcome, technical or otherwise

Types of changes

• [x] Docs change / refactoring / dependency upgrade
• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [x] 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

• [x] The documentation is up-to-date with the changes I made.
• [x] 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 cla@fb.com. 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]

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 https://github.com/facebookresearch/mbrl-lib/pull/135#discussion_r720686143 :

• 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 https://github.com/facebookresearch/mbrl-lib/pull/135#discussion_r720686143, 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]

I've implemented PybulletEnvHandler and MujocoEnvHandler based on the agreed interface.

PybulletEnvHandler has been tested with tests/pybullet/test_util.py. MujocoEnvHandler has not been tested at all.

Blockers:

• It seems that the main way of creating an env is to use hydra.utils.instantiate from a config object. How can create_handler be refactored to do it this way (instead of from a name)?

TODO:

• [x] Implement DmControlEnvHandler
• [x] Implement GymEnvHandler
• [x] Refactor mbrl.diagnostics to use handlers
  • [x] DatasetEvaluator
  • [x] FineTuner
  • [x] Visualizer

[dtch1997]

After thinking about it, I think make_env can be detached from EnvHandler since it is not engine-dependent. Then make_env can be modified to also return an EnvHandler of the appropriate type.

I'm not sure how to parse the cfg used in make_env to correctly determine the EnvHandler type though.

[dtch1997]

@luisenp would be helpful to get your input on the progress thus far and the suggested next steps

[luisenp]

Hi @dtch1997, sorry for the delay, I'll take a look at this today.

[dtch1997]

@luisenp good to hear back from you!

This is what I got when I ran pre-commit:

(mbrl-lib) dtch1997@DESKTOP-AR4R24K:~/code/mbrl-lib$ pre-commit run --all-files
black....................................................................Passed
flake8...................................................................Passed
mypy.....................................................................Failed
- hook id: mypy
- exit code: 2

tests/pybullet/test_util.py: error: Duplicate module named 'test_util' (also at 'tests/mujoco/test_util.py')
tests/pybullet/test_util.py: note: Are you missing an __init__.py? Alternatively, consider using --exclude to avoid checking one of them.
Found 1 error in 1 file (errors prevented further checking)

isort....................................................................Passed
isort....................................................................Passed

I fixed the flake8 error by deleting the problematic file. The relevant code has been fully refactored into pybullet_handler.py anyhow.

The mypy error seems more difficult to resolve since it seems to be complaining that mujoco and pybullet both have the same internal directory structure. I could fix it by 1) excluding the directory from mypy or 2) changing the file name but neither seem ideal. I found a relevant Git issue here: https://github.com/pre-commit/mirrors-mypy/issues/5

[dtch1997]

Also, my tests/pybullet failed with FAILED tests/pybullet/test_diagnostics.py::test_visualizer - RuntimeError: Tried to freeze an unsupported environment.

Yes this doesn't work yet. I need to refactor the mbrl.diagnostics module to use the new handlers, which requires instantiating the handler from a cfg object.

[dtch1997]

@luisenp should we use gym.make() as the entry point to PyBullet envs? Among other things, it makes things more difficult because all the PyBullet envs have intrinsic time limits which could be different from the time limit specified in cfg.overrides.trial_length.

The way I have it implemented now, the entry point in env_cfg is the raw env which enables wrapping it with the correct TimeLimit in EnvHandler.make_env.

[luisenp]

@luisenp should we use gym.make() as the entry point to PyBullet envs? Among other things, it makes things more difficult because all the PyBullet envs have intrinsic time limits which could be different from the time limit specified in cfg.overrides.trial_length.

The way I have it implemented now, the entry point in env_cfg is the raw env which enables wrapping it with the correct TimeLimit in EnvHandler.make_env.

Oh sorry, I didn't mean to imply we should use gym.makeas the entry point, what I was trying to illustrate is that we should be able to infer what the right EnvHandler from the object itself. Now that I think about it, for PyBullet env_cfg._target_ will probably look something like pybulletgym.envs.something.something, no? If so, this would be easy to parse to determine the right type of EnvHandler.

[dtch1997]

Now that I think about it, for PyBullet env_cfg.target will probably look something like pybulletgym.envs.something.something, no?

Yup you're exactly right, that's how I've implemented it currently

[luisenp]

@luisenp good to hear back from you!

This is what I got when I ran pre-commit:

(mbrl-lib) dtch1997@DESKTOP-AR4R24K:~/code/mbrl-lib$ pre-commit run --all-files
black....................................................................Passed
flake8...................................................................Passed
mypy.....................................................................Failed
- hook id: mypy
- exit code: 2

tests/pybullet/test_util.py: error: Duplicate module named 'test_util' (also at 'tests/mujoco/test_util.py')
tests/pybullet/test_util.py: note: Are you missing an __init__.py? Alternatively, consider using --exclude to avoid checking one of them.
Found 1 error in 1 file (errors prevented further checking)

isort....................................................................Passed
isort....................................................................Passed

I fixed the flake8 error by deleting the problematic file. The relevant code has been fully refactored into pybullet_handler.py anyhow.

The mypy error seems more difficult to resolve since it seems to be complaining that mujoco and pybullet both have the same internal directory structure. I could fix it by 1) excluding the directory from mypy or 2) changing the file name but neither seem ideal. I found a relevant Git issue here: pre-commit/mirrors-mypy#5

I'm a bit more lenient when it comes to test code conventions, so changing the file name wouldn't be too bad. On the other hand, it seems that adding __init__.py to both test folders resolves the mypy issue.

[dtch1997]

On the other hand, it seems that adding init.py to both test folders resolves the mypy issue.

Oh that's neat! Implemented in 2dd76c6

[dtch1997]

As of commit 2dd76c6 :

• All the tests in tests/pybullet should work
• All the linter checks should also pass

[luisenp]

Hi @dtch1997, let me know when the code is ready for another pass of review :) (at your own convenience of course)

[dtch1997]

@luisenp sure! I think I've addressed all the points brought up so far and all the tests should run. While I try to figure out how the other types of PyBullet env should be frozen, pleaese do have another pass at the PR in its current state.

[dtch1997]

Before we do that, I think a good test for the PyBullet part would be to modify diagnostics/control_env.py to also work with PyBullet, so that we can test the rollout logic using a planner on ground truth dynamics.

@luisenp I'm currently implementing this but I'm stuck on an error when I try to run the mbrl.diagnostics.control_env script. It seems like the problem is with the state getting and setting but I tested this in tests/pybullet/test_utils.py and it works without a problem. This is intended as more of a status update, and I'm still actively working on this, but feel free to share insight if you have any.

One thought I had was that eval_env.reset() is not being called appropriately somehow. As documented in https://github.com/benelot/pybullet-gym/blob/master/pybulletgym/envs/roboschool/envs/env_bases.py, env._p is only initialized when env.reset is called. However, eval_env.reset() is being called in mbrl.diagnostics.control_env:91. Also, the problem doesn't go away when I add a similar eval_env.reset() call in mbrl.diagnostics.control_env:160, so this seems unlikely.

Full stack trace:

(mbrl-lib) dtch1997@DESKTOP-AR4R24K:~/code/mbrl-lib$ python -m mbrl.diagnostics.control_env --env pybulletgym___HopperPyBulletEnv-v0
pybullet build time: Sep 20 2021 20:33:29
/home/dtch1997/anaconda3/envs/mbrl-lib/lib/python3.8/site-packages/gym/logger.py:30: UserWarning: WARN: Box bound precision lowered by casting to float32
  warnings.warn(colorize('%s: %s'%('WARN', msg % args), 'yellow'))
WalkerBase::__init__
argv[0]=
argv[0]=
/home/dtch1997/anaconda3/envs/mbrl-lib/lib/python3.8/site-packages/hydra/utils.py:32: UserWarning: `OmegaConf.is_none()` is deprecated, see https://github.com/omry/omegaconf/issues/547
  if OmegaConf.is_none(config):
pybullet build time: Sep 20 2021 20:33:29
/home/dtch1997/anaconda3/envs/mbrl-lib/lib/python3.8/site-packages/gym/logger.py:30: UserWarning: WARN: Box bound precision lowered by casting to float32
  warnings.warn(colorize('%s: %s'%('WARN', msg % args), 'yellow'))
WalkerBase::__init__
multiprocessing.pool.RemoteTraceback:
"""
Traceback (most recent call last):
  File "/home/dtch1997/anaconda3/envs/mbrl-lib/lib/python3.8/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "/home/dtch1997/code/mbrl-lib/mbrl/diagnostics/control_env.py", line 58, in evaluate_sequence_fn
    handler__.set_env_state(current_state, env)
  File "/home/dtch1997/code/mbrl-lib/mbrl/util/pybullet.py", line 180, in set_env_state
    return PybulletEnvHandler._set_env_state_locomotion(state, env)
  File "/home/dtch1997/code/mbrl-lib/mbrl/util/pybullet.py", line 211, in _set_env_state_locomotion
    env._p.restoreState(state_id)
AttributeError: 'HopperBulletEnv' object has no attribute '_p'
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/dtch1997/anaconda3/envs/mbrl-lib/lib/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/home/dtch1997/anaconda3/envs/mbrl-lib/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/home/dtch1997/code/mbrl-lib/mbrl/diagnostics/control_env.py", line 183, in <module>
    plan = controller.optimize(
  File "/home/dtch1997/code/mbrl-lib/mbrl/planning/trajectory_opt.py", line 558, in optimize
    best_solution = self.optimizer.optimize(
  File "/home/dtch1997/code/mbrl-lib/mbrl/planning/trajectory_opt.py", line 172, in optimize
    values = obj_fun(population)
  File "/home/dtch1997/code/mbrl-lib/mbrl/diagnostics/control_env.py", line 170, in trajectory_eval_fn
    return evaluate_all_action_sequences(
  File "/home/dtch1997/code/mbrl-lib/mbrl/diagnostics/control_env.py", line 48, in evaluate_all_action_sequences
    res = [res_obj.get() for res_obj in res_objs]
  File "/home/dtch1997/code/mbrl-lib/mbrl/diagnostics/control_env.py", line 48, in <listcomp>
    res = [res_obj.get() for res_obj in res_objs]
  File "/home/dtch1997/anaconda3/envs/mbrl-lib/lib/python3.8/multiprocessing/pool.py", line 771, in get
    raise self._value
AttributeError: 'HopperBulletEnv' object has no attribute '_p'

[luisenp]

Hi @dtch1997, thanks for the update! I just took a quick look, and given the line where the error is thrown, it's more likely that the missing reset would be an issue for the environments used for planning, a copy of which each process would have to use for planning, and not in eval_env, which is the environment that lives in the main thread were the actions are being executed.

If I add the line env__.reset()at the end of function init() (line 31), the error changes to

WalkerBase::__init__
argv[0]=
argv[0]=
pybullet build time: Jul  9 2021 21:27:33
/private/home/lep/.conda/envs/mbrl/lib/python3.7/site-packages/gym/logger.py:30: UserWarning: WARN: Box bound precision lowered by casting to float32
  warnings.warn(colorize('%s: %s'%('WARN', msg % args), 'yellow'))
WalkerBase::__init__
argv[0]=
argv[0]=
b3Warning[examples/SharedMemory/PhysicsDirect.cpp,1204]:
restoreState failedb3Warning[examples/SharedMemory/PhysicsDirect.cpp,1204]:
restoreState failedmultiprocessing.pool.RemoteTraceback:
"""
Traceback (most recent call last):
  File "/private/home/lep/.conda/envs/mbrl/lib/python3.7/multiprocessing/pool.py", line 121, in worker
    result = (True, func(*args, **kwds))
  File "/private/home/lep/code/mbrl/mbrl/diagnostics/control_env.py", line 59, in evaluate_sequence_fn
    handler__.set_env_state(current_state, env)
  File "/private/home/lep/code/mbrl/mbrl/util/pybullet.py", line 180, in set_env_state
    return PybulletEnvHandler._set_env_state_locomotion(state, env)
  File "/private/home/lep/code/mbrl/mbrl/util/pybullet.py", line 211, in _set_env_state_locomotion
    env._p.restoreState(state_id)
pybullet.error: Couldn't restore state.
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "mbrl/diagnostics/control_env.py", line 185, in <module>
    trajectory_eval_fn, callback=compute_population_stats
  File "/private/home/lep/code/mbrl/mbrl/planning/trajectory_opt.py", line 561, in optimize
    callback=callback,
  File "/private/home/lep/code/mbrl/mbrl/planning/trajectory_opt.py", line 172, in optimize
    values = obj_fun(population)
  File "mbrl/diagnostics/control_env.py", line 174, in trajectory_eval_fn
    current_state__,
  File "mbrl/diagnostics/control_env.py", line 49, in evaluate_all_action_sequences
    res = [res_obj.get() for res_obj in res_objs]
  File "mbrl/diagnostics/control_env.py", line 49, in <listcomp>
    res = [res_obj.get() for res_obj in res_objs]
  File "/private/home/lep/.conda/envs/mbrl/lib/python3.7/multiprocessing/pool.py", line 657, in get
    raise self._value
pybullet.error: Couldn't restore state.

[luisenp]

Hi @dtch1997, any updates on this? Let me know if there is anything else I can do to help. Thanks!

[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 :)