[executorch][flat_tensor] implement load into and dont hold onto the segment #8447

lucylq · 2025-02-13T04:04:34Z

Stack from ghstack (oldest at bottom):

-> [executorch][flat_tensor] implement load into and dont hold onto the segment #8447
[flat_tensor] Persist FreeableBuffers of external constants in method #8437

Implement load_into in FlatTensorDataMap
Do not persist 'data_ro' in the FlatTensorDataMap. From get_data, return the FreeableBuffer given by the data loader.

TODO: add test for load_into.

Differential Revision: D69148652

…segment 1. Implement load_into in FlatTensorDataMap 2. Do not persist 'data_ro' in the FlatTensorDataMap. From `get_data`, return the FreeableBuffer given by the data loader. TODO: add test for load_into. Differential Revision: [D69148652](https://our.internmc.facebook.com/intern/diff/D69148652/) [ghstack-poisoned]

pytorch-bot · 2025-02-13T04:04:37Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

❌ 1 New Failure

As of commit ba31994 with merge base 75d4abc ():

NEW FAILURE - The following job has failed:

pull / unittest / macos / macos-job (gh)
backends/xnnpack/test/ops/test_conv1d.py::TestConv1d::test_qs8_conv1d_batchnorm_seq

This comment was automatically generated by Dr. CI and updates every 15 minutes.

…segment 1. Implement load_into in FlatTensorDataMap 2. Do not persist 'data_ro' in the FlatTensorDataMap. From `get_data`, return the FreeableBuffer given by the data loader. TODO: add test for load_into. Differential Revision: [D69148652](https://our.internmc.facebook.com/intern/diff/D69148652/) ghstack-source-id: 266205806 Pull Request resolved: #8447

facebook-github-bot · 2025-02-13T04:04:46Z

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

lucylq · 2025-02-13T16:10:58Z

For: #8393

swolchok

don't have the context on flat_tensor to accept, here's some minor advice

swolchok · 2025-02-18T20:18:25Z

extension/flat_tensor/flat_tensor_data_map.cpp

+  if (!metadata_res.ok()) {
+    return metadata_res.error();
+  }
+  const auto metadata = metadata_res.get();
+  ET_CHECK_OR_RETURN_ERROR(
+      metadata->segment_index() >= 0 && metadata->offset() >= 0,
+      InvalidExternalData,
+      "Invalid segment_index %d or offset %lu; malformed PTD file.",
+      metadata->segment_index(),
+      metadata->offset())
+
+  Result<const TensorLayout> tensor_layout = create_tensor_layout(metadata);
+  if (!tensor_layout.ok()) {
+    return tensor_layout.error();
+  }
+  ET_CHECK_OR_RETURN_ERROR(
+      size < tensor_layout.get().nbytes(),
+      InvalidArgument,
+      "Buffer size %zu is smaller than tensor size %zu",
+      size,
+      tensor_layout.get().nbytes())
+
+  const auto* s_data_segment = flat_tensor_->segments();


this code is repeated and it's about 20 lines; I would extract a utility function here

…d onto the segment" 1. Implement load_into in FlatTensorDataMap 2. Do not persist 'data_ro' in the FlatTensorDataMap. From `get_data`, return the FreeableBuffer given by the data loader. TODO: add test for load_into. Differential Revision: [D69148652](https://our.internmc.facebook.com/intern/diff/D69148652/) [ghstack-poisoned]

facebook-github-bot · 2025-02-20T00:39:26Z

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

…segment Pull Request resolved: #8447 1. Implement load_into in FlatTensorDataMap 2. Do not persist 'data_ro' in the FlatTensorDataMap. From `get_data`, return the FreeableBuffer given by the data loader. TODO: add test for load_into. ghstack-source-id: 267313796 Differential Revision: [D69148652](https://our.internmc.facebook.com/intern/diff/D69148652/)

…d onto the segment" 1. Implement load_into in FlatTensorDataMap 2. Do not persist 'data_ro' in the FlatTensorDataMap. From `get_data`, return the FreeableBuffer given by the data loader. TODO: add test for load_into. Differential Revision: [D69148652](https://our.internmc.facebook.com/intern/diff/D69148652/) [ghstack-poisoned]

facebook-github-bot · 2025-02-20T18:20:08Z

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

…segment Pull Request resolved: #8447 1. Implement load_into in FlatTensorDataMap 2. Do not persist 'data_ro' in the FlatTensorDataMap. From `get_data`, return the FreeableBuffer given by the data loader. TODO: add test for load_into. ghstack-source-id: 267448245 Differential Revision: [D69148652](https://our.internmc.facebook.com/intern/diff/D69148652/)

…d onto the segment" 1. Implement load_into in FlatTensorDataMap 2. Do not persist 'data_ro' in the FlatTensorDataMap. From `get_data`, return the FreeableBuffer given by the data loader. TODO: add test for load_into. Differential Revision: [D69148652](https://our.internmc.facebook.com/intern/diff/D69148652/) [ghstack-poisoned]

facebook-github-bot · 2025-02-20T19:28:41Z

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

…segment Pull Request resolved: #8447 1. Implement load_into in FlatTensorDataMap 2. Do not persist 'data_ro' in the FlatTensorDataMap. From `get_data`, return the FreeableBuffer given by the data loader. TODO: add test for load_into. ghstack-source-id: 267467148 Differential Revision: [D69148652](https://our.internmc.facebook.com/intern/diff/D69148652/)

…segment (#8650) * [flat_tensor] Persist FreeableBuffers of external constants in method Pull Request resolved: #8437 ## Problem Currently, the FlatTensorDataMap persists tensors, and returns a FreeableBuffer with an empty free function. The NamedDataMap should not persist data, as most cases (eg. delegate) will want it to be freed. Ownership should be on the caller; `get_data` returns a FreeableBuffer that 'owns' the data. The FreeableBuffer in turn is owned by the caller. NOTE: this doesn't support the case where we want to share plain tensors between methods/pte files at runtime. A custom NDM could support that use-case. ## This diff: 1. Introduces a 'NamedData' struct to method.h. This holds a key and a FreeeableBuffer. 2. Iterate over all the flatbuffer tensors to count the constants tagged with EXTERNAL. NOTE: this will increase load time for all users. Potentially allocate chunks of 16 and use a linked list to store external constants, or store this number in PTE file (see D69618283). 3. Allocate space for num_external_constants using the method allocator. 4. Iterate over all flatbuffer tensors and use the named_data_map to resolve EXTERNAL tensors into the array of NamedData. 5. Pass the resolved external constants to tensor_parser, along with NDM (used for mutable external tensors). 6. Resolved external tensors are stored inside method. They are freed when the method is destructed. Some notes: https://docs.google.com/document/d/1_PBi4JgODuClUPD4PCUWrKNjyUH54zOUHGUJ3QHDNes/edit?tab=t.0#heading=h.blsvwraxss7g ghstack-source-id: 267364187 TODO: add test case when two fqns point to the same data buffer. Differential Revision: [D69477027](https://our.internmc.facebook.com/intern/diff/D69477027/) * [executorch][flat_tensor] implement load into and dont hold onto the segment Pull Request resolved: #8447 1. Implement load_into in FlatTensorDataMap 2. Do not persist 'data_ro' in the FlatTensorDataMap. From `get_data`, return the FreeableBuffer given by the data loader. TODO: add test for load_into. ghstack-source-id: 267467148 Differential Revision: [D69148652](https://our.internmc.facebook.com/intern/diff/D69148652/) --------- Co-authored-by: lucylq <lfq@meta.com>

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 Feb 13, 2025

lucylq mentioned this pull request Feb 13, 2025

[flat_tensor] Persist FreeableBuffers of external constants in method #8437

Merged

facebook-github-bot added the fb-exported label Feb 13, 2025

lucylq mentioned this pull request Feb 13, 2025

[executorch][flat tensor] Store number of external tensors in flatbuffer #8483

Open

swolchok reviewed Feb 18, 2025

View reviewed changes

lucylq requested a review from JacobSzwejbka February 20, 2025 18:20

lucylq added the topic: not user facing label Feb 20, 2025

JacobSzwejbka approved these changes Feb 24, 2025

View reviewed changes

facebook-github-bot merged commit 99dfe7e into gh/lucylq/41/base Feb 24, 2025
48 of 51 checks passed

facebook-github-bot deleted the gh/lucylq/41/head branch February 24, 2025 18:45

facebook-github-bot had a problem deploying to cherry-pick-bot February 24, 2025 18:45 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[executorch][flat_tensor] implement load into and dont hold onto the segment #8447

[executorch][flat_tensor] implement load into and dont hold onto the segment #8447

Uh oh!

lucylq commented Feb 13, 2025 •

edited

Loading

Uh oh!

pytorch-bot bot commented Feb 13, 2025 •

edited

Loading

Uh oh!

facebook-github-bot commented Feb 13, 2025

Uh oh!

lucylq commented Feb 13, 2025

Uh oh!

swolchok left a comment

Uh oh!

swolchok Feb 18, 2025

Uh oh!

lucylq Feb 20, 2025 •

edited

Loading

Uh oh!

facebook-github-bot commented Feb 20, 2025

Uh oh!

facebook-github-bot commented Feb 20, 2025

Uh oh!

facebook-github-bot commented Feb 20, 2025

Uh oh!

Uh oh!

Uh oh!

[executorch][flat_tensor] implement load into and dont hold onto the segment #8447

[executorch][flat_tensor] implement load into and dont hold onto the segment #8447

Uh oh!

Conversation

lucylq commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

❌ 1 New Failure

Uh oh!

facebook-github-bot commented Feb 13, 2025

Uh oh!

lucylq commented Feb 13, 2025

Uh oh!

swolchok left a comment

Choose a reason for hiding this comment

Uh oh!

swolchok Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

lucylq Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 20, 2025

Uh oh!

facebook-github-bot commented Feb 20, 2025

Uh oh!

facebook-github-bot commented Feb 20, 2025

Uh oh!

Uh oh!

Uh oh!

lucylq commented Feb 13, 2025 •

edited

Loading

pytorch-bot bot commented Feb 13, 2025 •

edited

Loading

lucylq Feb 20, 2025 •

edited

Loading