Skip to content

[Feature] CompressedStorage #3058

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.

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

Closed
wants to merge 1 commit into from
Closed

[Feature] CompressedStorage #3058

wants to merge 1 commit into from

Conversation

vmoens
Copy link
Collaborator

@vmoens vmoens commented Jul 11, 2025

@AdrianOrenstein feedback on this?

Copy link

pytorch-bot bot commented Jul 11, 2025

🔗 Helpful Links

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

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

❌ 10 New Failures, 6 Unrelated Failures

As of commit 3da84a9 with merge base 7e8f940 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

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

@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 Jul 11, 2025
@vmoens vmoens linked an issue Jul 11, 2025 that may be closed by this pull request
1 task
@vmoens vmoens added the enhancement New feature or request label Jul 11, 2025
@AdrianOrenstein
Copy link

I think the PR is great. If I get permissions to modify you could delegate some of the tasks to me if you'd like.

I've left some comments below:

  • The conversion of an object to a bytestream is hardcoded. If we move that into a function, it'll be easier to later extend this class to serialise the other objects like numpy.arrays or a pickle dump of some object.

  • I don't think we had any tests on numpy objects in the tensordict or arbitrary python objects.

  • Is the goal to keep the scope of the PR just for CPU data, CPU storage, CPU compression, and CPU decompression? If so, I might follow up with a PR to include some GPU compression. I like the API and it is in a state where I'd be able to extend it.

  • In docs/source/reference/data.rst you mention a 2-10x compression ratio. I was thinking of including pytest-benchmark as a dependancy and then we can get a number on compressing atari frames. This might also be out of scope so I could do it later.

  • There is pip install zstd that provides a stateless compression and decompression API which might be more convenient.

  • If you're open to adding pytest-benchmark as a dependancy, we could make this a regression test. It might make sense to add an Atari rollout to evaluate the expected compression ratio.

@vmoens
Copy link
Collaborator Author

vmoens commented Jul 12, 2025

All of these look good! I think it's going to be an amazing feature!

I don't think we want a separate PR for GPU. Not particularly opinionated though, if you feel like splitting this in separate chunks I'm good with it.
Also you can just branch out of this branch and make a new PR with your changes, I'll close this one.
Thanks!

@AdrianOrenstein
Copy link

AdrianOrenstein commented Jul 12, 2025

Great, I'll get started shortly on the gpu side of things. Also thanks for getting me started with this pr, it has helped me wrap my head around a lot of the details of the codebase.

@vmoens
Copy link
Collaborator Author

vmoens commented Jul 16, 2025

Closing in favor of #3062

@vmoens vmoens closed this Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Compressing data stored in the Replay Buffer
3 participants