Skip to content

[BugFix] Reject invalid ReplayBuffer generations - #4043

Merged
vmoens merged 2 commits into
pytorch:mainfrom
theap06:feature/rb-update-if-present
Aug 17, 2026
Merged

vmoens merged 2 commits into
pytorch:mainfrom
theap06:feature/rb-update-if-present

Conversation

@theap06

@theap06 theap06 commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Rejects never-written replay slots from update_if_present when both generation values are -1.

Test: pytest test/rb/test_rb_core.py -k TestUpdateIfPresent

@pytorch-bot

pytorch-bot Bot commented Jul 24, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/rl/4043

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 1a289eb with merge base 82a2593 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@github-actions github-actions Bot added Feature New feature Documentation Improvements or additions to documentation Benchmarks rl/benchmark changes ReplayBuffers and removed Feature New feature labels Jul 24, 2026
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 24, 2026
@theap06
theap06 marked this pull request as draft July 24, 2026 08:08
@github-actions github-actions Bot added the Feature New feature label Jul 24, 2026
@vmoens
vmoens force-pushed the feature/rb-update-if-present branch 2 times, most recently from 6b96677 to 6b16ca6 Compare August 5, 2026 16:23
@theap06
theap06 force-pushed the feature/rb-update-if-present branch from 6b16ca6 to b8bf0a7 Compare August 9, 2026 08:51
@theap06
theap06 marked this pull request as ready for review August 9, 2026 08:51
@theap06

theap06 commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main and #4046, resolved the obsolete generation-stamp history, and marked this ready for review. The focused core, Ray, and doctest coverage passes locally. I do not have permission to request reviewers or apply labels; @vmoens, could you review and add the ci/optdeps label, then rerun CI after the label is present? This PR touches the Ray optional-dependency integration.

@vmoens

vmoens commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Reviewed rebased head b8bf0a7a0bc3ec5b995569aaf1414f86ac2553b7, including the latest generation-stamp reconciliation commit and the request for optional-dependency CI.

One blocking correctness issue is at torchrl/data/replay_buffers/replay_buffers.py in the live = current == generation comparison. Generation -1 explicitly means “no usable stamp”, but equality treats (-1, -1) as live. I reproduced this with a size-10 lazy storage containing only 5 records: update_if_present(index=tensor([7]), generation=tensor([-1]), patch={"x": tensor([99.])}) returns updated=True and writes slot 7 even though len(rb) == 5. Please exclude negative current/captured generations (and add the never-written-slot regression) so only genuine live handles can mutate storage.

Targeted result: all 14 TestUpdateIfPresent tests pass, which confirms the missing case. After the fix, the author-requested ci/optdeps label/rerun is still appropriate because the diff changes the Ray integration.

@theap06
theap06 force-pushed the feature/rb-update-if-present branch from b8bf0a7 to 00375a9 Compare August 17, 2026 01:59
@theap06 theap06 changed the title [Feature] Add ReplayBuffer.update_if_present for generation-safe conditional updates [BugFix] Reject invalid ReplayBuffer generations Aug 17, 2026
@theap06

theap06 commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

Since #4049 landed the conditional-update feature on main, I rebased this PR down to the reported correctness fix. Negative current or captured generations are now always stale, with a never-written-slot regression. Focused result: 30 passed.

@vmoens vmoens added the user-facing User-facing changes - only include in major releases label Aug 17, 2026

@vmoens vmoens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just a lint issue

Comment thread torchrl/data/replay_buffers/replay_buffers.py

@vmoens vmoens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@vmoens
vmoens merged commit 048bb08 into pytorch:main Aug 17, 2026
123 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Benchmarks rl/benchmark changes BugFix CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Documentation Improvements or additions to documentation Feature New feature ReplayBuffers user-facing User-facing changes - only include in major releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants