-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Memory-Mapped IndexedDataset implementation #589
Memory-Mapped IndexedDataset implementation #589
Conversation
…le with IndexedDataset/IndexedDatasetBuilder
…and moved the option definition custom task args to more high-level options.add_dataset_args() (more appropriate). Implemented also utils funzion in indexed_dataset: make_dataset(), dataset_exists()
… larger file on disk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@myleott has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Can you please also update the docs for --lazy-load
: https://github.com/pytorch/fairseq/blob/master/docs/getting_started.rst
fairseq/tasks/language_modeling.py
Outdated
parser.add_argument('--lazy-load', action='store_true', | ||
help='load the dataset lazily') | ||
parser.add_argument('--raw-text', default=False, action='store_true', | ||
help='load raw text dataset') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's provide backward compatibility for these. Add back these flags, change the description to indicate [deprecated]
and add logic here: https://github.com/pytorch/fairseq/blob/master/fairseq/options.py#L111-L115.
Something like:
if getattr(args, 'raw_text', False):
utils.deprecation_warning('--raw-text is deprecated, please use --dataset-impl=raw')
args.dataset_impl = 'raw'
elif getattr(args, 'lazy_load', False):
utils.deprecation_warning('--lazy-load is deprecated, please use --dataset-impl=lazy')
args.dataset_impl = 'lazy'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have to include these changes, I will work on this asap!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@myleott has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@myleott has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@myleott has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Add backward compatibility and fix tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@myleott has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@@ -262,3 +293,169 @@ def finalize(self, index_file): | |||
write_longs(index, self.data_offsets) | |||
write_longs(index, self.sizes) | |||
index.close() | |||
|
|||
|
|||
def _warmup_mmap_file(path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @davidecaroselli, I noticed in your implementation, you require a full read of the dataset at the beginning of training to warm up the mmap file. Is this actually useful? Can I get away with not warming up the mmap file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Yes, that is super useful because that warmup ensure best performance during actual training. However it is not mandatory, meaning that everything works even without it. So if, for some strange reason, you want to avoid it, just comment the call!
Summary: Following discussion in facebookresearch/fairseq#574: - Implemented MMapIndexedDataset and MMapIndexedDatasetBuilder compatible with IndexedDataset/IndexedDatasetBuilder - Update scripts/read_binarized.py to support new MMapIndexedDataset - Option '--raw-text' and '--lazy-load' replaced with '--dataset-impl' and moved the option definition custom task args to more high-level options.add_dataset_args() (more appropriate) - Implemented also utils functions in indexed_dataset: make_dataset(), dataset_exists() Pull Request resolved: facebookresearch/fairseq#589 Differential Revision: D14597128 Pulled By: myleott fbshipit-source-id: 4e92d99920cbaa52cfe5a0f1f5d9ae5c92d4268e
Summary: Following discussion in facebookresearch/fairseq#574: - Implemented MMapIndexedDataset and MMapIndexedDatasetBuilder compatible with IndexedDataset/IndexedDatasetBuilder - Update scripts/read_binarized.py to support new MMapIndexedDataset - Option '--raw-text' and '--lazy-load' replaced with '--dataset-impl' and moved the option definition custom task args to more high-level options.add_dataset_args() (more appropriate) - Implemented also utils functions in indexed_dataset: make_dataset(), dataset_exists() Pull Request resolved: facebookresearch/fairseq#589 Differential Revision: D14597128 Pulled By: myleott fbshipit-source-id: 4e92d99920cbaa52cfe5a0f1f5d9ae5c92d4268e
Following discussion in #574: