Skip to content
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

[WIP]EP interface #18090

Draft
wants to merge 36 commits into
base: main
Choose a base branch
from
Draft

[WIP]EP interface #18090

wants to merge 36 commits into from

Conversation

RandySheriffH
Copy link
Contributor

WIP...

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lintrunner found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

import numpy

import onnxruntime
from onnxruntime.capi import _pybind_state as C

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'C' is not used.

import onnxruntime
from onnxruntime.capi import _pybind_state as C
from onnxruntime.capi.onnxruntime_pybind11_state import RunOptions

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'RunOptions' is not used.
import numpy

import onnxruntime
from onnxruntime.capi import _pybind_state as C

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'C' is not used.

import onnxruntime
from onnxruntime.capi import _pybind_state as C
from onnxruntime.capi.onnxruntime_pybind11_state import RunOptions

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'RunOptions' is not used.
import numpy

import onnxruntime
from onnxruntime.capi import _pybind_state as C

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'C' is not used.

import onnxruntime
from onnxruntime.capi import _pybind_state as C
from onnxruntime.capi.onnxruntime_pybind11_state import RunOptions

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'RunOptions' is not used.

/////////////////////////////////////////////////// Ep Interface ///////////////////////////////////////////////////

struct IExecutionProvider {
Copy link
Contributor

@jslhcl jslhcl Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is some dup code here, especially this class, which I have moved to include/onnxruntime/interface/provider/provider.h and renamed as ExecutionProvider #Resolved

using ArgPtrs = std::vector<ArgPtr>;

template <typename T>
struct ITensor : public IArg {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you going to move this class to interface/framework/Tensor.h?

Shall we consolidate this tensor interface with the one in interface/graph/graph.h ?

namespace onnxruntime {
class OpKernelContext;
}
#endif

#include "interface/framework/kernel.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to include kernel.h here? I don't see any class declared in kernel.h in the changes of this file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably shouldn't include interface/kernel here, as this file is for the internal kernel impl

@@ -9,6 +9,7 @@
#include "core/framework/op_node_proto_helper.h"
#include "core/graph/graph_viewer.h"
#include "core/common/gsl.h"
#include "interface/framework/kernel.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to include kernel.h here?


namespace interface {

enum TensorDataType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume I need to move DataType enum in interface/graph/graph.h here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use the same DataType definition for all the interface.

struct Celu {
Celu(const interface::IKernelInfo&) {}
onnxruntime::Status Compute(onnxruntime::interface::TensorView<float>& input,
onnxruntime::interface::Tensor<float>& output) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this Compute function doing this algorithm of Celu?

max(0,x) + min(0,alpha*(exp(x/alpha)-1))

My understanding is that it is executing max(0, identity(input)), and alpha is not used

struct Celu {
Celu(const interface::IKernelInfo&) {}
onnxruntime::Status Compute(onnxruntime::interface::TensorView<float>& input,
onnxruntime::interface::Tensor<float>& output) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use the interface (interface::ITensor) or the implementation (interface::Tensor) as the parameter?

using ArgPtr = std::unique_ptr<IArg>;
using ArgPtrs = std::vector<ArgPtr>;

struct ITensor : public IArg {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ITensor

Let internal tensor inherite from the ITensor.

openvino_ep::BackendManager::GetGlobalContext().onnx_model_path_name = std::string(onnx_path.begin(), onnx_path.end());
#else
openvino_ep::BackendManager::GetGlobalContext().onnx_model_path_name = graph_viewer.ModelPath().ToPathString();
openvino_ep::BackendManager::GetGlobalContext().onnx_model_path_name = graph_viewer->ModelPath();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onnx_model_path_name is not used so remove the ModelPath() API

#include <vector>
#ifdef INTREE_EP
#include "onnx/onnx_pb.h"
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we find a way to keep the To-Model-Proto onnx API internally under core?

/// <summary>
/// Enum of DataTypes using standard ONNX values. Casting to/from int32_t is encouraged.
/// </summary>
enum class DataType : int32_t {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enum class DataType : int32_t

we should use the same enume in interface/data_types.h

struct ModelProtoPtr {
const char* p;
size_t len;
size_t version;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size_t version;

what does this version means? onnx version? protobuf version? or ort version?

//#include "core/session/onnxruntime_c_api.h"
#include "core/framework/ortdevice.h"
#include "core/framework/stream_handles.h"
#include "core/framework/node_compute_info.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's carefully review the included headers. if those are the headers we are going to include (with header-only implementation), let's think about how to structure the include folders to represent the depenency.

#include <utility>
#include <vector>

#include "core/common/code_location.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code_location.h"

I think code location only used for dump call stack with windows debug build. do we need to expose it?

#pragma once

#include "core/common/common_no_ort_symbol.h"
#include "core/session/onnxruntime_c_api.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need CAPI?

// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

#pragma once
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file looks weird, it doesn't contain anything with Allocator, but named with allocator...

@@ -68,8 +69,8 @@ struct OpenVINOExecutionProviderInfo {
bool enable_dynamic_shapes)
: enable_vpu_fast_compile_(enable_vpu_fast_compile), device_id_(dev_id), num_of_threads_(num_of_threads), cache_dir_(cache_dir), num_streams_(num_streams), context_(context), enable_opencl_throttling_(enable_opencl_throttling), enable_dynamic_shapes_(enable_dynamic_shapes) {
if (dev_type == "") {
LOGS_DEFAULT(INFO) << "[OpenVINO-EP]"
<< "No runtime device selection option provided.";
//LOGS_DEFAULT(INFO) << "[OpenVINO-EP]"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOGS_DEFAULT(INFO)

we should add the logger interface

const void* GetExecutionHandle() const noexcept override {
return nullptr;
}
void RegisterKernels(interface::IKernelRegistry&) override {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void RegisterKernels(interface::IKernelRegistry&) override {}

why need override RegisterKernels?

@@ -8,6 +8,7 @@
#include "contexts.h"
#include <iomanip>
#include "ov_interface.h"
#include "onnx/onnx_pb.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onnx_pb

we include onnx_pb.h for what?

#else
ORT_UNUSED_PARAMETER(fused_node);
ORT_UNUSED_PARAMETER(subgraph);
return nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so for out of tree case, we just return nullptr? how could it work?

@@ -44,7 +43,7 @@ BasicBackend::BasicBackend(const ONNX_NAMESPACE::ModelProto& model_proto,
if (IsDebugEnabled()) {
std::string file_name = subgraph_context.subgraph_name + "_static.onnx";
std::fstream outfile(file_name, std::ios::out | std::ios::trunc | std::ios::binary);
model_proto.SerializeToOstream(outfile);
model_proto.SerializeToOstream(&outfile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outfile

why we need this change?

@jslhcl jslhcl force-pushed the rashuai/EpInterface branch from 01b3cee to 33a60f5 Compare December 29, 2023 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants