Skip to content

Conversation

@keyprocedure
Copy link
Contributor

@keyprocedure keyprocedure commented Jun 13, 2025

Summary

  • Adds load_into() to support memory mapped loading directly into the caller's buffer.
  • Enables copying a specific byte range from the file without creating an internal buffer.
  • Resuses input validation from load().

Fixes #11561

Test plan

  • Added unit tests to MmapDataLoaderTest for load_into() to validate copying for both aligned and offset data.
  • All Mmap Data Loader tests pass via:
./build-ninja/extension/data_loader/test/extension_data_loader_test --gtest_filter='MmapDataLoaderTest.*'
  • Full set of Data Loader tests pass via:
./build-ninja/extension/data_loader/test/extension_data_loader_test

@pytorch-bot
Copy link

pytorch-bot bot commented Jun 13, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/11654

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

❌ 1 New Failure

As of commit 5589d40 with merge base 7bd15b9 (image):

NEW FAILURE - The following job has failed:

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 Jun 13, 2025
@keyprocedure
Copy link
Contributor Author

@pytorchbot label "release notes: none"

@pytorch-bot pytorch-bot bot added the release notes: none Do not include this in the release notes label Jun 13, 2025
@keyprocedure keyprocedure changed the title Implement load_into to Mmap Data Loader Implement load_into for Mmap Data Loader Jun 13, 2025

ET_NODISCARD executorch::runtime::Result<size_t> size() const override;

ET_NODISCARD executorch::runtime::Error validate_input(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can you make this private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, moved to private section

@JacobSzwejbka
Copy link
Contributor

just letting ci run again and then will merge

@keyprocedure
Copy link
Contributor Author

CI failure looks unrelated: MimiModel is missing _context_for_encoder_decoder

@JacobSzwejbka JacobSzwejbka merged commit 18e4240 into pytorch:main Jun 23, 2025
95 of 96 checks passed
@JacobSzwejbka
Copy link
Contributor

Thanks for contributing!

@keyprocedure
Copy link
Contributor Author

I'm happy to help! Thanks for the review.

GregoryComer added a commit to GregoryComer/executorch that referenced this pull request Jun 24, 2025
GregoryComer added a commit that referenced this pull request Jun 24, 2025
### Summary
Revert #11654 and
#11858 due to Meta-internal
build breakage. I will re-land these (preserving attribution).
GregoryComer pushed a commit to GregoryComer/executorch that referenced this pull request Jun 24, 2025
### Summary
- Adds `load_into()` to support memory mapped loading directly into the
caller's buffer.
- Enables copying a specific byte range from the file without creating
an internal buffer.
- Resuses input validation from `load()`.

Fixes pytorch#11561

### Test plan
- Added unit tests to `MmapDataLoaderTest` for `load_into()` to validate
copying for both aligned and offset data.
- All Mmap Data Loader tests pass via: 
```
./build-ninja/extension/data_loader/test/extension_data_loader_test --gtest_filter='MmapDataLoaderTest.*'
```
- Full set of Data Loader tests pass via:
```
./build-ninja/extension/data_loader/test/extension_data_loader_test
```
GregoryComer pushed a commit to GregoryComer/executorch that referenced this pull request Jun 24, 2025
### Summary
- Adds `load_into()` to support memory mapped loading directly into the
caller's buffer.
- Enables copying a specific byte range from the file without creating
an internal buffer.
- Resuses input validation from `load()`.

Fixes pytorch#11561

### Test plan
- Added unit tests to `MmapDataLoaderTest` for `load_into()` to validate
copying for both aligned and offset data.
- All Mmap Data Loader tests pass via: 
```
./build-ninja/extension/data_loader/test/extension_data_loader_test --gtest_filter='MmapDataLoaderTest.*'
```
- Full set of Data Loader tests pass via:
```
./build-ninja/extension/data_loader/test/extension_data_loader_test
```
@GregoryComer
Copy link
Member

FYI I temporarily reverted this due to a meta-internal build issue. Sorry about that. I'm going to re-merge it shortly, preserving commit authorship. We'll also fix the CI so that it builds with -Wshadow.

@keyprocedure
Copy link
Contributor Author

Got it. Let me know if you'd like me to change anything.

GregoryComer pushed a commit that referenced this pull request Jun 24, 2025
### Summary
- Adds `load_into()` to support memory mapped loading directly into the
caller's buffer.
- Enables copying a specific byte range from the file without creating
an internal buffer.
- Resuses input validation from `load()`.

Fixes #11561

### Test plan
- Added unit tests to `MmapDataLoaderTest` for `load_into()` to validate
copying for both aligned and offset data.
- All Mmap Data Loader tests pass via: 
```
./build-ninja/extension/data_loader/test/extension_data_loader_test --gtest_filter='MmapDataLoaderTest.*'
```
- Full set of Data Loader tests pass via:
```
./build-ninja/extension/data_loader/test/extension_data_loader_test
```
hinriksnaer pushed a commit to hinriksnaer/executorch that referenced this pull request Jun 26, 2025
### Summary
- Adds `load_into()` to support memory mapped loading directly into the
caller's buffer.
- Enables copying a specific byte range from the file without creating
an internal buffer.
- Resuses input validation from `load()`.

Fixes pytorch#11561

### Test plan
- Added unit tests to `MmapDataLoaderTest` for `load_into()` to validate
copying for both aligned and offset data.
- All Mmap Data Loader tests pass via: 
```
./build-ninja/extension/data_loader/test/extension_data_loader_test --gtest_filter='MmapDataLoaderTest.*'
```
- Full set of Data Loader tests pass via:
```
./build-ninja/extension/data_loader/test/extension_data_loader_test
```
hinriksnaer pushed a commit to hinriksnaer/executorch that referenced this pull request Jun 26, 2025
### Summary
Revert pytorch#11654 and
pytorch#11858 due to Meta-internal
build breakage. I will re-land these (preserving attribution).
hinriksnaer pushed a commit to hinriksnaer/executorch that referenced this pull request Jun 26, 2025
### Summary
- Adds `load_into()` to support memory mapped loading directly into the
caller's buffer.
- Enables copying a specific byte range from the file without creating
an internal buffer.
- Resuses input validation from `load()`.

Fixes pytorch#11561

### Test plan
- Added unit tests to `MmapDataLoaderTest` for `load_into()` to validate
copying for both aligned and offset data.
- All Mmap Data Loader tests pass via: 
```
./build-ninja/extension/data_loader/test/extension_data_loader_test --gtest_filter='MmapDataLoaderTest.*'
```
- Full set of Data Loader tests pass via:
```
./build-ninja/extension/data_loader/test/extension_data_loader_test
```
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. release notes: none Do not include this in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement load_into for mmap data loader

4 participants