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

RunAsync C/CXX API #16613

Merged
merged 30 commits into from
Jul 16, 2023
Merged

RunAsync C/CXX API #16613

merged 30 commits into from
Jul 16, 2023

Conversation

RandySheriffH
Copy link
Contributor

@RandySheriffH RandySheriffH commented Jul 6, 2023

Implement RunAsync API - the session will run in a thread of intra-op thread pool.

Copy link
Contributor

@pranavsharma pranavsharma left a comment

Choose a reason for hiding this comment

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

How will users turn on this feature? By configuring num inter op threads? Should we've a session option to turn on async runs? Not all sessions require it and for those that don't creating an inter-op threadpool is waste of resources.
Reminder to add documentation.

include/onnxruntime/core/session/onnxruntime_c_api.h Outdated Show resolved Hide resolved
onnxruntime/core/session/inference_session.cc Outdated Show resolved Hide resolved
onnxruntime/core/session/inference_session.cc Outdated Show resolved Hide resolved
@RandySheriffH RandySheriffH changed the title RunAsync C API RunAsync C/CXX API Jul 6, 2023
@RandySheriffH
Copy link
Contributor Author

RandySheriffH commented Jul 6, 2023

How will users turn on this feature? By configuring num inter op threads? Should we've a session option to turn on async runs? Not all sessions require it and for those that don't creating an inter-op threadpool is waste of resources. Reminder to add documentation.

Let's stick with intra op thread pool to avoid all those.

@RandySheriffH RandySheriffH marked this pull request as ready for review July 7, 2023 17:23
@pranavsharma
Copy link
Contributor

Also, a reminder that the documentation should call out that the callback is going to be executed in one of the intra op threadpool threads.

@yuslepukhin
Copy link
Member

Is there a design document for this? In my book running async requires a huge number of changes.
Also what is the purpose of running async? There is no IO, we are CPU bound. What would a callback do? Set a condition var? What is the difference between sync call return and wait on cond var?
I fail to understand what the async mean.

@RandySheriffH
Copy link
Contributor Author

Also, a reminder that the documentation should call out that the callback is going to be executed in one of the intra op threadpool threads.

Would this be good enough?

/** \brief Run the model asynchronously in a thread owned by intra op thread pool

@fs-eire
Copy link
Contributor

fs-eire commented Jul 11, 2023

Is there a design document for this? In my book running async requires a huge number of changes. Also what is the purpose of running async? There is no IO, we are CPU bound. What would a callback do? Set a condition var? What is the difference between sync call return and wait on cond var? I fail to understand what the async mean.

I have to agree with you... My understanding is that, this PR is just a change to introduce the async API, which does not have a real "async" implementation. The major value of this PR is for reviewing the interface. The implementation does not work in ort-web anyway.

@RandySheriffH
Copy link
Contributor Author

RandySheriffH commented Jul 11, 2023

Is there a design document for this? In my book running async requires a huge number of changes. Also what is the purpose of running async? There is no IO, we are CPU bound. What would a callback do? Set a condition var? What is the difference between sync call return and wait on cond var? I fail to understand what the async mean.

I have to agree with you... My understanding is that, this PR is just a change to introduce the async API, which does not have a real "async" implementation. The major value of this PR is for reviewing the interface. The implementation does not work in ort-web anyway.

No, it does not target ort-web, at least not for now. The target of the PR is to make RunAsync work for desktop and mobile for 1.16, specifically, run session in separate thread and invoke callback. For web cases, the API might migrate with a different behavior in the future.

pranavsharma
pranavsharma previously approved these changes Jul 12, 2023
include/onnxruntime/core/session/onnxruntime_cxx_api.h Outdated Show resolved Hide resolved
onnxruntime/core/session/inference_session.h Outdated Show resolved Hide resolved
onnxruntime/core/session/inference_session.cc Show resolved Hide resolved
onnxruntime/core/session/inference_session.cc Outdated Show resolved Hide resolved
onnxruntime/core/session/inference_session.cc Show resolved Hide resolved
onnxruntime/core/session/inference_session.cc Outdated Show resolved Hide resolved
onnxruntime/core/session/onnxruntime_c_api.cc Outdated Show resolved Hide resolved
onnxruntime/core/session/onnxruntime_c_api.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@pranavsharma pranavsharma left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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.

6 participants