From 9896df77997be7038b7924c4469c9f822456d49a Mon Sep 17 00:00:00 2001 From: Eric Liang Date: Fri, 22 Feb 2019 11:18:51 -0800 Subject: [PATCH] [rllib] Guard against PPO value function not training with RNN models (#4037) * better lstm settings * 1.0 * docs * warn on truncate * clarify * Update ppo_policy_graph.py * Update ppo_policy_graph.py * Update ppo_policy_graph.py --- python/ray/rllib/agents/ppo/ppo.py | 14 ++++++++++---- python/ray/rllib/agents/ppo/ppo_policy_graph.py | 12 +++++++++++- python/ray/rllib/examples/cartpole_lstm.py | 2 ++ 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/python/ray/rllib/agents/ppo/ppo.py b/python/ray/rllib/agents/ppo/ppo.py index 3a2404b90..3798f63ff 100644 --- a/python/ray/rllib/agents/ppo/ppo.py +++ b/python/ray/rllib/agents/ppo/ppo.py @@ -151,7 +151,8 @@ class PPOAgent(Agent): if (self.config["batch_mode"] == "truncate_episodes" and not self.config["use_gae"]): raise ValueError( - "Episode truncation is not supported without a value function") + "Episode truncation is not supported without a value " + "function. Consider setting batch_mode=complete_episodes.") if (self.config["multiagent"]["policy_graphs"] and not self.config["simple_optimizer"]): logger.info( @@ -159,7 +160,12 @@ class PPOAgent(Agent): "by the multi-GPU optimizer. Consider setting " "simple_optimizer=True if this doesn't work for you.") if self.config["observation_filter"] != "NoFilter": - # TODO(ekl): consider setting the default to be NoFilter logger.warning( - "By default, observations will be normalized with {}".format( - self.config["observation_filter"])) + "By default, observations will be normalized with {}. ".format( + self.config["observation_filter"]) + + "If you are using image or discrete type observations, " + "consider disabling this with observation_filter=NoFilter.") + if not self.config["vf_share_layers"]: + logger.warning( + "By default, the value function will NOT share layers with " + "the policy model (vf_share_layers=False).") diff --git a/python/ray/rllib/agents/ppo/ppo_policy_graph.py b/python/ray/rllib/agents/ppo/ppo_policy_graph.py index 611ba299f..3159de562 100644 --- a/python/ray/rllib/agents/ppo/ppo_policy_graph.py +++ b/python/ray/rllib/agents/ppo/ppo_policy_graph.py @@ -2,6 +2,7 @@ from __future__ import absolute_import from __future__ import division from __future__ import print_function +import logging import tensorflow as tf import ray @@ -13,6 +14,8 @@ from ray.rllib.models.catalog import ModelCatalog from ray.rllib.utils.annotations import override from ray.rllib.utils.explained_variance import explained_variance +logger = logging.getLogger(__name__) + class PPOLoss(object): def __init__(self, @@ -189,7 +192,14 @@ class PPOPolicyGraph(LearningRateSchedule, TFPolicyGraph): # mean parameters and standard deviation parameters and # do not make the standard deviations free variables. vf_config["free_log_std"] = False - vf_config["use_lstm"] = False + if vf_config["use_lstm"]: + vf_config["use_lstm"] = False + logger.warning( + "It is not recommended to use a LSTM model with " + "vf_share_layers=False (consider setting it to True). " + "If you want to not share layers, you can implement " + "a custom LSTM model that overrides the " + "value_function() method.") with tf.variable_scope("value_function"): self.value_function = ModelCatalog.get_model({ "obs": obs_ph, diff --git a/python/ray/rllib/examples/cartpole_lstm.py b/python/ray/rllib/examples/cartpole_lstm.py index ddc89c47e..1a70a0d04 100644 --- a/python/ray/rllib/examples/cartpole_lstm.py +++ b/python/ray/rllib/examples/cartpole_lstm.py @@ -169,6 +169,8 @@ if __name__ == "__main__": configs = { "PPO": { "num_sgd_iter": 5, + "vf_share_layers": True, + "vf_loss_coeff": 0.0001, }, "IMPALA": { "num_workers": 2,