From 37b360a5242a0f591af9a78cc2916302aa36cadf Mon Sep 17 00:00:00 2001 From: Denis_Drobyshev Date: Sun, 20 Sep 2026 23:00:04 +0300 Subject: [PATCH 1/2] GAIL(seed=0) named nothing Three fresh processes, same seed, same machine: 421.70 496.20 385.10 and two calls inside one process gave 185.3 and 442.9. `test_gail_imitates_expert` asserts a threshold on that, which is why it fails about one run in ten and has been blocking every pull request here. Neither source is the weights. Both are data, and both are invisible to `set_seed` for the reason its own docstring gives -- it cannot reach a generator an object built for itself: * the policy rollouts in `_collect_policy_transitions` start from `self.env`, and an env owns an `np.random.default_rng()` with no 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 of every iteration. Seeding the env once in `__init__` is enough for the first: `reset()` keeps the generator it was handed, so the unseeded resets that follow draw from a seeded stream. The dataset takes a seed drawn from this agent's own generator, so it still differs per iteration and still reproduces. Both are necessary and neither is sufficient. Measured, two runs in one process at the same seed: both fixes 128.8 and 128.8; env seeding reverted 124.9 and 154.8; dataset seeding reverted 120.0 and 75.6. At the real test's parameters three fresh processes now return 380.0, 380.0, 380.0 against a threshold of 200. The new test is sized to catch both -- the first version of it was short enough to pass with the env fix reverted, which would have been a regression test that did not regress. It calls `set_seed` per run, matching 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. Two constructions of GAILDiscriminator in one process give different checksums; after `set_seed(0)` they match. So `seed=` means "reproducible given the same global state" and not yet "reproducible". Making it mean the second is a change to every agent and belongs in its own decision, not smuggled in here -- but it is the remaining half of #15. --- CHANGELOG.md | 8 ++++++++ src/decisionrl/imitation.py | 19 +++++++++++++++++- tests/test_imitation.py | 40 +++++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e877f9..66dd5ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,14 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). the freeze semantics of Stable-Baselines3's `VecNormalize`. ### Fixed +- `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/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_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}" From 4eadfadcad6c459af478c2863e96a565ed6715df Mon Sep 17 00:00:00 2001 From: Denis_Drobyshev Date: Sun, 20 Sep 2026 23:04:40 +0300 Subject: [PATCH 2/2] GRPO had the first half of the same bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GRPO(seed=0) дважды: 91.2 94.7 `_rollout_episode` resets the env once per episode and never with a seed, and `learn` never seeds it either, so every episode started from OS entropy. Same cause as GAIL's rollouts, same reason `set_seed` cannot help: an env owns an `np.random.default_rng()` built without a seed, which its docstring says. Seeded once at the top of `learn`, which is where `off_policy`, `tabular` and `sac_discrete` already do it. A reset keeps the generator it was handed, so the per-episode resets that follow draw from a seeded stream. Two runs now return 145.5 and 145.5. I audited the rest rather than assuming. Ten algorithms reset inside their training loop; eight of them -- dqn, dreamer, mbpo, off_policy, reinforce, rssm, sac_discrete, tabular -- seed the first reset, and the on-policy base and recurrent_ppo seed their vector env the same way. GRPO was the only one left, and `decision_transformer` and `her` seed per episode deliberately. So this is two cases in the package, not a pattern still hiding elsewhere. The test fails with this line reverted, which is the only reason to believe it. --- CHANGELOG.md | 7 +++++++ src/decisionrl/algorithms/grpo.py | 9 +++++++++ tests/test_grpo.py | 25 +++++++++++++++++++++++++ 3 files changed, 41 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66dd5ad..8682399 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,13 @@ 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 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/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}"