[RLlib] Fix wrong assert variable in `_update_env_seed_if_necessary` by nathon-lee · Pull Request #61823 · ray-project/ray

nathon-lee · 2026-03-18T02:21:55Z

Description

This PR fixes a small bug in rllib's _update_env_seed_if_necessary() helper.

The assert in that function was checking worker_idx < max_num_envs_per_env_runner, but the boundary there should apply to vector_idx, not worker_idx. Because of this, valid cases with a large worker_idx could be rejected incorrectly, while the intended guard is to ensure that vector_idx stays within the per-worker env limit used in the seed calculation.

This PR makes the fix by updating the assert to:

vector_idx < max_num_envs_per_env_runner

and adds a regression test covering the boundary behavior:

a valid large worker_idx with an in-range vector_idx should succeed
an out-of-range vector_idx should still raise

The change is intentionally minimal and should be low risk.

Related issues

Fixes #61593

Additional information

I kept the change narrowly scoped to the incorrect assert and the corresponding regression test.

I was not able to fully run the targeted test locally in this checkout because the local environment is missing the compiled ray._raylet module, but the logic change is straightforward and the added test is designed specifically to prevent regression for this boundary condition.

Signed-off-by: nathon-lee <leejianwoo@gmail.com>

gemini-code-assist

Code Review

This pull request correctly fixes a bug in _update_env_seed_if_necessary by using vector_idx instead of worker_idx in the assertion to prevent seed collisions. The accompanying regression tests are well-designed, covering both the success case for the fix and the failure case for out-of-bounds indices. I have one suggestion to improve the maintainability of the new tests by replacing magic numbers with a named constant.

gemini-code-assist · 2026-03-18T02:27:51Z

+    def test_update_env_seed_accepts_max_worker_idx_with_valid_vector_idx(self):
+        env = SeedRecordingEnv()
+
+        _update_env_seed_if_necessary(env, seed=7, worker_idx=1000, vector_idx=999)
+
+        self.assertEqual(env.last_seed, 1000 * 1000 + 999 + 7)
+
+    def test_update_env_seed_rejects_too_large_vector_idx(self):
+        env = SeedRecordingEnv()
+
+        with self.assertRaisesRegex(
+            AssertionError, "Too many envs per worker. Random seeds may collide."
+        ):
+            _update_env_seed_if_necessary(
+                env, seed=7, worker_idx=0, vector_idx=1000
+            )


The new tests use the magic number 1000, which corresponds to max_num_envs_per_env_runner in _update_env_seed_if_necessary. This makes the tests brittle if the constant in the implementation ever changes.

To improve maintainability, consider defining this value as a constant and using it in these tests. This makes the intent clearer and simplifies future updates.

Here's a suggestion that refactors both tests to use a local constant:

def test_update_env_seed_accepts_max_worker_idx_with_valid_vector_idx(self): # This value is hardcoded in `_update_env_seed_if_necessary`. max_num_envs_per_env_runner = 1000 env = SeedRecordingEnv() worker_idx = max_num_envs_per_env_runner vector_idx = max_num_envs_per_env_runner - 1 seed = 7 _update_env_seed_if_necessary( env, seed=seed, worker_idx=worker_idx, vector_idx=vector_idx ) self.assertEqual( env.last_seed, worker_idx * max_num_envs_per_env_runner + vector_idx + seed, ) def test_update_env_seed_rejects_too_large_vector_idx(self): # This value is hardcoded in `_update_env_seed_if_necessary`. max_num_envs_per_env_runner = 1000 env = SeedRecordingEnv() with self.assertRaisesRegex( AssertionError, "Too many envs per worker. Random seeds may collide." ): _update_env_seed_if_necessary( env, seed=7, worker_idx=0, vector_idx=max_num_envs_per_env_runner )

Signed-off-by: nathon-lee <leejianwoo@gmail.com>

pseudo-rnd-thoughts

Personally, I think that this assert shouldn't exist as we starting to work with users that certainly have more than a thousand environments per env-runner.
Its easier just not to have this assert and let the user get an error if the sub-environment doesn't exist anymore.
@ArturNiederfahrenhorst

Signed-off-by: nathon-lee <leejianwoo@gmail.com>

nathon-lee · 2026-03-23T14:15:51Z

Personally, I think that this assert shouldn't exist as we starting to work with users that certainly have more than a thousand environments per env-runner. Its easier just not to have this assert and let the user get an error if the sub-environment doesn't exist anymore. @ArturNiederfahrenhorst

