Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Deprecation] Deprecate ambiguous device for memmap replay buffer #1624

Merged
merged 2 commits into from
Oct 10, 2023
Merged

[Deprecation] Deprecate ambiguous device for memmap replay buffer #1624

merged 2 commits into from
Oct 10, 2023

Conversation

vmoens
Copy link
Contributor

@vmoens vmoens commented Oct 10, 2023

Description

We allow memmap storages to sit on a "device", which basically means that when accessed, the data will be sent to that device.
This is confusing and suboptimal, and will break once we have memory mapped tensors that are actually tensors (see pytorch/tensordict#541)

@facebook-github-bot facebook-github-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 Oct 10, 2023
@vmoens vmoens added bug Something isn't working quality code quality labels Oct 10, 2023
@vmoens vmoens marked this pull request as ready for review October 10, 2023 15:49
matteobettini
matteobettini approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@matteobettini matteobettini left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

So before when they were loaded form disk they were firstly loaded to cpu mem and then automatically put on device?

@vmoens
Copy link
Contributor Author

vmoens commented Oct 10, 2023

LGTM

So before when they were loaded form disk they were firstly loaded to cpu and then automatically put on device?

Something like that yes
It was a very bad design choice.

@vmoens vmoens merged commit 70c650e into main Oct 10, 2023
32 of 57 checks passed
@vmoens vmoens deleted the fix_memmap_device branch October 10, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matteobettini matteobettini matteobettini approved these changes

Assignees
No one assigned
Labels
bug Something isn't working CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. quality code quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vmoens @matteobettini @facebook-github-bot