Skip to content

Enhance load_method to support optional planned memory allocator #8032

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

Conversation

cptspacemanspiff
Copy link
Contributor

@cptspacemanspiff cptspacemanspiff commented Jan 29, 2025

Summary

The module api for the executorch runtime is pretty nice, however the load_method function forces the user to use the heirarchical allocator auto-generated by load_method. This is less than ideal if we have to do anything special with our memory allocation.

My specific use-case is having the ability to share memory allocations between multiple methods. Specifically, I have a memory manager does all the bookwork, and the user can request a hierarchical allocator from the manager during the load method call.

This is related to issue #8030

That being said, this is probably useful in general for cases where a default 'buffer' is not desired, but where one would still like to use the Module extension.

Test plan

Currently testing has just been that it works, all it does is:

  1. add an additional parameter (pointer to hierarchical allocator), by default null.
  2. checks if the pointer is null, in which case no allocator was provided and a allocator is created.
  3. if non-null skip allocator creation and use the provided allocator. the lifetime of the allocator is handled externally.

cc @JacobSzwejbka @dbort

Copy link

pytorch-bot bot commented Jan 29, 2025

🔗 Helpful Links

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

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

✅ No Failures

As of commit 4956dac with merge base 2094659 (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 Jan 29, 2025
@manuelcandales manuelcandales added module: extension Issues related to code under extension/ release notes: api Changes to public facing apis (any interfaces, pybinded runtime methods, etc.) triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module module: runtime Issues related to the core runtime and code under runtime/ labels Jan 29, 2025
Copy link
Contributor

@JacobSzwejbka JacobSzwejbka 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 to me will let @shoumikhin take a look as well

@dbort dbort requested review from shoumikhin and removed request for dbort January 29, 2025 23:24
@shoumikhin
Copy link
Contributor

@cptspacemanspiff thanks a lot for the idea and implementation, I think that's a great way to provide the customers yet another point of customization. I started leaving suggestings to the code, but then realized it may be more readable as a demo PR, please feel free to borrow and update yours.
Thanks!

@cptspacemanspiff cptspacemanspiff force-pushed the load_method-support-optional-planned-memory-allocator branch from e943466 to 864f2cd Compare February 9, 2025 00:11
@cptspacemanspiff
Copy link
Contributor Author

@shoumikhin
Sorry for delay,
I copied your changes and tested to make sure things still worked.
I think this should be good?

- Updated the load_method signature to accept an optional runtime::HierarchicalAllocator parameter.
@cptspacemanspiff cptspacemanspiff force-pushed the load_method-support-optional-planned-memory-allocator branch from 5d7be33 to b55c86f Compare March 1, 2025 14:56
@cptspacemanspiff
Copy link
Contributor Author

cptspacemanspiff commented Mar 1, 2025

@shoumikhin

Can you look over this?
Just rebased to get around a merge conflict in main, It should be good to go.

@facebook-github-bot
Copy link
Contributor

@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@JacobSzwejbka JacobSzwejbka merged commit 75898bf into pytorch:main Mar 11, 2025
58 checks passed
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. module: extension Issues related to code under extension/ module: runtime Issues related to the core runtime and code under runtime/ release notes: api Changes to public facing apis (any interfaces, pybinded runtime methods, etc.) triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants