diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e877f9..8682399 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/decisionrl/algorithms/grpo.py b/src/decisionrl/algorithms/grpo.py index 39d84e7..4249884 100644 --- a/src/decisionrl/algorithms/grpo.py +++ b/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) diff --git a/src/decisionrl/imitation.py b/src/decisionrl/imitation.py index 96f66b1..d8fa869 100644 --- a/src/decisionrl/imitation.py +++ b/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 diff --git a/tests/test_grpo.py b/tests/test_grpo.py index 4bac5bf..6d47cac 100644 --- a/tests/test_grpo.py +++ b/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}" diff --git a/tests/test_imitation.py b/tests/test_imitation.py index 463e127..1e97bab 100644 --- a/tests/test_imitation.py +++ b/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}"