-
Notifications
You must be signed in to change notification settings - Fork 685
Add program-data separation to pybindings #13886
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
This pull request was exported from Phabricator. Differential Revision: D76353209 |
This PR needs a
|
This pull request was exported from Phabricator. Differential Revision: D76353209 |
Summary: Pull Request resolved: pytorch#13886 Add support for optional data path for pybindings. Differential Revision: D76353209
26a1dad
to
fbe5a98
Compare
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/13886
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit 8eb077b with merge base fbda3a9 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
tensor_data = bytes(exec_program._tensor_data.pop("_default_external_constant")) | ||
ptd.write(tensor_data) | ||
|
||
executorch_program = self.runtime._load_for_executorch(pte_file, ptd_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.
probably need to update the pyi
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.
Thanks, updated!
Summary: Add support for optional data path for pybindings. Differential Revision: D76353209
fbe5a98
to
599a6e1
Compare
This pull request was exported from Phabricator. Differential Revision: D76353209 |
Summary: Add support for optional data path for pybindings. Differential Revision: D76353209
599a6e1
to
5b4cb9a
Compare
This pull request was exported from Phabricator. Differential Revision: D76353209 |
) | ||
ptd.write(tensor_data) | ||
|
||
executorch_program = self.runtime._load_for_executorch(pte_file, ptd_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.
It's fine to update the legacy API (methods that start with _) but we have modern pybindings APIs that we want users to use.
Don't we need to update runtime.load_program(pte, ptd) too?
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.
Thanks Mergen, sorry for the late response. I think runtime.load_program is only intended to take a PTE file --> it understands and deserializes the PTE file format (not PTD). The C++ Program object doesn't hold a PTD file.
We can potentially support this in load_method, which takes a NamedDataMap (from PTD) as an optional argument. We'd need to add pybindings on extension/flat_tensor to load the PTD file for this.
Having it in Module may be OK at the moment, let me know what you think (I can add pybindings for extension/flat_tensor in a separate PR).
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.
executorch/runtime/__init__.py
Lines 199 to 204 in cac1a71
def load_program( | |
self, | |
data: Union[bytes, bytearray, BinaryIO, Path, str], | |
*, | |
verification: Verification = Verification.InternalConsistency, | |
) -> Program: |
5b4cb9a
to
36eb4be
Compare
Summary: Add support for optional data path for pybindings. Reviewed By: JacobSzwejbka Differential Revision: D76353209
Summary: Add support for optional data path for pybindings. Reviewed By: JacobSzwejbka Differential Revision: D76353209
36eb4be
to
a91be3a
Compare
This pull request was exported from Phabricator. Differential Revision: D76353209 |
Summary: Pull Request resolved: pytorch#13886 Pull request resolved: pytorch#13886 Add support for optional data path for pybindings. Reviewed By: JacobSzwejbka Differential Revision: D76353209
This pull request was exported from Phabricator. Differential Revision: D76353209 |
a91be3a
to
8eb077b
Compare
Summary: Add support for optional data path for pybindings.
Differential Revision: D76353209