Skip to content

Conversation

pseudo-rnd-thoughts
Copy link
Member

Why are these changes needed?

SegmentTree is a component of the rllib PrioritizedEpisodeReplayBuffer however for extreme edge case prefix sum values then find_prefixsum_idx will return invalid out of bounds value.
I couldn't find a bug, rather if the prefixsum_value is equal to the SegmentTree.sum() then traversing down the tree could cause it to return invalid indexes.

I've added unittests to reproduce the original error and check against it

Related issue number

Close #54284

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run pre-commit jobs to lint the changes in this PR. (pre-commit setup)
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Signed-off-by: Mark Towers <mark@anyscale.com>
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 addresses an edge case in SegmentTree.find_prefixsum_idx that could lead to returning an invalid index, particularly when the prefixsum value aligns with the sum of a subtree. The fix involves a small adjustment to the prefixsum value. The addition of a unit test to reproduce and verify the fix is a great step. My feedback focuses on making the logical fix more robust and less dependent on a fixed magic number, which could be brittle under certain conditions.

Signed-off-by: Mark Towers <mark@anyscale.com>
@pseudo-rnd-thoughts pseudo-rnd-thoughts added the rllib RLlib related issues label Oct 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rllib RLlib related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RLlib] Unexpected KeyError while training SAC

2 participants