[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

@@ -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).")

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,

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,