[Core ML] Improve asset management #13560

cymbalrush · 2025-08-20T20:10:38Z

Fix asset management issues in CoreML delegate

Do not use the temporary directory for asset storage.
Use the staging directory for storing temporary assets, and ensure it is cleaned up after use.
Perform aggressive cleanup of the temporary directory to avoid leftovers.
Guarantee proper cleanup of staging directories after operations to prevent stale files.

pytorch-bot · 2025-08-20T20:10:42Z

🔗 Helpful Links

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

📄 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 0541ad9 with merge base 8aca197 ():

NEW FAILURE - The following job has failed:

Build documentation / build (buck2) / Build doc (gh)
At least one of the pre-conditions you specified did not hold

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

github-actions · 2025-08-20T20:11:27Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

metascroy · 2025-08-20T22:44:10Z

backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.mm

@@ -254,6 +254,18 @@ BOOL is_asset_alive(NSMapTable<NSString *, ETCoreMLAsset *> *assets_in_use_map,

    return assets;
 }
+
+NSURL * _Nullable move_to_directory(NSURL *url,


This looks similar to the pattern "[self moveURL:dstURL toUniqueURLInDirectory:self.trashDirectoryURL error:nil];"

Should we consolidate?

Updated to use move_to_directory

metascroy · 2025-08-20T22:52:00Z

backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.mm

+                                    NSFileManager *fileManager,
+                                    NSError * __autoreleasing *error) {
+    NSURL *dstURL = [directoryURL URLByAppendingPathComponent:[NSUUID UUID].UUIDString];
+    if (![fileManager moveItemAtURL:url toURL:dstURL error:error]) {


Do we need to check if url is nil when using moveItemAtURL?

I added checks.

metascroy · 2025-08-20T22:54:18Z

backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.mm

+
+    // Remove any existing contents in the staging directory by moving them to the trash directory,
+    // then recreate a clean staging directory for new use.
+    move_to_directory([assetsDirectoryURL URLByAppendingPathComponent:@"staging"], managedTrashDirectoryURL, fileManager, nil);


What happens if we have two models?

In model2, the staging directory is being used during compilation, but then in model1 we execute this line and move the staging directory?

ETCoreMLAssetManager is only created once when the backend is loaded, and this happens atinit time.

ETCoreMLAssetManager is created once for a single PTE file on delegate init, but what if there are multiple PTE files?

Do they all share the same staging directory? And can that lead to issues when one tries to delete the staging directory when another is compiling from it?

No ETCoreMLAssetManager is only created once - https://github.com/pytorch/executorch/blob/main/backends/apple/coreml/runtime/delegate/backend_delegate.mm#L138

ETCoreMLAssetManager and ETCoreMLModelManager are only created once. You can put breakpoints to verify it.

What if I load two ET programs on two separate processes? Won't each process create ETCoreMLModelManager?

We need a persistent identifier, but I haven’t identified a suitable one yet. For now, I have removed the deletion step. Although we clean the directories in the staging area, if the app is terminated before the cleanup occurs, stale directories may remain. However, since these directories are located in the caches directory, the OS will automatically clean them up when storage is low.

Sounds good. I guess we could clean up things in staging that have last touched time stamps > 1 day during asset manager creation.

metascroy · 2025-08-20T23:04:08Z

backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.mm

-
+
+    // Always clean the trash directory to ensure a minimal footprint.
+    // The `trashQueue` is serialized, so only one cleanup will run at a time.


What happens if something is moving to the trashDirectory when it's being deleted by another thread?

For example, withTemporaryDirectory moves staging files to the trashDirectory after compilation, but then another model triggers cleanupTrashDirectory on another thread.

That should be fine trashQueue is serial and we get the list of directories to delete at the start of the method

NSDirectoryEnumerator *enumerator = [fileManager enumeratorAtURL:self.trashDirectoryURL includingPropertiesForKeys:@[] options:NSDirectoryEnumerationSkipsSubdirectoryDescendants errorHandler:errorHandler];

metascroy · 2025-08-20T23:05:38Z

backends/apple/coreml/runtime/delegate/ETCoreMLModelLoader.mm

-    }
-
-    // If store failed then we will load the model from compiledURL.
-    auto backingAsset = Asset::make(compiledModelURL, identifier, assetManager.fileManager, error);


Why are we removing this?

If store fails then it's an error there is no point recovering from it.

metascroy · 2025-08-20T23:09:11Z

backends/apple/coreml/runtime/delegate/ETCoreMLModelManager.mm

+
+    // If modelURL is not provided, write model files to the destination directory (dstURL)
+    // and obtain a URL pointing to them. Otherwise, use the provided modelURL.
+    modelURL = (modelURL == nil) ? ::write_model_files(dstURL, self.fileManager, identifier, modelAssetType.value(), inMemoryFS, error) : modelURL;


When would modelURL be non-null? Is it mainly the case where the model is already compiled in the PTE?

In the case of Analyzer we also save the raw model and we don't want to write the model files again if it already exists.

metascroy · 2025-08-20T23:10:22Z

backends/apple/coreml/runtime/delegate/ETCoreMLModelManager.mm

            return compiledModelURL;
        }
    }
 }

