[CoreML] Create EP by AppendExecutionProvider #22675

wejoncy · 2024-10-31T11:17:08Z

Description

Motivation and Context

onnxruntime/core/providers/coreml/coreml_execution_provider.cc

onnxruntime/test/providers/coreml/coreml_basic_test.cc

csharp/src/Microsoft.ML.OnnxRuntime/SessionOptions.shared.cs

js/react_native/ios/OnnxruntimeModule.mm

skottmckay · 2024-11-06T05:51:52Z

onnxruntime/core/providers/coreml/coreml_execution_provider.cc

      coreml_version_(coreml::util::CoreMLVersion()) {
+  coreml_flags_ = options.count("coreml_flags") ? std::stoi(options.at("coreml_flags")) : 0;


nit: would be good to define the option names as constants in a header

Can we also add options so the current flags don't need to be used? Some might fold together like the flags to choose device might be in an option called 'MLComputeUnits' and allow 'MLComputeUnitsCPUAndNeuralEngine|MLComputeUnitsCPUAndGPU|MLComputeUnitsCPUOnly|MLComputeUnitsAll' to be provided as a value. #Closed

define the option names as constants in a header

seems not avaible for now. Other EPs used option names as well and we don't have onnxruntime_provider_options_config_keys.h.
Too much changes involved for this pattern.

we can support both provider_options: "MLComputeUnits" and "coreml_flags". "coreml_flags" is compataible with the old API which is essential as the old API is used in many place.

Can the option names be defined in include\onnxruntime\core\providers\coreml\coreml_provider_factory.h?

Optional for an external user to include that file with the new setup.

Option names are completely EP specific so I don't think we'd want an onnxruntime_provider_options_config_keys.h.

I wasn't suggesting getting rid of support for the existing coreml_flags. Just that a user should be forced to use coreml_flags for older settings, and an option key/value for newer ones. Ideally it's one (old style) or the other (new style).

onnxruntime/core/session/provider_registration.cc

skottmckay · 2024-11-06T06:11:30Z

onnxruntime/test/perftest/ort_test_session.cc

@@ -439,7 +439,7 @@ select from 'TF8', 'TF16', 'UINT8', 'FLOAT', 'ITENSOR'. \n)");
      }
    }
    // COREML_FLAG_CREATE_MLPROGRAM
-    Ort::ThrowOnError(OrtSessionOptionsAppendExecutionProvider_CoreML(session_options, coreml_flags));
+    session_options.AppendExecutionProvider("Coreml", {{"coreml_flags", std::to_string(coreml_flags)}});


May be better to convert this is just pass through option key/value pairs instead of setting flags (assuming we add options that cover the current flags). That way it doesn't need an update in the future.

We should have some shared parsing code though instead of so many different EPs parsing for a '|' to find the key and value. It's a little ridiculous that DNNL, CUDA, TensorRT, QNN, SNPE, DML, ACL and VitisAI are all repeating the same basic logic. Maybe ParseSessionConfigs in test/perftest/command_args_parser.cc can be used. Move it into a shared helper and call to create unordered_map<string, string> for the key/value pairs. The EP specific code can validate values using that map.

onnxruntime/test/perftest/strings_helper.cc

github-actions

You can commit the suggested changes from lintrunner.

onnxruntime/test/perftest/ort_test_session.cc

onnxruntime/test/perftest/strings_helper.cc

github-actions

You can commit the suggested changes from lintrunner.

onnxruntime/core/providers/coreml/coreml_provider_factory.cc

github-actions

You can commit the suggested changes from lintrunner.

objectivec/ort_coreml_execution_provider.mm

objectivec/test/ort_session_test.mm

skottmckay · 2024-11-21T05:17:31Z

include/onnxruntime/core/providers/coreml/coreml_provider_factory.h

+static const char* const kCoremlProviderOption_MLModelFormat = "MLModelFormat";
+static const char* const kCoremlProviderOption_MLAllowStaticInputShapes = "MLAllowStaticInputShapes";
+static const char* const kCoremlProviderOption_MLEnableOnSubgraphs = "MLEnableOnSubgraphs";
+static const char* const kCoremlProviderOption_MLModelCacheDir = "MLModelCacheDir";


Do we need the 'ML' prefix on these values? It makes sense to me for MLComputeUnits as that maps to a CoreML enum, but I'm not sure that any of the others do.

We should also provide clear documentation on what is valid/expected for each value even if it's simply copied from the flags doco.

