Hindsight Experience Replay (HER) - Reloaded

[araffin]

Refactored HER, starting from scratch. All previous code was removed.

Coverage bumps from 76% to 82% =)!

+ removed unused dependencies
+ update doc examples
+ add some additional exploration hacks for DDPG/SAC
+ update custom policy doc

TODO:

• flipping bit test env
• DQN support
• SAC support
• DDPG support
• VecEnv support
• Clean up
• Documentation
• 4 different goal selection strategies
• performance check on highway-envs (see DDPG + HER - ParkingEnv-v0 Farama-Foundation/HighwayEnv#15)
• perf check on robotics env (Mujoco)
• support Discrete obs
• find and fix bug with VecEnv

The repo for Mujoco experiments: https://github.com/araffin/her-experiments
now using rl baselines zoo (HER-support branch): https://github.com/araffin/rl-baselines-zoo/tree/HER-support

Mujoco Robotics envs

I would be confident about the implementation when we'll manage to solve FetchPush. One current drawback is that we do not support multiprocessing, which helps exploration.

From the paper of HER (OpenAI): 1 epoch = 1900 episodes = 1900 x 50 = 95 000 timesteps

• FetchReach-v1: ok for DDPG and SAC (20k steps)
• FetchPush-v1: ok for DDPG (8 workers, 2e6 steps), ok for SAC with early stopping (> 2e6 steps)

Closes #198
Closes #350

[araffin]

@hill-a @erniejunior @AdamGleave @ccolas The branch is now ready for review, I'm just waiting @ccolas results next week to ensure that it can solve Mujoco envs.

Known caveat: VecEnvWrapper are currently not supported but VecEnv are (I did not come up with an elegant fix yet)

[araffin]

@hill-a @ccolas I just managed to make HER + SAC work on FetchReach, there seem to be a bug with VecEnv. Until I find the bug, I'll deactivate the VecEnv for SAC.
I'll push that soon to the rl zoo on HER-Support branch.

[araffin]

Good news, I made HER + DDPG worked on my laptop (I had to tune the hyperparams so it does not blow up my RAM).
The main trick was to have several workers (4, i think it should work even better with more).
The only trick from OpenAI I did not include: the l2 penalty on the action.

So now, when I'll find and fix the bug with the VecEnvs, we can merge this branch!

[araffin]

@hill-a @ccolas The performance check were successful on HER + DDPG (thanks to @keshaviyengar ), and HER + SAC (with a few trick to make it work with one worker).
Only the VecEnv bug remaining...

[araffin]

I found (and fixed) the bug! I was expecting keys in the wrong order.

[araffin]

unfortunately, it looks like I introduced another bug...

[araffin]

The bug may comes from gym...
If you check observation.spaces.keys() you get ['achieved_goal', 'desired_goal', 'observation']
but if you check env.reset().keys(), you get ['observation', 'achieved_goal', 'desired_goal']...

[araffin]

@hill-a @erniejunior @AdamGleave @ccolas the PR is ready for final review.
Performance have been checked, bug fixed for VecEnv and support for Discrete obs added.

[hill-a]

Only slight change:

maybe add replay_wrapper=None to OffPolicyRLModel.learn?
https://github.com/hill-a/stable-baselines/blob/HER-2/stable_baselines/common/base_class.py#L705

[araffin]

maybe add replay_wrapper=None to OffPolicyRLModel.learn?

Will do that ;)

[hill-a]

Awsome work, love that high coverage percent! ^^
LGTM.