Skip to content

Warn users when n_steps is passed with custom replay buffer + doc update - #2275

Merged
araffin merged 10 commits into
DLR-RM:masterfrom
Koustav-github:fix/n-steps-ignored-with-custom-replay-buffer
Aug 16, 2026
Merged

araffin merged 10 commits into
DLR-RM:masterfrom
Koustav-github:fix/n-steps-ignored-with-custom-replay-buffer

Conversation

@Koustav-github

@Koustav-github Koustav-github commented Aug 12, 2026 •

Copy link
Copy Markdown
Contributor

Description

Moves the n-step validation in OffPolicyAlgorithm._setup_model() out of the
if self.replay_buffer_class is None: branch, so it also applies when the user supplies the
buffer class. Previously, passing replay_buffer_class explicitly caused n_steps > 1 to be
silently ignored: the model kept n_steps, but the buffer could not compute n-step returns, so
sample() returned discounts=None and training fell back to 1-step returns with no error or
warning.

Now, when n_steps > 1:

  • Dict observation spaces raise (previously raised only on the default path)
  • a class that is not an NStepReplayBuffer subclass raises, naming the class
  • an NStepReplayBuffer subclass receives n_steps/gamma, as the default path already does

The subclass case uses issubclass rather than an identity check, so user subclasses keep
working — today such a subclass silently receives the buffer's own defaults (n_steps=3,
gamma=0.99) instead of the values passed to the algorithm.

Motivation and Context

Closes #2274.

This mostly affects HER users, since HerReplayBuffer is the class the docs recommend passing
explicitly — SAC(..., replay_buffer_class=HerReplayBuffer, n_steps=3) silently trained with
1-step returns. It also made the same configuration behave inconsistently: rejected with an
AssertionError on the default path, accepted silently when the class was named.

  • I have raised an issue to propose this change

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)

Marked as breaking because the Dict-obs rejection now raises ValueError instead of
AssertionError. This is the open question from the issue — I chose ValueError so both
rejections share a type and survive python -O, but I'm happy to keep assert if you prefer
the smaller diff. Opening as a draft until you've had a chance to weigh in; no rush.

Checklist

  • I've read the CONTRIBUTION guide (required)
  • I have updated the changelog accordingly (docs/misc/changelog.md) (required).
  • My change requires a change to the documentation.
  • I have updated the tests accordingly (required for a bug fix or a new feature).
  • I have updated the documentation accordingly.
  • I have opened an associated PR on the SB3-Contrib repository (if necessary)
  • I have opened an associated PR on the RL-Zoo3 repository (if necessary)
  • I have reformatted the code using make format (required)
  • I have checked the codestyle using make check-codestyle and make lint (required)
  • I have ensured make pytest and make type both pass. (required)
  • I have checked that the documentation builds using make doc (required)

Notes on verification

  • black could not run on my local Python 3.12.5 (it refuses with a CPython memory-safety
    warning), so I ran it via uvx --python 3.12.4 black — 95 files unchanged. ruff and mypy
    pass locally.
  • I ran the affected suites rather than the full make pytest: test_n_step_replay.py (28
    passed), plus test_buffers.py, test_her.py, test_run.py, test_callbacks.py,
    test_save_load.py. A few failures in test_save_load.py and test_performance_her are
    pre-existing on Windows and reproduce without this change.
    • Ran the full make pytest suite (pytest -m "not expensive"): 877 passed, 7 failed,
      29 skipped
      . All 7 failures are pre-existing on Windows and unrelated to this change:
      test_performance_her[10] (a learning-performance threshold — confirmed it fails
      identically with this patch reverted), two test_monitor.py and one test_vec_monitor.py
      tests failing with PermissionError: [WinError 32] on the monitor CSV file handle, and
      three test_save_load.py tests exercising open_path() where Windows raises
      PermissionError instead of the expected IsADirectoryError. CI on Linux should cover
      all of these properly.

Disclosure per CONTRIBUTING.md: I used an AI code assistant (Claude) while investigating this
and drafting the patch. I have reviewed and tested the change and understand it.

if self.replay_buffer_class is None:
if isinstance(self.observation_space, spaces.Dict):
self.replay_buffer_class = DictReplayBuffer
assert self.n_steps == 1, "N-step returns are not supported for Dict observation spaces yet."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be reverted no? (well, anything not related to the warning)
And maybe add quick test to check that the warning is correct emitted.

@Koustav-github Koustav-github Aug 16, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hey @araffin,

that was a fair call, my files drifted due to some merge conflicts, i changed them in my latest commit.

and for the quick test u can run this part of the test_n_step_replay.py

tests/test_n_step_replay.py::test_n_steps_warns_with_custom_replay_buffer_class  -> this will raise the warning
tests/test_n_step_replay.py::test_custom_replay_buffer_class_without_n_steps -> should pass with no warning

@araffin araffin added the LLM generated We do not accept LLM generated issues/PR, please tell your human label Aug 16, 2026
@araffin araffin changed the title Fix n_steps being silently ignored when replay_buffer_class is passed… Warn users when n_steps is passed with custom replay buffer + doc update Aug 16, 2026
@araffin
araffin merged commit 45a5500 into DLR-RM:master Aug 16, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LLM generated We do not accept LLM generated issues/PR, please tell your human

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: n_steps > 1 is silently ignored when replay_buffer_class is passed explicitly

2 participants