GAIL и GRPO: seed= не значил ничего

CHANGELOG.md

@@ -19,6 +19,21 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   the freeze semantics of Stable-Baselines3's `VecNormalize`.
 
 ### Fixed
+- `GRPO(seed=...)` was not reproducible either, and for the first of the same two
+  reasons: `_rollout_episode` resets the env once per episode and never with a seed, so
+  every episode started from OS entropy. `GRPO(seed=0)` returned 91.2 and then 94.7; it
+  now returns one number. Seeded once at the top of `learn`, as `off_policy`, `tabular`
+  and `sac_discrete` already do. An audit of the other algorithms found no third case:
+  every other training loop seeds its first reset, and the unseeded resets that follow
+  draw from a seeded stream.
+- `GAIL(seed=...)` was not reproducible. Two sources, both of them data rather than
+  weights, and both invisible to `set_seed` for the reason its own docstring gives: the
+  policy rollouts start from `self.env`, whose `np.random.default_rng()` is built without
+  a seed, and the policy dataset the discriminator trains against was rebuilt each
+  iteration as `TransitionDataset(...)` with no seed, so its minibatch indices came from
+  OS entropy on every discriminator epoch. Three fresh processes at seed 0 returned
+  421.70, 496.20 and 385.10; they now return one number. This is what made
+  `test_gail_imitates_expert` fail about one run in ten.
 - `BC` and `GAIL` raised on any machine with a GPU. Both take `device="auto"`, which puts
   the network on CUDA when there is one, while `TransitionDataset` defaults to `"cpu"` and
   `collect_expert_dataset` never passes anything else — so the documented way of using them

src/decisionrl/algorithms/grpo.py

@@ -200,6 +200,15 @@ def _update(self, obs_all, act_all, logp_all, adv_all) -> dict:
 
     def learn(self, total_steps: int, callback=None, log_interval: int = 1) -> "GRPO":
         self._total_timesteps = self.num_timesteps + total_steps
+        # Seed the env once here, as off_policy, tabular and sac_discrete do at
+        # the top of their own `learn`. `_rollout_episode` resets per episode
+        # without a seed, and an env owns an `np.random.default_rng()` built
+        # without one, so until this line GRPO drew every episode's starting
+        # state from OS entropy: `GRPO(seed=0)` returned 91.2 and then 94.7.
+        # A reset keeps the generator it was handed, so the unseeded resets
+        # that follow draw from a seeded stream.
+        if self.seed is not None:
+            self.env.reset(seed=self.seed)
         if callback is not None:
             callback.on_training_start(self)
         returns_window: deque = deque(maxlen=100)

src/decisionrl/imitation.py

@@ -195,6 +195,17 @@ def __init__(self, env: Env, expert_dataset: TransitionDataset, learning_rate: f
         self.policy = PPO(self.wrapped, learning_rate=learning_rate, n_steps=n_steps,
                           hidden_sizes=hidden_sizes, device=device, seed=seed, **ppo_kwargs)
         self.rng = np.random.default_rng(seed)
+        self.seed = seed
+        # The policy rollouts below draw their starting states from this env,
+        # and an env owns an `np.random.default_rng()` built without a seed --
+        # `set_seed` cannot reach it, as its own docstring says. Without this
+        # line every iteration of `learn` starts from OS entropy, so `seed=`
+        # names nothing: three fresh processes at seed 0 returned 421.70,
+        # 496.20 and 385.10 on one machine. Seeding here is enough for all of
+        # them: `reset()` keeps the generator it was given, so the unseeded
+        # resets that follow draw from a seeded stream.
+        if seed is not None:
+            self.env.reset(seed=seed)
 
     def _collect_policy_transitions(self, n: int):
         obs_l, act_l = [], []
@@ -209,8 +220,14 @@ def _collect_policy_transitions(self, n: int):
         return np.asarray(obs_l, dtype=np.float32), np.asarray(act_l)
 
     def _update_discriminator(self, pol_obs, pol_act, epochs, batch_size):
+        # Drawn from this agent's own generator rather than left unseeded: the
+        # dataset is rebuilt every iteration and samples its minibatch indices
+        # from whatever generator it was given, so `TransitionDataset(...)`
+        # with no seed means `np.random.default_rng()` and OS entropy on every
+        # discriminator epoch of every iteration.
         pol = TransitionDataset(pol_obs, pol_act, np.zeros(len(pol_obs)), pol_obs,
-                                np.zeros(len(pol_obs)), device=str(self.device))
+                                np.zeros(len(pol_obs)), device=str(self.device),
+                                seed=int(self.rng.integers(2**32)))
         losses = []
         for _ in range(epochs):
             # The expert dataset comes from the caller and is on whatever

