opened
utkarshjp7
username_0
This PR removes `optimizer_args` pattern from torch algorithm constructors. All torch algorithms now accept a constructed optimizer instead of an optimizer class.
Remove optimizer_args from torch algos
issue
created
krzentner
username_1
Ryan and I discussed this for some time. We eventually came to the following argument:
  - In most of garage, we expose the important arguments in easy-to-reach places (e.g. `hidden_layers` arguments to policies).
  - We should also do that in algorithms for consistency, and because we feel this way of providing ease-of-use is best for garage's users.
  - For most users, the most important parameter is the learning rate. In particular, the learning rate is a more important parameter than the algorithm itself.
  - Therefore the learning rate parameters (e.g. `lr`, `max_kl`, `policy_lr`, etc.) should be exposed to the users as arguments to the algorithm, so that it can be specified without specifying the optimizer itself.
  - There should still be a way of passing a custom optimizer for algorithms where doing so makes sense (i.e. gradient methods that aren't TRPO).

After thinking about what that specifically looks like, we think that the following design would be best:
 - For algorithms where the optimizer cannot reasonably be changed from the default (TRPO), all hyper-parameters of the optimizer should be exposed as arguments on the algorithm. It should not be possible to specify the optimizer as an argument. In other words, we should remove the `optimizer` argument from TRPO, and keep / add the `max_kl` argument.
 - For algorithms where it is reasonable to set the optimizer to most torch optimizers, there should be an `optimizer` argument which specifies the type of the optimizer. There should also be a parameter to set that optimizer's important hyperparameters (e.g. `lr`, `policy_lr`, `q_lr`, etc.).
 - For algorithms where the optimizer can be specified, it should also be possible to pass a tuple of (optimizer type, optimizer hyper parameters) as the `optimizer`. If a  tuple is passed, the second parameter must be a dictionary, and the other arguments specifying hyper-parameters for that algorithm (e.g. `policy_lr`) should not be passed. If any of them are passed, the algorithm should raise `ValueError`.

By the way, this probably also involves writing a utility function, which we think should probably be in something like this:
```python
# In src/garage/torch/algos/trpo.py:

class TRPO:
    def __init__(self, ..., max_kl=0.2):
        ...
    ...

# In src/garage/torch/algos/_utils.py

class _Default:
    pass

def _make_optimizer(optimizer_type, module, **kwargs):
    if isinstance(optimizer_type, tuple):
        opt_type, opt_args = optimizer_type
        for name, arg in kwargs.items():
            if not isinstance(arg, Default):
                raise ValueError('Should not specify {} and explicit optimizer args'.format(name))
        return opt_type(module.parameters(), **opt_args)
    else:
        return policy_optimizer(module.parameters(), **kwargs)

# In src/garage/torch/algos/__init__.py
from garage.torch.algos._utils import _Default, _make_optimizer
from garage.torch.algos.ddpg import DDPG
...
__all__ = ... # Should not include _Default, etc.

# In src/garage/torch/algos/ddpg.py
from garage.torch.algos import _Default, _make_optimizer

class DDPG:
    def __init__(self, policy_optimizer=torch.optim.Adam, policy_lr=_Default(1e-3), ...):
        ...
        self.policy_opt = _make_optimizer(policy_optimizer, self.policy, lr=policy_lr)
        ...
    ...
```
comment
created
ryanjulian
username_2
@username_0 when you do this, can you also file an issue to make this consistent in the TF and NP trees?
comment