Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CoreML] Create EP by AppendExecutionProvider #22675
base: main
Are you sure you want to change the base?
[CoreML] Create EP by AppendExecutionProvider #22675
Changes from 7 commits
43c882f
a78de11
f25b18f
4ee1cee
8c12932
f710c14
ece5e58
f6e2cbe
60f6624
373cf8f
b81160f
6479c13
cb75741
fbc166c
7ac7cfd
cfcc3ab
c49f250
07f5772
a732326
3e5386d
9e7c87a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Check warning on line 82 in objectivec/include/ort_coreml_execution_provider.h
GitHub Actions / Optional Lint C++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. All avaiable key-valus are listed in the comments.
Check warning on line 83 in objectivec/include/ort_coreml_execution_provider.h
GitHub Actions / Optional Lint C++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Check warning on line 22 in onnxruntime/core/providers/coreml/coreml_execution_provider.h
GitHub Actions / Optional Lint C++
Check warning on line 22 in onnxruntime/core/providers/coreml/coreml_provider_factory.cc
GitHub Actions / Optional Lint C++
Check warning on line 26 in onnxruntime/core/providers/coreml/coreml_provider_factory.cc
GitHub Actions / Optional Lint C++
Check warning on line 26 in onnxruntime/core/providers/coreml/coreml_provider_factory.cc
GitHub Actions / Optional Lint C++
Check warning on line 63 in onnxruntime/core/providers/coreml/coreml_provider_factory.cc
GitHub Actions / Optional Lint C++
Check warning on line 82 in onnxruntime/core/providers/coreml/coreml_provider_factory.cc
GitHub Actions / Optional Lint C++