A user might discover this file first so would also be good to mention that these values are intended to be used with Ort.::SessionOptions::AppendExecutionProvider (C++ API)/SessionOptionsAppendExecutionProvider (C API). #Closed

objectivec/ort_coreml_execution_provider.mm

skottmckay · 2024-11-21T05:24:46Z

objectivec/include/ort_coreml_execution_provider.h

+ * @param error Optional error information set if an error occurs.
+ * @return Whether the provider was enabled successfully.
+ */
+- (BOOL)appendCoreMLExecutionProviderWithOptions_v2:(NSDictionary*)provider_options


A user of appendCoreMLExecutionProviderWithOptions can easily look at the documentation for ORTCoreMLExecutionProviderOptions to know what is available. How should they discover the provider options that can be used here?

Good catch. All avaiable key-valus are listed in the comments.

skottmckay · 2024-11-21T05:29:59Z

onnxruntime/core/providers/coreml/coreml_execution_provider.h

@@ -12,9 +12,14 @@ namespace coreml {
 class Model;
 }

+struct CoreMLOptions {
+  uint32_t coreml_flags = 0;
+  std::string cache_path;


nit: can we keep changes to add supporting a cache path in a separate PR?

Also wondering if this is going to grow better if we keep it separate from coreml_flags. It's a little hard to see what is supported with the current setup. If we define the full set of options in this struct, and we populate those from either the flags, or the provider options, it's clear what the full set of options is.

I'd suggest making CoreMLOptions a class and make it responsible for validation. Can have a ctor for flags and one for provider options. That way all the validation is done in one place and it's clear which class is responsible for validation.

Yeah, it's good to seperate cache path in another PR.

If we make different flag has its own variable to mark it. How can we capatiable it with current coreml_flags?
Besides, we can easily pass coreml_flags(uint32_t) to object-c. can we pass a class from c++ to obect-c directly?

onnxruntime/test/perftest/command_args_parser.cc

include/onnxruntime/core/providers/coreml/coreml_provider_factory.h

skottmckay · 2024-11-21T05:54:29Z

onnxruntime/test/perftest/strings_helper.cc

+      // Error: must use a '|' to separate the key and value for session configuration entries.
+      return false;
+    }
+
+    std::string key(token_sv.substr(0, pos));
+    std::string value(token_sv.substr(pos + 1));
+
+    auto it = session_configs.find(key);
+    if (it != session_configs.end()) {
+      // Error: specified duplicate session configuration entry: {key}


Should we throw from here with the specific error info instead of duplicating the throw with the 'use | to separate values' message in multiple places, which may not be the actual cause of the failure? #Closed

Sounds good. I add another available_keys for early check.

onnxruntime/test/providers/coreml/dynamic_input_test.cc

Co-authored-by: Scott McKay <[email protected]>

objectivec/ort_coreml_execution_provider.mm

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

include/onnxruntime/core/providers/coreml/coreml_provider_factory.h

objectivec/include/ort_coreml_execution_provider.h

include/onnxruntime/core/providers/coreml/coreml_provider_factory.h

onnxruntime/test/perftest/ort_test_session.cc

Co-authored-by: Scott McKay <[email protected]>

github-actions

You can commit the suggested changes from lintrunner.

onnxruntime/core/providers/coreml/coreml_execution_provider.h

Co-authored-by: Scott McKay <[email protected]>

github-actions

You can commit the suggested changes from lintrunner.

onnxruntime/core/providers/coreml/coreml_execution_provider.h

wejoncy changed the title ~~genenral appendEP for coreml~~ [CoreML] Support to use general API to create EP Nov 1, 2024

wejoncy force-pushed the jicwen/coreml_aep branch 2 times, most recently from 96b7f1d to 530000a Compare November 4, 2024 09:30

github-actions bot reviewed Nov 4, 2024

View reviewed changes

onnxruntime/core/providers/coreml/coreml_execution_provider.cc Outdated Show resolved Hide resolved

onnxruntime/test/providers/coreml/coreml_basic_test.cc Outdated Show resolved Hide resolved

wejoncy changed the title ~~[CoreML] Support to use general API to create EP~~ [CoreML] Create EP by AppendExecutionProvider Nov 4, 2024

wejoncy force-pushed the jicwen/coreml_aep branch from fb7042f to 2339e3d Compare November 4, 2024 14:09

skottmckay reviewed Nov 6, 2024

View reviewed changes

github-advanced-security bot found potential problems Nov 6, 2024

View reviewed changes

onnxruntime/test/perftest/strings_helper.cc Fixed Show fixed Hide fixed

github-actions bot reviewed Nov 6, 2024

View reviewed changes

refactor coreml EP creater

43c882f

wejoncy force-pushed the jicwen/coreml_aep branch from 261d1bc to 43c882f Compare November 7, 2024 11:02

wejoncy added 4 commits November 7, 2024 03:20

refactor coreml test option

a78de11

define constant name to replace raw string

f25b18f

fix coreml_flag

4ee1cee

fix

8c12932

wejoncy marked this pull request as ready for review November 9, 2024 08:51

wejoncy mentioned this pull request Nov 15, 2024

[Documentation] CoreML python support and how to switch to MLProgram #22792

Open

add python new api

f710c14

github-actions bot reviewed Nov 18, 2024

View reviewed changes

onnxruntime/core/providers/coreml/coreml_provider_factory.cc Outdated Show resolved Hide resolved

add object-c api

ece5e58

github-actions bot reviewed Nov 19, 2024

View reviewed changes

objectivec/ort_coreml_execution_provider.mm Outdated Show resolved Hide resolved

objectivec/test/ort_session_test.mm Outdated Show resolved Hide resolved

objectivec/test/ort_session_test.mm Outdated Show resolved Hide resolved

wejoncy added 2 commits November 18, 2024 20:35

format

f6e2cbe

add ane

60f6624

wejoncy requested a review from skottmckay November 20, 2024 09:10

skottmckay reviewed Nov 21, 2024

View reviewed changes

Apply suggestions from code review

373cf8f

Co-authored-by: Scott McKay <[email protected]>

github-actions bot reviewed Nov 21, 2024

View reviewed changes

objectivec/ort_coreml_execution_provider.mm Outdated Show resolved Hide resolved

wejoncy and others added 3 commits November 21, 2024 16:28

Update objectivec/ort_coreml_execution_provider.mm

b81160f

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Adress comments

6479c13

comments

cb75741

wejoncy added 2 commits November 21, 2024 21:39

most likly key

fbc166c

remove key suggestion

7ac7cfd

wejoncy force-pushed the jicwen/coreml_aep branch from 44c9e88 to 7ac7cfd Compare November 22, 2024 05:40

skottmckay reviewed Nov 22, 2024

View reviewed changes

wejoncy and others added 2 commits November 22, 2024 01:25

unify coreml options

cfcc3ab

Apply suggestions from code review

c49f250

Co-authored-by: Scott McKay <[email protected]>

github-actions bot reviewed Nov 22, 2024

View reviewed changes

onnxruntime/core/providers/coreml/coreml_execution_provider.h Outdated Show resolved Hide resolved

Update onnxruntime/test/perftest/ort_test_session.cc

07f5772

Co-authored-by: Scott McKay <[email protected]>

github-actions bot reviewed Nov 22, 2024

View reviewed changes

onnxruntime/core/providers/coreml/coreml_execution_provider.h Outdated Show resolved Hide resolved

wejoncy added 3 commits November 22, 2024 01:34

return void for ParseSessionConfigs

a732326

not throw for first_entry_args

3e5386d

misspelling

9e7c87a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CoreML] Create EP by AppendExecutionProvider #22675

