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.

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

PyTorch: improve memory-efficiency in batched non-shuffle buffer #762

Merged
merged 1 commit into from Jul 26, 2022

Conversation

chongxiaoc
Copy link
Collaborator

@chongxiaoc chongxiaoc commented Jul 12, 2022

Avoid creating many copies in _make_batch().

Fix #763

@chongxiaoc chongxiaoc requested a review from selitvin July 12, 2022 23:26
@chongxiaoc chongxiaoc force-pushed the non-shuffle branch 2 times, most recently from bef1fc1 to 3a0911e Compare July 12, 2022 23:30
@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #762 (15c5b38) into master (fa8a881) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #762      +/-   ##
==========================================
- Coverage   86.27%   86.26%   -0.01%     
==========================================
  Files          85       85              
  Lines        5084     5081       -3     
  Branches      785      783       -2     
==========================================
- Hits         4386     4383       -3     
  Misses        559      559              
  Partials      139      139              
Impacted Files Coverage Δ
petastorm/reader_impl/pytorch_shuffling_buffer.py 96.58% <100.00%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa8a881...15c5b38. Read the comment docs.

@chongxiaoc chongxiaoc force-pushed the non-shuffle branch 2 times, most recently from b221591 to 6884b3a Compare July 13, 2022 01:06
@chongxiaoc
Copy link
Collaborator Author

@selitvin Can you take a look?

@chongxiaoc
Copy link
Collaborator Author

chongxiaoc commented Jul 21, 2022

Updated PR:

  • Memory usage is even lower because of removing intermediate deque().

Screen Shot 2022-07-20 at 10 33 33 PM

  • Performance comparison of iterating 13798 batches from production dataset at Uber.
    Master branch: 145.2844157218933 seconds
    Updated PR: 43.827380657196045 seconds.
    3X faster.

@selitvin

@chongxiaoc chongxiaoc force-pushed the non-shuffle branch 7 times, most recently from 904ad1c to e5620b6 Compare July 25, 2022 20:30
Copy link
Collaborator

@selitvin selitvin left a comment

Choose a reason for hiding this comment

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

Looks good! Please add a line to release notes. Perhaps we can improve the _batch_i variable name?


def test_batched_noop_shuffling_buffer():
"""Check intermediate status of batched non-shuffling buffer"""
import torch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move the import to the top?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is out-dated test from previous commit.

self.store.append(batch)
self._buffer = []
self._done_adding = False
self._batch_i = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps a more self explanatory name instead of _batch_i? Something like first_unconsumed_row or something along the lines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated with name and comments.

Avoid creating many copies in _make_batch().
Reuse same rowgroup to generate all available batches.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PyTorch Batched Non-shuffle Buffer Large Memory Consumption
2 participants