Good point — I removed the assert instead of switching it to vector_idx, and updated the test to verify that large vector_idx values are allowed.

ArturNiederfahrenhorst · 2026-03-23T14:48:58Z

Agreed. The assertion does not make sense anymore.

nathon-lee · 2026-03-25T02:30:16Z

@ArturNiederfahrenhorst @pseudo-rnd-thoughts Thanks! I’ve pushed the fix.
If it looks good now, could you please merge the PR when you have time?

github-actions · 2026-04-08T12:36:28Z

This pull request has been automatically marked as stale because it has not had
any activity for 14 days. It will be closed in another 14 days if no further activity occurs.
Thank you for your contributions.

You can always ask for help on our discussion forum or Ray's public slack channel.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

pseudo-rnd-thoughts · 2026-04-09T08:36:51Z

@@ -132,9 +132,6 @@ def _update_env_seed_if_necessary(
    # A single RL job is unlikely to have more than 10K
    # rollout workers.
    max_num_envs_per_env_runner: int = 1000


This should be remove as well now along with the comment above

pseudo-rnd-thoughts · 2026-04-09T08:38:32Z

        self.assertTrue(len(unroll_ids_2) > 1)
        ev.stop()

+    def test_update_env_seed_accepts_max_worker_idx_with_valid_vector_idx(self):


Merge these two tests together

def test_update_env_seed(self): env = SeedRecordingEnv() _update_env_seed_if_necessary(env, seed=7, worker_idx=0, vector_idx=1000) self.assertEqual(env.last_seed, 1007) _update_env_seed_if_necessary(env, seed=7, worker_idx=1000, vector_idx=999) self.assertEqual(env.last_seed, 1000 * 1000 + 999 + 7)

pseudo-rnd-thoughts · 2026-04-09T08:39:03Z

@nathon-lee Could you make these two changes then we're good to merge

nathon-lee · 2026-04-09T08:40:13Z

@nathon-lee如果您能做这两项更改，我们就可以合并了。

ok , @pseudo-rnd-thoughts hi, I've made the revisions as per your suggestion, and it's ready to merge

Signed-off-by: nathon <leejianwoo@gmail.com>

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 13d5812. Configure here.}

pseudo-rnd-thoughts · 2026-04-10T08:14:32Z

The CI seems to be failing for reasons unrelated to this PR

nathon-lee · 2026-04-10T10:16:26Z

The CI seems to be failing for reasons unrelated to this PR

I re-ran the CI, but it still failed with the same ray.init() timeout. It seems to be an unrelated flaky test. Could a maintainer please take a look when you have a chance? @pseudo-rnd-thoughts

pseudo-rnd-thoughts · 2026-04-14T09:33:45Z

@nathon-lee Thanks, I lost track of this PR, yeah, we just need to update the branch to fix it

nathon-lee · 2026-04-14T10:11:51Z

@nathon-lee Thanks, I lost track of this PR, yeah, we just need to update the branch to fix it

Thanks, appreciate the merge!

…ay-project#61823) ## Description This PR fixes a small bug in `rllib`'s `_update_env_seed_if_necessary()` helper. The assert in that function was checking `worker_idx < max_num_envs_per_env_runner`, but the boundary there should apply to `vector_idx`, not `worker_idx`. Because of this, valid cases with a large `worker_idx` could be rejected incorrectly, while the intended guard is to ensure that `vector_idx` stays within the per-worker env limit used in the seed calculation. This PR makes the fix by updating the assert to: - `vector_idx < max_num_envs_per_env_runner` and adds a regression test covering the boundary behavior: - a valid large `worker_idx` with an in-range `vector_idx` should succeed - an out-of-range `vector_idx` should still raise The change is intentionally minimal and should be low risk. ## Related issues Fixes ray-project#61593 ## Additional information I kept the change narrowly scoped to the incorrect assert and the corresponding regression test. I was not able to fully run the targeted test locally in this checkout because the local environment is missing the compiled `ray._raylet` module, but the logic change is straightforward and the added test is designed specifically to prevent regression for this boundary condition. --------- Signed-off-by: nathon-lee <leejianwoo@gmail.com> Signed-off-by: nathon <leejianwoo@gmail.com>

fix: Fix wrong assert variable in _update_env_seed_if_necessary

17a403d

Signed-off-by: nathon-lee <leejianwoo@gmail.com>

