Implement HER

[megan-klaiber]

Description

HER inherits from OffPolicyAlgorithm and takes the model as an argument. It also implements its own collect_rollout function.
HER can operate in two modes for now. online_sampling being True or False. If True, HER samples are added while sampling, otherwise they are added at the end of an episode. If online sampling is used, a custom HerReplayBuffer will be used which stores the transitions episode-wise.

Motivation and Context

• I have raised an issue to propose this change (required for new features and bug fixes)

closes #8

Types of changes

• 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)
• Documentation (update in the documentation)

Checklist:

• I've read the CONTRIBUTION guide (required)
• I have updated the changelog accordingly (required).
• My change requires a change to the documentation.
• I have updated the tests accordingly (required for a bug fix or a new feature).
• I have updated the documentation accordingly.
• I have reformatted the code using make format (required)
• I have checked the codestyle using make check-codestyle and make lint (required)
• I have ensured make pytest and make type both pass. (required)

Missing:

• Handle info dict in reward computation
• Refactor offline version to use her replay buffer
• VecNormalize support
• more tests for VecNormalize support
• benchmark sb2 vs sb3 on parking env
• test to check errors for max episode length

Note: we are using a maximum length of 127 characters per line

Results

Results on https://github.com/eleurent/highway-env

her_parking.pdf

her.pdf

[araffin]

you will need also to update the documentation: add HER to the module and to the examples (you can mostly copy-paste what was done in SB2 documentation ;))

[araffin]

sde support should not be here

[araffin]

you should wrap it only afterward and check if the wrapper is needed or not

[araffin]

Suggested change

	), "Invalid goal selection strategy," "please use one of {}".format(list(GoalSelectionStrategy))
	), f"Invalid goal selection strategy, please use one of {list(GoalSelectionStrategy))}"

we require python 3.6+, so you can use f-strings

[araffin]

you need that because HER inherits from the off-policy class? I would make it inherit from the BaseAlgorithm then.
It seems that you are initializing two models (and two replay buffers, including one that you don't use)

[araffin]

or maybe keep OffpolicyAlgorithm as base class but initialize empty buffer, so you can re-use learn() from the base class

[araffin]

?

[araffin]

missing comment, looks weird compared to what is described in the docstring

[araffin]

please avoid one character variable, you can use obs_ instead

[araffin]

this logic cannot be shared with the "offline" version?

[araffin]

you don't need to re-implement those as they are already in the base wrapper class, no?

[araffin]

Would be clearer to use NamedTuple (cf what is done for the replay buffer)

[araffin]

after thinking more about it, I think we need to define __get__attr_ to automaticaly retrieve the attribute from self.model if present. This would allow to write directly self._sample_action() .

[araffin]

Please avoid name without meaning: achieved_goal instead of ag ;)

[araffin]

can be simplified

[araffin]

Maybe make self.n_transitions_stored private and create a getter using @property

[araffin]

you need to re-initialize the number of transitions stored too, no?

[araffin]

is there no quicker way of doing that? (without duplicating too much code)

I would store HER specific arguments in the model (self.model.dict) , see what is done in SB2.

[araffin]

this is not properly defined in the HER replay buffer

[araffin]

LGTM =)

Tested on simulated envs and on a real robot, time to merge now.

[avandekleut]

LGTM =)

Tested on simulated envs and on a real robot, time to merge now.
@araffin what environments and hyperparameters did you use? I'm trying to get this working on the Fetch environments.

[araffin]

@araffin what environments and hyperparameters did you use? I'm trying to get this working on the Fetch environments.

depends on which Fetch please look at the rl zoo: https://github.com/DLR-RM/rl-baselines3-zoo.
For env that are not FetchReach, multiprocessing is usually required but currently not implemented, otherwise you can take a look at the SB2 zoo:
araffin/rl-baselines-zoo#53
and
araffin/rl-baselines-zoo#50

[phiresky]

It looks like the code added in this PR seems to break normal envs with dict observation space since it assumes that whenever the observation space is a dictionary the user wants HER:

 File ".../lib/python3.8/site-packages/stable_baselines3/common/base_class.py", line 203, in _wrap_env
    env = ObsDictWrapper(env)
  File ".../lib/python3.8/site-packages/stable_baselines3/common/vec_env/obs_dict_wrapper.py", line 28, in __init__
    self.obs_dim = venv.observation_space.spaces["observation"].shape[0]
KeyError: 'observation'

specifically, this code:

        # check if wrapper for dict support is needed when using HER
        if isinstance(env.observation_space, gym.spaces.dict.Dict):
            env = ObsDictWrapper(env)

doesn't verify at all that HER is what I want, and assumes the dict has a specific purpose, breaking training any envs with dict obs space

[araffin]

It looks like the code added in this PR seems to break normal envs with dict observation space since it assumes that whenever the observation space is a dictionary the user wants HER:

Please read #216
Dict observation spaces other than GoalEnv are currently not supported by SB3 (but there is an active PR for that here #243)

[phiresky]

@araffin thanks for the info and links. might be good to throw a more readable error in that case. For now this works fine for me:

class FlattenVecWrapper(VecEnvWrapper):
    def __init__(self, env):
        super().__init__(env)
        self.observation_space = gym.spaces.flatten_space(self.venv.observation_space)

    def reset(self, **kwargs):
        observation = self.venv.reset(**kwargs)
        return self.observation(observation)

    def step_wait(self):
        observation, reward, done, info = self.venv.step_wait()
        return self.observation(observation), reward, done, info

    def observation(self, observation):
        return [gym.spaces.flatten(self.venv.observation_space, o) for o in observation]