Skip to content

Commit 31b766b

Browse files
cymbalrushfacebook-github-bot
cymbalrush
authored and
facebook-github-bot
committed
Fix test failures (#3876)
Summary: Fixes two issues - Add model is now `x + y` earlier it used to be ``` int z = x + y; z = z + x; z = z + x; z = z + z; ``` - CoreML delegate was loading model manager at static initialization and there is a chance that executorch runtime might not have been initialized. `ETCoreMLModelManagerDelegate` loads it lazily, so that the runtime is guaranteed to be initialized. Pull Request resolved: #3876 Reviewed By: cccclai Differential Revision: D58257664 Pulled By: mergennachin fbshipit-source-id: bd8197d552572134b79ef777da85799e07b257dd
1 parent 2ee83be commit 31b766b

13 files changed

+206
-64
lines changed

‎backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ NS_ASSUME_NONNULL_BEGIN
104104
///
105105
/// @param error On failure, error is filled with the failure information.
106106
/// @retval `YES` is the assets are purged otherwise `NO`.
107-
- (BOOL)purge:(NSError* __autoreleasing*)error;
107+
- (BOOL)purgeAndReturnError:(NSError* __autoreleasing*)error;
108108

109109
/// The estimated size of the assets store. The returned value might not correct if the asset
110110
/// directory is tampered externally.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -740,7 +740,7 @@ - (BOOL)_purge:(NSError * __autoreleasing *)error {
740740
return static_cast<BOOL>(status);
741741
}
742742

