Skip to content

Conversation

@ArturNiederfahrenhorst
Copy link
Contributor

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.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly fixes an issue where an incorrect attribute self._minibatch_size was accessed in the IMPALA algorithm's configuration validation. The correct attribute self.minibatch_size is now used. Additionally, a new test is introduced to cover this validation logic, which was previously untested. The fix is correct and the addition of the test is a great improvement. I've added one suggestion to enhance the new test by parameterizing it and adding another validation scenario to make it more comprehensive and maintainable.

Comment on lines +21 to +33
def test_impala_minibatch_size_check(self):
config = (
impala.IMPALAConfig()
.environment("CartPole-v1")
.training(minibatch_size=100)
.env_runners(rollout_fragment_length=30)
)

with pytest.raises(
ValueError,
match=r"`minibatch_size` \(100\) must either be None or a multiple of `rollout_fragment_length` \(30\)",
):
config.validate()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Great job adding a test for this validation logic! To make it even more robust and easier to maintain, I suggest parameterizing the test using pytest.mark.parametrize. This allows us to easily cover more scenarios. I've included a second test case to also validate the check against total_train_batch_size, which is part of the same validation logic.

    @pytest.mark.parametrize(
        "minibatch_size, rollout_fragment_length, match",
        [
            (100, 30, r"`minibatch_size` \(100\) must either be None or a multiple of `rollout_fragment_length` \(30\)"),
            (600, 30, r"smaller than or equal to `total_train_batch_size` \(500\)"),
        ],
    )
    def test_impala_minibatch_size_check(
        self, minibatch_size, rollout_fragment_length, match
    ):
        config = (
            impala.IMPALAConfig()
            .environment("CartPole-v1")
            .training(minibatch_size=minibatch_size)
            .env_runners(rollout_fragment_length=rollout_fragment_length)
        )

        with pytest.raises(ValueError, match=match):
            config.validate()

@ray-gardener ray-gardener bot added the rllib RLlib related issues label Nov 13, 2025
@pseudo-rnd-thoughts pseudo-rnd-thoughts added the go add ONLY when ready to merge, run all tests label Nov 13, 2025
@simonsays1980 simonsays1980 merged commit 5feb457 into ray-project:master Nov 14, 2025
6 checks passed
ArturNiederfahrenhorst added a commit to ArturNiederfahrenhorst/ray that referenced this pull request Nov 16, 2025
## 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.
Aydin-ab pushed a commit to Aydin-ab/ray-aydin that referenced this pull request Nov 19, 2025
## 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.

Signed-off-by: Aydin Abiar <aydin@anyscale.com>
ykdojo pushed a commit to ykdojo/ray that referenced this pull request Nov 27, 2025
## 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.

Signed-off-by: YK <1811651+ykdojo@users.noreply.github.com>
SheldonTsen pushed a commit to SheldonTsen/ray that referenced this pull request Dec 1, 2025
## 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go add ONLY when ready to merge, run all tests rllib RLlib related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants