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

When developing an ONNX Runtime execution provider, how to get the current InferenceSession instance which the execution provider registers with? #18046

Open
microwish opened this issue Oct 20, 2023 · 10 comments
Assignees
Labels
core runtime issues related to core runtime platform:windows issues related to the Windows platform

Comments

@microwish
Copy link

microwish commented Oct 20, 2023

Describe the issue

When developing an ONNX Runtime execution provider, how to get the current InferenceSession instance which the execution provider registers with?

Seems like there is no approach to that so far?

Can we make a few changes to the class InferenceSession of ONNXRuntime core?

  • Opiton 1
    1. In the base class IExecutionProvider, add a new protected memeber variable std::shared_ptr<InferenceSession> curr_sess_ and related public getter method and setter method. Then every child class of IExecutionProvider will inherit this attribute and related getter method and setter method.
    2. In the method InferenceSession::RegisterExecutionProvider(), assign the this pointer to curr_sess_ of an instance of a child class of IExecutionProvider using the corresponding setter method of the child class.
  • Option 2
    1. In a child class MyCostumExecutionProvider, add a new private memeber variable std::shared_ptr<InferenceSession> curr_sess_ and related public getter method and setter method.
    2. In the method InferenceSession::RegisterExecutionProvider() OR InferenceSession::Initialize(), specially handle MyCustomExecutionProvider to assign the this pointer to MyCustomExecutionProvider::curr_sess_ using the corresponding setter method of MyCustomExecutionProvider. An example of specially handling a specific execution provider in InferenceSession class is DmlExecutionProvider.

To reproduce

N/A.

Urgency

No response

Platform

All

OS Version

All

ONNX Runtime Installation

Other / Unknown

ONNX Runtime Version or Commit ID

All

ONNX Runtime API

C++

Architecture

X64

Execution Provider

Other / Unknown

Execution Provider Library Version

No response

@github-actions github-actions bot added the platform:windows issues related to the Windows platform label Oct 20, 2023
@pranavsharma
Copy link
Contributor

What's your use case? Why do you need a reference to the session in the EP?

@microwish
Copy link
Author

Hi @pranavsharma ,

We already have a couple of EPs in ONNX Runtime codebase, now we are planning to develop a "unified" EP to cover all the existing separate EPs of ours.

In order to reuse the implementation of those existing EPs as much as possible, in the "unified" EP, we need to access (in a read-only manner) those EPs.
Seems like the only way for that is InferenceSession --> SessionState::GetExecutionProviders().

So, I was thinking to add a reference to InferenceSessioin in our new "unified" EP.

@microwish
Copy link
Author

Hi @pranavsharma , can you or your team members please share whatever suggestions and insights? Thanks

@pranavsharma
Copy link
Contributor

pranavsharma commented Oct 24, 2023

You can include the headers of the relevant EPs in your unified EP and construct them when constructing the unified EP. You'll have one cmake file that builds all the EPs as part of the unified EP which will abstract all the other EPs. The other EPs can still co-exist independently. Will this work?
Thing is even if you were to get access to the session, there's no guarantee all the EPs that you're trying to unify will be registered to the session.
Which EPs are we talking about here?
If this doesn't work, maybe it's better to setup a meeting to go over the context/details.

@microwish
Copy link
Author

microwish commented Oct 24, 2023

Hi @pranavsharma , thanks for the detailed response.
We did consider this way. Technically and functionally, this should be working. Some details below:

  1. In the unified EP class MyUnifiedExecutionProvider, add private member variables of type std::unique_ptr<IExecutionProvider> pointing at the covered downstream EPs and related public getter/setter methods.
  2. In the destructor of MyUnifiedExecutionProvider, reset the pointers pointing at the covered downstream EPs (this is not explicitly required).
  3. In the method MyUnifiedProviderFactory::CreateProvider(), assign values to those std::unique_ptr<IExecutionProvider> member variables.
  • IExecutionProviderFactory::CreateProvider()s are called in InitializeSession() (onnxruntime/core/session/onnxruntime_c_api.cc).
  • If downstream EPs were not decommissioned, multitple pointers (created by MyUnifiedProviderFactory and ONNXRT session, respectively) to them would be co-existing.
  • So, some minor concerns about memory management and possible cache reuse.

Meanwhile, we are aware that adding a reference to InferenceSession in EPs might lead to potential memory leaks and cyclic dependencies, so we do need to be very careful.

Moreover, it would be great if we could set up a meeting about such unified EP implementation.
The interaction between EP and ONNXRT is mature and we kind of have a routine to follow for implementation.
However, seems like the interaction between different EPs hasn't been touched yet.

@pranavsharma
Copy link
Contributor

Different EPs are not supposed to be interacting with each other. Multiple pointers to the same EP would co-exist only if the user has registered both the unified EP and one of the other EPs in the unified EP. Depending on the EP, this may or may not be a problem. We need more details. Before setting up a meeting I would like to know what organization you're representing, what EPs you're trying to unify and most importantly the motivation and context behind this work.
cc @faxu

@pranavsharma pranavsharma added the core runtime issues related to core runtime label Oct 24, 2023
@pranavsharma pranavsharma self-assigned this Oct 24, 2023
@microwish
Copy link
Author

microwish commented Oct 24, 2023

Hi @pranavsharma @faxu ,

I'm from AMD. I was intentionally hiding the info of my organization and using my personal account.
The EPs we are trying to cover include Vitis AI EP, MIGraphX EP, etc using a unified EP.

@microwish
Copy link
Author

Hi @pranavsharma @faxu ,

Any more information you'd like to know before we can schedule a meeting? Thanks

@microwish
Copy link
Author

Please NOTE - this is still NOT an official AMD comment.

@pranavsharma :

To address some of your early concerns:

  1. As @faxu mentioned, Peng is the current PoC. It might be due to some historical reasons. In fact, our team has been working more on ONNXRT EPs and we are currently working on the unified EP which is part of company wide strategies (e.g., https://www.eetimes.com/rocm-is-amds-no-1-priority-exec-says/).
  2. Unfortunately, as we are swamped with prioritized projects at hand, we haven't gotten a chance to talk to Peng yet. But it wil happen some time later.

Regarding our current proposal (and its further evolution) to ONNXRT core:

Briefly and at hight level, apart from some technical benefits (with careful implementation), we may be able to (or even we should) open up (perhaps with some constraints) the intaraction between different EPs. This way, ONNXRT EPs would form an ecosystem which would be like (not technically) the ecosystem of dialects of MLIR.
We already have use cases for this, even though we can implement those use cases in alternative ways.

@pranavsharma
Copy link
Contributor

I don't think we want different EPs to be interacting with each other during graph partitioning and/or execution. Unification of EPs from the same hardware/device is really a code organization/reuse issue than opening up EP interactions. Let's discuss when we're ready to prioritize this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core runtime issues related to core runtime platform:windows issues related to the Windows platform
Projects
None yet
Development

No branches or pull requests

2 participants