743-
- (BOOL)purge:(NSError * __autoreleasing *)error {
743+
- (BOOL)purgeAndReturnError:(NSError * __autoreleasing *)error {
744744
__block BOOL result = 0;
745745
dispatch_sync(self.syncQueue, ^{
746746
result = [self _purge:error];

‎backends/apple/coreml/runtime/delegate/ETCoreMLModelManager.h‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,11 @@ __attribute__((objc_subclassing_restricted))
103103
/// @retval `YES` if the model was pre-warmed otherwise `NO`.
104104
- (BOOL)prewarmModelWithHandle:(ModelHandle*)handle error:(NSError* __autoreleasing*)error;
105105

106-
/// The `ETCoreMLAssetManager` instance used to manage models cache.
107-
@property (strong, readonly, nonatomic) ETCoreMLAssetManager* assetManager;
106+
/// Purges model cache.
107+
///
108+
/// @param error On failure, error is filled with the failure information.
109+
/// @retval `YES` if the cache is purged otherwise `NO`.
110+
- (BOOL)purgeModelsCacheAndReturnError:(NSError* __autoreleasing*)error;
108111

109112
@end
110113

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,7 @@ @interface ETCoreMLModelManager () {
352352
}
353353

354354
@property (nonatomic, readonly, strong) NSFileManager *fileManager;
355+
@property (strong, readonly, nonatomic) ETCoreMLAssetManager* assetManager;
355356
@property (nonatomic, readonly, strong) NSMutableDictionary<NSValue *, id<ETCoreMLModelExecutor>> *handleToExecutorMap;
356357
@property (nonatomic, readonly, strong) NSMapTable<NSString *, dispatch_queue_t> *modelIdentifierToLoadingQueueMap;
357358
@property (nonatomic, readonly, strong) NSMutableDictionary<NSString *, ETCoreMLAsset *> *modelIdentifierToPrewarmedAssetMap;
@@ -832,4 +833,8 @@ - (BOOL)unloadModelWithHandle:(ModelHandle *)handle {
832833
return result;
833834
}
834835

836+
- (BOOL)purgeModelsCacheAndReturnError:(NSError *__autoreleasing *)error {
837+
return [self.assetManager purgeAndReturnError:error];
838+
}
839+
835840
@end

‎backends/apple/coreml/runtime/delegate/backend_delegate.mm‎

Lines changed: 154 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
#import <ETCoreMLModel.h>
1111
#import <ETCoreMLModelManager.h>
1212
#import <ETCoreMLStrings.h>
13-
#import <atomic>
1413
#import <backend_delegate.h>
1514
#import <model_event_logger.h>
1615
#import <multiarray.h>
@@ -88,6 +87,153 @@ MLComputeUnits get_compute_units(const Buffer& buffer) {
8887
}
8988
} //namespace
9089

90+
@interface ETCoreMLModelManagerDelegate : NSObject
91+
92+
- (instancetype)init NS_UNAVAILABLE;
93+
94+
+ (instancetype)new NS_UNAVAILABLE;
95+
96+
- (instancetype)initWithConfig:(BackendDelegate::Config)config NS_DESIGNATED_INITIALIZER;
97+
98+
- (BOOL)loadAndReturnError:(NSError * _Nullable __autoreleasing *)error;
99+
100+
- (void)loadAsynchronously;
101+
102+
- (ModelHandle*)loadModelFromAOTData:(NSData*)data
103+
configuration:(MLModelConfiguration*)configuration
104+
error:(NSError* __autoreleasing*)error;
105+
106+
- (BOOL)executeModelWithHandle:(ModelHandle*)handle
107+
argsVec:(const std::vector<executorchcoreml::MultiArray>&)argsVec
108+
loggingOptions:(const executorchcoreml::ModelLoggingOptions&)loggingOptions
109+
eventLogger:(const executorchcoreml::ModelEventLogger* _Nullable)eventLogger
110+
error:(NSError* __autoreleasing*)error;
111+
112+
- (BOOL)unloadModelWithHandle:(ModelHandle*)handle;
113+
114+
- (BOOL)purgeModelsCacheAndReturnError:(NSError * _Nullable __autoreleasing *)error;
115+
116+
@property (assign, readonly, nonatomic) BackendDelegate::Config config;
117+
@property (strong, readonly, nonatomic) dispatch_queue_t syncQueue;
118+
@property (strong, nonatomic, nullable) ETCoreMLModelManager *impl;
119+
@property (assign, readonly, nonatomic) BOOL isAvailable;
120+
121+
@end
122+
123+
@implementation ETCoreMLModelManagerDelegate
124+
125+
- (instancetype)initWithConfig:(BackendDelegate::Config)config {
126+
self = [super init];
127+
if (self) {
128+
_config = std::move(config);
129+
_syncQueue = dispatch_queue_create("com.executorchcoreml.modelmanagerdelegate.sync", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
130+
}
131+
132+
return self;
133+
}
134+
135+
- (BOOL)_loadAndReturnError:(NSError * _Nullable __autoreleasing *)error {
136+
if (self.impl != nil) {
137+
return YES;
138+
}
139+
140+
ETCoreMLAssetManager *assetManager = create_asset_manager(ETCoreMLStrings.assetsDirectoryPath,
141+
ETCoreMLStrings.trashDirectoryPath,
142+
ETCoreMLStrings.databaseDirectoryPath,
143+
ETCoreMLStrings.databaseName,
144+
self.config.max_models_cache_size,
145+
error);
146+
if (!assetManager) {
147+
return NO;
148+
}
149+
150+
ETCoreMLModelManager *modelManager = [[ETCoreMLModelManager alloc] initWithAssetManager:assetManager];
151+
if (!modelManager) {
152+
return NO;
153+
}
154+
155+
self.impl = modelManager;
156+
157+
if (self.config.should_prewarm_asset) {
158+
[modelManager prewarmRecentlyUsedAssetsWithMaxCount:1];
159+
}
160+
161+
return YES;
162+
}
163+
164+
- (BOOL)loadAndReturnError:(NSError * _Nullable __autoreleasing *)error {
165+
__block NSError *localError = nil;
166+
__block BOOL result = NO;
167+
dispatch_sync(self.syncQueue, ^{
168+
result = [self _loadAndReturnError:&localError];
169+
});
170+
171+
if (error) {
172+
*error = localError;
173+
}
174+
175+
return result;
176+
}
177+
178+
- (void)loadAsynchronously {
179+
dispatch_async(self.syncQueue, ^{
180+
(void)[self _loadAndReturnError:nil];
181+
});
182+
}
183+
184+
- (ModelHandle*)loadModelFromAOTData:(NSData*)data
185+
configuration:(MLModelConfiguration*)configuration
186+
error:(NSError* __autoreleasing*)error {
187+
if (![self loadAndReturnError:error]) {
188+
return nil;
189+
}
190+
191+
return [self.impl loadModelFromAOTData:data
192+
configuration:configuration
193+
error:error];
194+
}
195+
196+
- (BOOL)executeModelWithHandle:(ModelHandle*)handle
197+
argsVec:(const std::vector<executorchcoreml::MultiArray>&)argsVec
198+
loggingOptions:(const executorchcoreml::ModelLoggingOptions&)loggingOptions
199+
eventLogger:(const executorchcoreml::ModelEventLogger* _Nullable)eventLogger
200+
error:(NSError* __autoreleasing*)error {
201+
assert(self.impl != nil && "Impl must not be nil");
202+
return [self.impl executeModelWithHandle:handle
203+
argsVec:argsVec
204+
loggingOptions:loggingOptions
205+
eventLogger:eventLogger
206+
error:error];
207+
}
208+
209+
- (nullable ETCoreMLModel*)modelWithHandle:(ModelHandle*)handle {
210+
assert(self.impl != nil && "Impl must not be nil");
211+
return [self.impl modelWithHandle:handle];
212+
}
213+
214+
- (BOOL)unloadModelWithHandle:(ModelHandle*)handle {
215+
assert(self.impl != nil && "Impl must not be nil");
216+
return [self.impl unloadModelWithHandle:handle];
217+
}
218+
219+
- (BOOL)purgeModelsCacheAndReturnError:(NSError * _Nullable __autoreleasing *)error {
220+
if (![self loadAndReturnError:error]) {
221+
return NO;
222+
}
223+
224+
return [self.impl purgeModelsCacheAndReturnError:error];;
225+
}
226+
227+
- (BOOL)isAvailable {
228+
if (![self loadAndReturnError:nil]) {
229+
return NO;
230+
}
231+
232+
return YES;
233+
}
234+
235+
@end
236+
91237
namespace executorchcoreml {
92238

93239
std::string BackendDelegate::ErrorCategory::message(int code) const {
@@ -114,20 +260,9 @@ MLComputeUnits get_compute_units(const Buffer& buffer) {
114260
class BackendDelegateImpl: public BackendDelegate {
115261
public:
116262
explicit BackendDelegateImpl(const Config& config) noexcept
117-
:BackendDelegate(), config_(config) {
118-
NSError *localError = nil;
119-
ETCoreMLAssetManager *asset_manager = create_asset_manager(ETCoreMLStrings.assetsDirectoryPath,
120-
ETCoreMLStrings.trashDirectoryPath,
121-
ETCoreMLStrings.databaseDirectoryPath,
122-
ETCoreMLStrings.databaseName,
123-
config.max_models_cache_size,
124-
&localError);
125-
126-
model_manager_ = (asset_manager != nil) ? [[ETCoreMLModelManager alloc] initWithAssetManager:asset_manager] : nil;
127-
if (model_manager_ != nil && config_.should_prewarm_asset) {
128-
[model_manager_ prewarmRecentlyUsedAssetsWithMaxCount:1];
129-
}
130-
available_.store(model_manager_ != nil, std::memory_order_seq_cst);
263+
:BackendDelegate(), model_manager_([[ETCoreMLModelManagerDelegate alloc] initWithConfig:config])
264+
{
265+
[model_manager_ loadAsynchronously];
131266
}
132267

133268
BackendDelegateImpl(BackendDelegateImpl const&) = delete;
@@ -142,11 +277,6 @@ explicit BackendDelegateImpl(const Config& config) noexcept
142277
ModelHandle *modelHandle = [model_manager_ loadModelFromAOTData:data
143278
configuration:configuration
144279
error:&localError];
145-
if (modelHandle && config_.should_prewarm_model) {
146-
NSError *localError = nil;
147-
[model_manager_ prewarmModelWithHandle:modelHandle error:&localError];
148-
}
149-
150280
return modelHandle;
151281
}
152282

@@ -158,7 +288,7 @@ bool execute(Handle* handle,
158288
NSError *error = nil;
159289
if (![model_manager_ executeModelWithHandle:handle
160290
argsVec:args
161-
loggingOptions:logging_options
291+
loggingOptions:logging_options
162292
eventLogger:event_logger
163293
error:&error]) {
164294
ec = static_cast<ErrorCode>(error.code);
@@ -173,7 +303,7 @@ bool is_valid_handle(Handle* handle) const noexcept override {
173303
}
174304

175305
bool is_available() const noexcept override {
176-
return available_.load(std::memory_order_acquire);
306+
return static_cast<bool>(model_manager_.isAvailable);
177307
}
178308

179309
std::pair<size_t, size_t> get_num_arguments(Handle* handle) const noexcept override {
@@ -187,12 +317,11 @@ void destroy(Handle* handle) const noexcept override {
187317

188318
bool purge_models_cache() const noexcept override {
189319
NSError *localError = nil;
190-
bool result = static_cast<bool>([model_manager_.assetManager purge:&localError]);
320+
bool result = static_cast<bool>([model_manager_ purgeModelsCacheAndReturnError:&localError]);
191321
return result;
192322
}
193323

194-
ETCoreMLModelManager *model_manager_;
195-
std::atomic<bool> available_;
324+
ETCoreMLModelManagerDelegate *model_manager_;
196325
Config config_;
197326
};
198327

‎backends/apple/coreml/runtime/delegate/coreml_backend_delegate.mm‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#import <backend_delegate.h>
1212
#import <coreml_backend/delegate.h>
1313
#import <executorch/runtime/core/evalue.h>
14+
#import <executorch/runtime/platform/log.h>
1415
#import <memory>
1516
#import <model_event_logger.h>
1617
#import <model_logging_options.h>

‎backends/apple/coreml/runtime/test/BackendDelegateTests.mm‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#import <model_logging_options.h>
1616
#import <multiarray.h>
1717
#import <objc_array_util.h>
18+
#import <executorch/runtime/platform/runtime.h>
1819

1920
using namespace executorchcoreml;
2021

@@ -58,6 +59,10 @@ + (nullable NSURL *)bundledResourceWithName:(NSString *)name extension:(NSString
5859
return [bundle URLForResource:name withExtension:extension];
5960
}
6061

62+
+ (void)setUp {
63+
torch::executor::runtime_init();
64+
}
65+
6166
- (void)setUp {
6267
@autoreleasepool {
6368
_delegate = BackendDelegate::make(BackendDelegate::Config());
@@ -151,9 +156,6 @@ - (void)testAddModelExecution {
151156
int y = 50;
152157
// add_coreml_all does the following operations.
153158
int z = x + y;
154-
z = z + x;
155-
z = z + x;
156-
z = z + z;
157159

158160
NSArray<MLMultiArray *> *inputs = [ETCoreMLTestUtils inputsForModel:model repeatedValues:@[@(x), @(y)] error:&localError];
159161
XCTAssertNotNil(inputs);

‎backends/apple/coreml/runtime/test/CoreMLBackendDelegateTests.mm‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,12 @@
66
// Please refer to the license found in the LICENSE file in the root directory of the source tree.
77

88
#import <XCTest/XCTest.h>
9-
10-
#import <string>
11-
129
#import <executorch/runtime/core/data_loader.h>
1310
#import <executorch/runtime/core/exec_aten/testing_util/tensor_factory.h>
1411
#import <executorch/runtime/executor/method.h>
1512
#import <executorch/runtime/executor/program.h>
1613
#import <executorch/runtime/platform/runtime.h>
14+
#import <string>
1715

1816
static constexpr size_t kRuntimeMemorySize = 50 * 1024U * 1024U; // 50 MB
1917

@@ -128,7 +126,7 @@ @interface CoreMLBackendDelegateTests : XCTestCase
128126
@implementation CoreMLBackendDelegateTests
129127

130128
+ (void)setUp {
131-
runtime_init();
129+
torch::executor::runtime_init();
132130
}
133131

134132
+ (nullable NSURL *)bundledResourceWithName:(NSString *)name extension:(NSString *)extension {

‎backends/apple/coreml/runtime/test/ETCoreMLAssetManagerTests.mm‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,11 @@
66
//
77
// Please refer to the license found in the LICENSE file in the root directory of the source tree.
88

9-
#import <XCTest/XCTest.h>
10-
9+
#import "ETCoreMLTestUtils.h"
1110
#import <ETCoreMLAsset.h>
1211
#import <ETCoreMLAssetManager.h>
13-
14-
#import "ETCoreMLTestUtils.h"
12+
#import <XCTest/XCTest.h>
13+
#import <executorch/runtime/platform/runtime.h>
1514

1615
@interface ETCoreMLAssetManagerTests : XCTestCase
1716

@@ -23,6 +22,10 @@ @interface ETCoreMLAssetManagerTests : XCTestCase
2322

2423
@implementation ETCoreMLAssetManagerTests
2524

25+
+ (void)setUp {
26+
torch::executor::runtime_init();
27+
}
28+
2629
- (void)setUp {
2730
@autoreleasepool {
2831
NSError *localError = nil;
@@ -145,7 +148,7 @@ - (void)testPurge {
145148
// Close the asset so that it could be deleted.
146149
[asset close];
147150
}
148-
XCTAssertTrue([self.assetManager purge:&localError]);
151+
XCTAssertTrue([self.assetManager purgeAndReturnError:&localError]);
149152
}
150153

151154
@end

0 commit comments

Comments
 (0)