-#if ET_EVENT_TRACER_ENABLED


We are we removing the compile flag here?

No, moving it.

metascroy · 2025-08-20T23:25:33Z

backends/apple/coreml/runtime/delegate/ETCoreMLModelManager.mm

+                                        inMemoryFS:(const inmemoryfs::InMemoryFileSystem*)inMemoryFS
+                                             error:(NSError * __autoreleasing *)error {
+    NSString *identifier = @(metadata.identifier.c_str());
+    NSString *rawIdentifier = raw_model_identifier(identifier);


What does raw here mean? Is it to distinguish from the compiled asset (which gets the unqualified identified as its name)?

facebook-github-bot · 2025-08-21T16:29:25Z

@metascroy has imported this pull request. If you are a Meta employee, you can view this in D80715432.

facebook-github-bot · 2025-08-21T21:31:11Z

@metascroy has imported this pull request. If you are a Meta employee, you can view this in D80715432.

metascroy · 2025-08-21T21:34:07Z

Overall changes look great @cymbalrush

Running internal/external CI. It does look like the linter is failing. Can you run lint?

From ET repo:

pip install -r requirements-lintrunner.txt

And then run:

lintrunner -a

facebook-github-bot · 2025-08-22T21:57:08Z

@metascroy has imported this pull request. If you are a Meta employee, you can view this in D80715432.

metascroy · 2025-08-22T22:14:46Z

Attemping land from phabricator

cymbalrush requested review from shoumikhin and cccclai as code owners August 20, 2025 20:10

meta-cla 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 Aug 20, 2025

cymbalrush force-pushed the fix_assets branch from 8349d3f to fc8e5f7 Compare August 20, 2025 20:13

cymbalrush changed the title ~~Improve asset management~~ [Core ML] Improve asset management Aug 20, 2025

cymbalrush force-pushed the fix_assets branch 3 times, most recently from 64ecf65 to 8f19e4a Compare August 20, 2025 20:30

metascroy added the ciflow/trunk label Aug 20, 2025

metascroy reviewed Aug 20, 2025

View reviewed changes

pytorch-bot bot removed the ciflow/trunk label Aug 21, 2025

cymbalrush force-pushed the fix_assets branch from cebcfd3 to cfe86bf Compare August 21, 2025 18:52

cymbalrush added 2 commits August 21, 2025 12:38

Improve asset management

6c5bb97

Address comemnts

e1187a1

cymbalrush force-pushed the fix_assets branch from 47a4f9a to e1187a1 Compare August 21, 2025 19:41

Merge branch 'main' into fix_assets

de47c33

metascroy approved these changes Aug 21, 2025

View reviewed changes

metascroy added the ciflow/trunk label Aug 21, 2025

Merge branch 'main' into fix_assets

0bb893e

pytorch-bot bot removed the ciflow/trunk label Aug 22, 2025

lint

0541ad9

metascroy merged commit de17042 into pytorch:main Aug 23, 2025
111 of 112 checks passed



		// Always clean the trash directory to ensure a minimal footprint.
		// The `trashQueue` is serialized, so only one cleanup will run at a time.

[Core ML] Improve asset management #13560

[Core ML] Improve asset management #13560

Uh oh!

Conversation

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

Uh oh!

Fix asset management issues in CoreML delegate

Uh oh!

pytorch-bot bot commented Aug 20, 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/13560

❌ 1 New Failure

Uh oh!

github-actions bot commented Aug 20, 2025

This PR needs a release notes: label

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metascroy Aug 21, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Aug 21, 2025

Uh oh!

facebook-github-bot commented Aug 21, 2025

Uh oh!

metascroy commented Aug 21, 2025

Uh oh!

facebook-github-bot commented Aug 22, 2025

Uh oh!

metascroy commented Aug 22, 2025

Uh oh!

Uh oh!

Uh oh!

cymbalrush commented Aug 20, 2025 •

edited

Loading

pytorch-bot bot commented Aug 20, 2025 •

edited

Loading

This PR needs a `release notes:` label

metascroy Aug 21, 2025 •

edited

Loading