Skip to content

Default to file load mode in module #10827

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
Merged
merged 1 commit into from
May 13, 2025
Merged

Default to file load mode in module #10827

merged 1 commit into from
May 13, 2025

Conversation

GregoryComer
Copy link
Member

@GregoryComer GregoryComer commented May 12, 2025 •
edited
Loading

Summary:
We've seen a number of people hit issues with mlock as the default load mode. In particular, mlock will fail above a certain PTE size and may require elevated permissions on some systems. We should not default to using mlock and can leave at as an opt-in optimization.

Differential Revision: D74607072

@GregoryComer GregoryComer requested a review from shoumikhin as a code owner May 12, 2025 21:14
@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented May 12, 2025 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 585f0c7 with merge base 27e159e (image):
💚 Looks good so far! There are no failures yet. 💚

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 May 12, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74607072

@shoumikhin
Copy link
Contributor

shoumikhin commented May 12, 2025 •
edited
Loading

How about changing it to File by default, so that everything else becomes explicit and people know what they do?
Btw, It is the default for ObjC/Swift bindings.

@GregoryComer
Copy link
Member Author

GregoryComer commented May 12, 2025 •
edited
Loading

How about changing it to File by default, so that everything else becomes explicit and people know what they do? Btw, It is the default for ObjC/Swift bindings.

That's good with me - I'll swap it to File. I initially went with MMap because peak memory is reduced by quite a bit, but I'm okay to leave that as an opt-in optimization.

@GregoryComer @facebook-github-bot
Default to mmap load mode in module (pytorch#10827)
585f0c7 
Summary:

We've seen a number of people hit issues with mlock as the default load mode. In particular, mlock will fail above a certain PTE size and may require elevated permissions on some systems. We should not default to using mlock and can leave at as an opt-in optimization.

We could use file or mmap mode. I'm leaning towards mmap, as it can lead to lower peak resident memory size, which can be significant for larger models.

Differential Revision: D74607072
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74607072

@GregoryComer GregoryComer changed the title Default to mmap load mode in module Default to file load mode in module May 12, 2025
@GregoryComer GregoryComer added the release notes: api Changes to public facing apis (any interfaces, pybinded runtime methods, etc.) label May 12, 2025
@GregoryComer GregoryComer merged commit aa73a55 into pytorch:main May 13, 2025
94 of 96 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shoumikhin shoumikhin shoumikhin approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported release notes: api Changes to public facing apis (any interfaces, pybinded runtime methods, etc.)
Projects
None yet
Milestone
No milestone
Development

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