Conversation
| #ifdef __cplusplus | ||
| extern "C" { | ||
| #endif | ||
| OrtExecutionProviderFactory* RegisterCustomEp() { |
There was a problem hiding this comment.
return Status instead #Resolved
There was a problem hiding this comment.
Do we have to do this? This function will new a factory object by invoking its constructor which has no return type
… EP as graph API is not exported by ORT. Need to put these graph API into ortapi structure
…roviderAdapter::Compile()
| } OrtMetaDef; | ||
|
|
||
| typedef struct OrtIndexedSubGraph { | ||
| OrtMetaDef* meta_def; // TODO(leca): how to define a nested structure pointer? |
There was a problem hiding this comment.
Does this have to be a pointer to an OrtMetaDef? It may be simpler if this meta_def is contained by value instead. #Resolved
There was a problem hiding this comment.
It looks we will check the pointer is null or not to distinguish between single node mode and fused node mode (See base class IExecutionProvider::GetCapability() which does not set this pointer and TryAssignSingleNode() which will check this pointer)
|
|
||
| OutTreeEp::OutTreeEp(const char* ep_type, const OutTreeEpInfo& ep_info) : info(ep_info) { | ||
| type = ep_type; | ||
| OrtExecutionProvider::GetCapability = [](const OrtExecutionProvider* this_, const OrtGraphViewer* graph, size_t* cnt, OrtIndexedSubGraph*** indexed_sub_graph) { |
There was a problem hiding this comment.
If I'm understanding correctly, the type of the OrtIndexedSubGraph*** indexed_sub_graph parameter is essentially asking the EP to fill out an array of pointers to OrtIndexedSubGraph objects.
Would it be simpler to change this to OrtIndexedSubgraph** indexed_sub_graph so that the EP fills out an array of OrtIndexedSubGraph objects directly? Each OrtIndexedSubgraph struct is a simple POD that can be created on the stack and copied around. It seems like it would result in less pointer tracking. #Resolved
There was a problem hiding this comment.
Also, who is responsible for freeing this memory and when? If the EP allocates an array, then the EP should free it. The currently example leaks the allocations.
Edit: one possibility is to have onnxruntime call a new EP function (e.g., ReleaseOrtIndexedSubGraph()) so the the EP can free the memory. onnxruntime would call this once it is done using the indexed_sub_graph.
There was a problem hiding this comment.
The problem is that we don't know how many OrtIndexedSubGraph would be before we call GetCapability() function. I will fix the leak issue in the coming commits
| @@ -0,0 +1,627 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
Check warning
Code scanning / lintrunner
CLANGFORMAT/format
| @@ -0,0 +1,285 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
Check warning
Code scanning / lintrunner
CLANGFORMAT/format
| @@ -0,0 +1,266 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
Check warning
Code scanning / lintrunner
CLANGFORMAT/format
| @@ -0,0 +1,147 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
Check warning
Code scanning / lintrunner
CLANGFORMAT/format
| @@ -0,0 +1,557 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
Check warning
Code scanning / lintrunner
CLANGFORMAT/format
| @@ -0,0 +1,110 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
Check warning
Code scanning / lintrunner
CLANGFORMAT/format
| @@ -0,0 +1,54 @@ | |||
| #include "qnn_execution_provider.h" | |||
Check warning
Code scanning / lintrunner
CLANGFORMAT/format
| @@ -0,0 +1,33 @@ | |||
| #pragma once | |||
Check warning
Code scanning / lintrunner
CLANGFORMAT/format
| * | ||
| * \since Version 1.xx. | ||
| */ | ||
| ORT_API2_STATUS(SessionOptionsAppendPluginExecutionProvider, _In_ OrtSessionOptions* options, _In_ const char* ep_name, _In_ OrtEnv* env, |
There was a problem hiding this comment.
Can this can be removed? We may be able to use the existing C API function called SessionOptionsAppendExecutionProvider. The existing C API does not take an OrtEnv parameter, but we can just get the default OrtEnv since there is only one per process.
|
General comment, please, re-format so the lines do not exceed 120 chars limit. |
| return InlinedVector<const Node*>(); | ||
| } | ||
|
|
||
| bool IsBuiltInEp() const { return builtin_ep_; } |
| #include <string> | ||
| #include <set> | ||
|
|
||
| struct OrtTypeConstraints { |
| #include "core/common/logging/logging.h" | ||
| #include "core/framework/allocator.h" | ||
| #include "core/session/onnxruntime_c_api_ep.h" | ||
|
|
There was a problem hiding this comment.
Can we forward declare the types and avoid public header inclusion in the header?
|
|
||
| #include <atomic> | ||
| #include <memory> | ||
| #include <unordered_set> |
| * | ||
| * \param[in] kernel_registry Opaque pointer of KernelRegistry object | ||
| * \param[in] custom_op Custom Op where the kernel compute function is defined | ||
| * \param[in] type_constraints |
| OrtExecutionProvider*(ORT_API_CALL* CreateExecutionProvider)(OrtExecutionProviderFactory* this_, const char* const* ep_option_keys, const char* const* ep_option_values, size_t option_size); | ||
| } OrtExecutionProviderFactory; | ||
|
|
||
| struct OrtGraphApi { |
There was a problem hiding this comment.
This is really a read-only graph viewer.
Suggest to name it appropriatly
Perhaps, the name should reflect the fact that this API is specifically for EP interaction.
| ONNXTensorElementDataType data_type; | ||
| const char* data; | ||
| size_t data_len; | ||
| } OrtTensorRef; |
There was a problem hiding this comment.
This seems to be a repeat of the existing API such as TensorTypeAndShape. Can we re-use that part?
| size_t num_external_initializer_files); | ||
| }; | ||
|
|
||
| /** \brief Create OrtDevice object. |
There was a problem hiding this comment.
Suggestion is to create a table that is separate from
the main API. Reasons:
Most of the clients do not need that code.
But it does affect language bindings.
For example, we now need to pad C# imported API structure, although it is unlikely we would ever need that in the C#, but if we do we can add that separate.
| ORT_API2_STATUS(OrtGraph_IsConstantInitializer, const OrtGraphViewer* graph, const char* name, bool check_outer_scope, _Out_ bool* out); | ||
|
|
||
| /** \brief Get the NodeIndex values of the graph nodes sorted in topological order | ||
| * |
There was a problem hiding this comment.
It looks like the output ptr data is const, please, specify that it is not to be freed by the client.
| size_t node_index_len; | ||
| } OrtIndexedSubGraph; | ||
|
|
||
| typedef struct OrtComputeContext { |
There was a problem hiding this comment.
Suggest adding a constructor for _cplusplus
| /** \brief Gets the path of the owning model if any | ||
| * | ||
| * \param[in] graph The graph to query | ||
| * \param[out] model_path The path of the owning model if any |
| * \param[out] out True if the graph is a subgraph | ||
| * | ||
| */ | ||
| ORT_API2_STATUS(OrtGraph_IsSubgraph, const OrtGraphViewer* graph, _Out_ bool* out); |
| * \remarks The caller is responsible for freeing the byte array using OrtFreeMem. | ||
| * | ||
| */ | ||
| ORT_API2_STATUS(OrtGraph_SerializeToArray, const OrtGraphViewer* graph, _Out_ void** data, _Out_ size_t* data_size); // TODO(leca): review and discuss |
| * \param[in] onnx_model_path The file path to save to | ||
| * | ||
| */ | ||
| ORT_API2_STATUS(OrtGraph_DumpOnnxModel, const OrtGraphViewer* graph, const char* onnx_model_path); |
| auto& eps = GetExecutionProviders(); | ||
| for (auto& ep : eps) { | ||
| ep->RegisterStreamHandlers(GetStreamHandleRegistryInstance(), *allocators_); | ||
| std::string register_resource_after = ""; |
| std::string register_resource_after = ""; | ||
| IExecutionProvider* plugin_ep = nullptr; | ||
| for (auto& ep : execution_providers_) { | ||
| if (register_resource_after == "") { |
| std::unordered_map<std::string, std::set<ONNXTensorElementDataType>>::iterator iter = type_constraints_.find(type_symbol); | ||
| if (iter == type_constraints_.end()) { | ||
| std::set<ONNXTensorElementDataType> types{type}; | ||
| type_constraints_[type_symbol] = types; |
- Modify CMakeLists.txt for TRT EP plugin - Add "-l" for specifying EP plugin lib path for onnxruntime_perf_test
adrianlizarraga
left a comment
There was a problem hiding this comment.
Some memory/lifetime comments for OrtIndexedSubGraph in GetCapability()
| } OrtMetaDef; | ||
|
|
||
| typedef struct OrtIndexedSubGraph { | ||
| OrtMetaDef* meta_def; // TODO(leca): how to define a nested structure pointer? |
There was a problem hiding this comment.
nit: I don't think it's necessary for this to be a separate memory allocation. I see that it was based on our internal IndexedSubGraph implementation, but in my view reducing the number of memory allocation makes things simpler. Perhaps meta_def can be stored inline and can add a boolean to indicate if the meta_def is valid.
typedef struct OrtMetaDef {
bool is_valid;
// ...
} OrtMetaDef;
typedef struct OrtIndexedSubGraph {
OrtMetaDef meta_def;
// ...
} OrtIndexedSubGraph;| virtual std::vector<std::unique_ptr<ComputeCapability>> GetCapability(const GraphViewer& graph_viewer, const IKernelLookup& kernel_lookup) const override { | ||
| size_t cnt = 0; | ||
| OrtIndexedSubGraph** indexed_subgraph = nullptr; | ||
| if (ep_impl_->GetCapability) ep_impl_->GetCapability(ep_impl_, reinterpret_cast<const OrtGraphViewer*>(&graph_viewer), &cnt, &indexed_subgraph); |
There was a problem hiding this comment.
This is currently passing an OrtIndexedSubGraph*** to the EP's GetCapability() function and asking the EP to allocate an array of pointers to OrtIndexSubGraph objects. Because the EP is allocating the memory, we currently have a separate API to allow the EP to delete this memory.
I wonder if it would be simpler to allow ORT to pass in an OrtAllocator to the EP. The EP would use this ORT-owned allocator to allocate memory for the array. This woud remove the need for a separate C API to clean up the memory. Also, it would allow the parameter to be a OrtIndexedSubGraph** instead of OrtIndexedSubGraph***.
### Description This PRs sets the foundation for the EP ABI, which allows plugin-EPs to interface with ORT using a binary stable interface. A plugin-EP can be built separately from ORT and is not tied to a specific commit of ORT. Currently, this PR adds basic APIs necessary to allow an example plugin-EP to compile and run a simple model with a single `Mul` node. - Example plugin-EP implementation: https://github.com/microsoft/onnxruntime/blob/adrianl/ep-abi/onnxruntime/test/autoep/library/example_plugin_ep.cc - APIs: - Graph IR: https://github.com/microsoft/onnxruntime/blob/adrianl/ep-abi/include/onnxruntime/core/session/onnxruntime_c_api.h#L5290-L5439 - Plugin EP: https://github.com/microsoft/onnxruntime/blob/adrianl/ep-abi/include/onnxruntime/core/session/onnxruntime_c_api.h#L6177-L6481 - Example app code (from unit tests): https://github.com/microsoft/onnxruntime/blob/adrianl/ep-abi/onnxruntime/test/autoep/test_autoep_selection.cc#L614 ### Motivation and Context Based on #21450 --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Edward Chen <18449977+edgchen1@users.noreply.github.com>
|
This pull request has been automatically closed because it has merge conflicts and has been inactive for more than 30 days. Please rebase on the target branch and open a new PR. |
### Description This PRs sets the foundation for the EP ABI, which allows plugin-EPs to interface with ORT using a binary stable interface. A plugin-EP can be built separately from ORT and is not tied to a specific commit of ORT. Currently, this PR adds basic APIs necessary to allow an example plugin-EP to compile and run a simple model with a single `Mul` node. - Example plugin-EP implementation: https://github.com/microsoft/onnxruntime/blob/adrianl/ep-abi/onnxruntime/test/autoep/library/example_plugin_ep.cc - APIs: - Graph IR: https://github.com/microsoft/onnxruntime/blob/adrianl/ep-abi/include/onnxruntime/core/session/onnxruntime_c_api.h#L5290-L5439 - Plugin EP: https://github.com/microsoft/onnxruntime/blob/adrianl/ep-abi/include/onnxruntime/core/session/onnxruntime_c_api.h#L6177-L6481 - Example app code (from unit tests): https://github.com/microsoft/onnxruntime/blob/adrianl/ep-abi/onnxruntime/test/autoep/test_autoep_selection.cc#L614 ### Motivation and Context Based on microsoft#21450 --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Edward Chen <18449977+edgchen1@users.noreply.github.com>
Description
Out-Tree EP feature.
Motivation and Context