nathon-lee requested a review from a team as a code owner March 18, 2026 02:21

gemini-code-assist Bot reviewed Mar 18, 2026

View reviewed changes

fix: Fix wrong assert variable in _update_env_seed_if_necessary

6f3986e

Signed-off-by: nathon-lee <leejianwoo@gmail.com>

ray-gardener Bot added rllib RLlib related issues community-contribution Contributed by the community labels Mar 18, 2026

Merge branch 'master' into fix_issue_61593

a9bf353

pseudo-rnd-thoughts requested changes Mar 23, 2026

View reviewed changes

rllib: remove env seed assert for large vector indices

718bba8

Signed-off-by: nathon-lee <leejianwoo@gmail.com>

cursor Bot reviewed Mar 23, 2026

View reviewed changes

Comment thread rllib/evaluation/rollout_worker.py

github-actions Bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Apr 8, 2026

Merge branch 'master' into fix_issue_61593

a07fbe4

cursor Bot reviewed Apr 9, 2026

View reviewed changes

Comment thread rllib/evaluation/tests/test_rollout_worker.py

pseudo-rnd-thoughts requested changes Apr 9, 2026

View reviewed changes

pseudo-rnd-thoughts removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Apr 9, 2026

fix: delete old var max_num_envs_per_env_runner and merge two unit test

fa9e2e5

Signed-off-by: nathon <leejianwoo@gmail.com>

pseudo-rnd-thoughts approved these changes Apr 9, 2026

View reviewed changes

pseudo-rnd-thoughts added the go add ONLY when ready to merge, run all tests label Apr 9, 2026

Merge branch 'master' into fix_issue_61593

13d5812

cursor Bot reviewed Apr 10, 2026

View reviewed changes

Comment thread rllib/evaluation/rollout_worker.py

Merge branch 'master' into fix_issue_61593

538f722

pseudo-rnd-thoughts changed the title ~~fix: Fix wrong assert variable in _update_env_seed_if_necessary~~ Apr 10, 2026

Merge branch 'master' into fix_issue_61593

8ac81f0

ArturNiederfahrenhorst merged commit 781d562 into ray-project:master Apr 14, 2026
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[RLlib] Fix wrong assert variable in `_update_env_seed_if_necessary`#61823

[RLlib] Fix wrong assert variable in `_update_env_seed_if_necessary`#61823
ArturNiederfahrenhorst merged 9 commits into
ray-project:masterfrom
nathon-lee:fix_issue_61593

nathon-lee commented Mar 18, 2026

gemini-code-assist Bot left a comment

gemini-code-assist Bot Mar 18, 2026

pseudo-rnd-thoughts left a comment

nathon-lee commented Mar 23, 2026

Uh oh!

ArturNiederfahrenhorst commented Mar 23, 2026

nathon-lee commented Mar 25, 2026

github-actions Bot commented Apr 8, 2026

Uh oh!

pseudo-rnd-thoughts Apr 9, 2026

pseudo-rnd-thoughts Apr 9, 2026

pseudo-rnd-thoughts commented Apr 9, 2026

nathon-lee commented Apr 9, 2026 •

edited

Loading

cursor Bot left a comment

Uh oh!

pseudo-rnd-thoughts commented Apr 10, 2026

nathon-lee commented Apr 10, 2026

pseudo-rnd-thoughts commented Apr 14, 2026

Uh oh!

nathon-lee commented Apr 14, 2026

Labels

3 participants

Uh oh!

Conversation

nathon-lee commented Mar 18, 2026

Description

Related issues

Additional information

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

gemini-code-assist Bot Mar 18, 2026

Choose a reason for hiding this comment

pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

nathon-lee commented Mar 23, 2026

Uh oh!

ArturNiederfahrenhorst commented Mar 23, 2026

nathon-lee commented Mar 25, 2026

github-actions Bot commented Apr 8, 2026

Uh oh!

pseudo-rnd-thoughts Apr 9, 2026

Choose a reason for hiding this comment

pseudo-rnd-thoughts Apr 9, 2026

Choose a reason for hiding this comment

pseudo-rnd-thoughts commented Apr 9, 2026

nathon-lee commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

pseudo-rnd-thoughts commented Apr 10, 2026

nathon-lee commented Apr 10, 2026

pseudo-rnd-thoughts commented Apr 14, 2026

Uh oh!

nathon-lee commented Apr 14, 2026

Labels

3 participants

nathon-lee commented Apr 9, 2026 •

edited

Loading