tests/test_grpo.py

@@ -6,6 +6,7 @@
 from decisionrl.algorithms import GRPO
 from decisionrl.envs import CartPole
 from decisionrl.training import evaluate_policy
+from decisionrl.utils import set_seed
 
 
 def test_grpo_predicts_valid_actions(quiet_logger):
@@ -37,3 +38,27 @@ def test_grpo_learns_cartpole(quiet_logger):
     agent.learn(30_000)
     mean_return, _ = evaluate_policy(agent, CartPole(), n_episodes=10, seed=100)
     assert mean_return > 150.0
+
+
+def test_grpo_is_reproducible_from_its_seed(quiet_logger):
+    """Two GRPO runs at one seed have to give one answer.
+
+    `_rollout_episode` resets the env once per episode and never with a seed,
+    and an env owns an `np.random.default_rng()` built without one, so every
+    episode started from OS entropy: `GRPO(seed=0)` returned 91.2 and then 94.7.
+    `set_seed` cannot reach that generator, which is what its own docstring
+    warns about.
+
+    `set_seed` is called per run here because that is the contract as it stands
+    -- an agent's networks are initialised from global torch state rather than
+    from its own seed, in every algorithm in this package -- so without it this
+    would be testing something nobody has promised yet.
+    """
+    def once():
+        set_seed(0)
+        agent = GRPO(CartPole(), seed=0, logger=quiet_logger)
+        agent.learn(1500)
+        return evaluate_policy(agent, CartPole(), n_episodes=5, seed=100)[0]
+
+    first, second = once(), once()
+    assert first == second, f"GRPO(seed=0) gave {first} and then {second}"

tests/test_imitation.py

@@ -7,6 +7,7 @@
 from decisionrl.envs import CartPole
 from decisionrl.imitation import BC, GAIL, DAgger, GAILDiscriminator, collect_expert_dataset
 from decisionrl.training import evaluate_policy
+from decisionrl.utils import set_seed
 
 
 def _expert(o):
@@ -126,3 +127,42 @@ def test_bc_trains_when_the_dataset_is_on_another_device(quiet_logger):
     bc = BC(CartPole(), seed=0, logger=quiet_logger)                  # cuda
     assert data.device != bc.device
     bc.train(data, n_iters=3, batch_size=16)
+
+
+def test_gail_is_reproducible_from_its_seed(quiet_logger):
+    """Two GAIL runs at one seed have to give one answer.
+
+    They did not. `seed=` named nothing, for two reasons that `set_seed` cannot
+    reach and its own docstring warns about:
+
+    * the policy rollouts start from `self.env`, and an env owns an
+      `np.random.default_rng()` built without a seed, so every iteration of
+      `learn` drew its starting states from OS entropy;
+    * the policy dataset the discriminator trains against was rebuilt each
+      iteration as `TransitionDataset(...)` with no seed, so its minibatch
+      indices came from OS entropy too, on every discriminator epoch.
+
+    Three fresh processes at seed 0 returned 421.70, 496.20 and 385.10 before
+    this; afterwards they return one number, three times.
+
+    `set_seed` is called per run, which is what conftest does per test, because
+    that is the contract as it stands: an agent's networks are initialised from
+    global torch state rather than from its own `seed`, here and in every other
+    algorithm in this package. So `seed=` currently means "reproducible given
+    the same global state", not "reproducible". Making it mean the second is a
+    change to every agent, not to this one, and is not what this test is for --
+    but without saying so, the `set_seed` below looks like ceremony.
+
+    Short on purpose -- this is about determinism, not about learning, and the
+    full-size run is `test_gail_imitates_expert` above.
+    """
+    def once():
+        set_seed(0)
+        data = collect_expert_dataset(CartPole(), _expert, 2000, seed=0)
+        gail = GAIL(CartPole(), data, n_steps=512, batch_size=64, n_epochs=2, seed=0,
+                    logger=quiet_logger)
+        gail.learn(iterations=2, steps_per_iter=1024, disc_epochs=3, disc_batch=64)
+        return evaluate_policy(gail, CartPole(), n_episodes=10, seed=100)[0]
+
+    first, second = once(), once()
+    assert first == second, f"GAIL(seed=0) gave {first} and then {second}"