Re land asset management#14768
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14768
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit d4f77e5 with merge base 06ea3d6 ( BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
|
@metascroy has imported this pull request. If you are a Meta employee, you can view this in D83893376. |
|
@cymbalrush can you have a look? |
| bool is_directory_empty(NSURL *url, NSFileManager *fm, NSError * __autoreleasing *error) { | ||
| BOOL is_directory = NO; | ||
| if (![fm fileExistsAtPath:url.path isDirectory:&is_directory] && !is_directory) { | ||
| bool is_empty_directory_or_not_exist(NSURL *dirURL, NSFileManager *fm, NSError * __autoreleasing *error) { |
There was a problem hiding this comment.
nit: could we change the name to is_missing_or_empty_directory?
cymbalrush
left a comment
cymbalrush
left a comment
There was a problem hiding this comment.
Looks good, thank you!
metascroy
force-pushed
the
re-land-asset-management
branch
from
5d4e389 to
3a2fd6b
Compare
|
@metascroy has imported this pull request. If you are a Meta employee, you can view this in D83893376. |
Summary: This re-lands #13560 To see changes from first attempt, see 16ff41b...428c1c4 Several notable changes: 1. In the original PR, we removed a backup path where if the transaction to store the compiled asset fails, the prediction could still succeed (https://github.com/pytorch/executorch/pull/13560/files#r2289523798). This is restored here: 16ff41b...428c1c4#diff-ce24872f2e45ee7bf53f50b319b8f77f2f340b01b0889748deaaa754084d4fefR495-R509 2. We add new logic to re-create the asset directory if needed inside _storeAssetAtURL: 16ff41b...428c1c4#diff-4178f5b6716f52c9cb12ee99be51a9d69d3ed0e4609e9afb0429571cf9cf84a3R468 3. We add new logic to re-create the trash directory if needed inside moveItemAtURLToTrash (16ff41b...428c1c4#diff-4178f5b6716f52c9cb12ee99be51a9d69d3ed0e4609e9afb0429571cf9cf84a3R432). Previously this function was called move_to_directory. 4. The previous code had a nested transaction inside _storeAssetAtURL (because _removeAssetAtURL also starts a transaction). We remove the nested transaction by passing in alreadyInsideTransaction to _removeAssetAtURL). 5. We add new NSFileProtectionCompleteUntilFirstUserAuthentication to our directories and database files. In addition to the above changes, we cleaned up some code: 1. The function create_directory_if_needed is removed from backend_delegate.mm: 16ff41b...428c1c4#diff-89543b8e2fe49bec8243d18801819bc2594e86d60af8823c03ac2c3e6fc7607dL48. An equivalent function exists in the asset manager. 2. Introduced loadModelWithCompiledAsset and use it rather than using loadModelWithContentsOfURL (16ff41b...428c1c4#diff-ce24872f2e45ee7bf53f50b319b8f77f2f340b01b0889748deaaa754084d4fefL585) Reviewed By: mergennachin Differential Revision: D83893376 Pulled By: metascroy
facebook-github-bot
force-pushed
the
re-land-asset-management
branch
from
90a4f4a to
d4f77e5
Compare
|
@metascroy has exported this pull request. If you are a Meta employee, you can view the originating Diff in D83893376. |
Differential Revision: D83893376 Pull Request resolved: pytorch#14768
4 participants
This re-lands #13560
To see changes from first attempt, see 16ff41b...428c1c4
Several notable changes:
In the original PR, we removed a backup path where if the transaction to store the compiled asset fails, the prediction could still succeed (https://github.com/pytorch/executorch/pull/13560/files#r2289523798). This is restored here: 16ff41b...428c1c4#diff-ce24872f2e45ee7bf53f50b319b8f77f2f340b01b0889748deaaa754084d4fefR495-R509
We add new logic to re-create the asset directory if needed inside _storeAssetAtURL: 16ff41b...428c1c4#diff-4178f5b6716f52c9cb12ee99be51a9d69d3ed0e4609e9afb0429571cf9cf84a3R468
We add new logic to re-create the trash directory if needed inside moveItemAtURLToTrash (16ff41b...428c1c4#diff-4178f5b6716f52c9cb12ee99be51a9d69d3ed0e4609e9afb0429571cf9cf84a3R432). Previously this function was called move_to_directory.
The previous code had a nested transaction inside _storeAssetAtURL (because _removeAssetAtURL also starts a transaction). We remove the nested transaction by passing in alreadyInsideTransaction to _removeAssetAtURL).
We add new NSFileProtectionCompleteUntilFirstUserAuthentication to our directories and database files.
In addition to the above changes, we cleaned up some code:
The function create_directory_if_needed is removed from backend_delegate.mm: 16ff41b...428c1c4#diff-89543b8e2fe49bec8243d18801819bc2594e86d60af8823c03ac2c3e6fc7607dL48. An equivalent function exists in the asset manager.
Introduced loadModelWithCompiledAsset and use it rather than using loadModelWithContentsOfURL (16ff41b...428c1c4#diff-ce24872f2e45ee7bf53f50b319b8f77f2f340b01b0889748deaaa754084d4fefL585)