Skip to content

Commit bf02378

Browse files
ArturNiederfahrenhorstSheldonTsen
authored andcommitted
[RLlib] Fix access to self._minibatch_size (ray-project#58595)
## Description In IMPALA, we access an attribute `self._minibatch_size` which does not exist anymore. It should be `self._minibatch_size`. While this check is nice, it's effectively untested code. This PR introduces a test that adds a small test that triggers the relevant code path.
1 parent e7d268a commit bf02378

File tree

2 files changed

+17
-1
lines changed

2 files changed

+17
-1
lines changed

rllib/algorithms/impala/impala.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -427,7 +427,7 @@ def validate(self) -> None:
427427
and self.minibatch_size <= self.total_train_batch_size
428428
):
429429
self._value_error(
430-
f"`minibatch_size` ({self._minibatch_size}) must either be None "
430+
f"`minibatch_size` ({self.minibatch_size}) must either be None "
431431
"or a multiple of `rollout_fragment_length` "
432432
f"({self.rollout_fragment_length}) while at the same time smaller "
433433
"than or equal to `total_train_batch_size` "

rllib/algorithms/impala/tests/test_impala.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import unittest
22

3+
import pytest
4+
35
import ray
46
import ray.rllib.algorithms.impala as impala
57
from ray.rllib.policy.sample_batch import DEFAULT_POLICY_ID
@@ -16,6 +18,20 @@ def setUpClass(cls) -> None:
1618
def tearDownClass(cls) -> None:
1719
ray.shutdown()
1820

21+
def test_impala_minibatch_size_check(self):
22+
config = (
23+
impala.IMPALAConfig()
24+
.environment("CartPole-v1")
25+
.training(minibatch_size=100)
26+
.env_runners(rollout_fragment_length=30)
27+
)
28+
29+
with pytest.raises(
30+
ValueError,
31+
match=r"`minibatch_size` \(100\) must either be None or a multiple of `rollout_fragment_length` \(30\)",
32+
):
33+
config.validate()
34+
1935
def test_impala_lr_schedule(self):
2036
# Test whether we correctly ignore the "lr" setting.
2137
# The first lr should be 0.05.

0 commit comments

Comments
 (0)