[CoreML] Create EP by AppendExecutionProvider #22675

wejoncy commented Oct 31, 2024

skottmckay Nov 6, 2024 •

edited

Loading

wejoncy Nov 6, 2024 •

edited

Loading

wejoncy Nov 6, 2024

skottmckay Nov 6, 2024

skottmckay Nov 6, 2024

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

skottmckay Nov 21, 2024 •

edited

Loading

skottmckay Nov 21, 2024

wejoncy Nov 21, 2024

skottmckay Nov 21, 2024

wejoncy Nov 21, 2024

skottmckay Nov 21, 2024 •

edited

Loading

wejoncy Nov 21, 2024

github-actions bot left a comment

github-actions bot left a comment

		coreml_version_(coreml::util::CoreMLVersion()) {
		coreml_flags_ = options.count("coreml_flags") ? std::stoi(options.at("coreml_flags")) : 0;

[CoreML] Create EP by AppendExecutionProvider #22675

Are you sure you want to change the base?

[CoreML] Create EP by AppendExecutionProvider #22675

Conversation

wejoncy commented Oct 31, 2024

Description

Motivation and Context

skottmckay Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

wejoncy Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

skottmckay Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skottmckay Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

skottmckay Nov 6, 2024 •

edited

Loading

wejoncy Nov 6, 2024 •

edited

Loading

skottmckay Nov 21, 2024 •

edited

Loading

skottmckay Nov 21, 2024 •

edited

Loading