Skip to content

python api's for execution provider registration - #1826

Merged
jywu-mysoft merged 5 commits into
masterfrom
jywu_pybind_provider_reg
Sep 13, 2019
Merged

python api's for execution provider registration#1826
jywu-mysoft merged 5 commits into
masterfrom
jywu_pybind_provider_reg

Conversation

@jywu-mysoft

Copy link
Copy Markdown
Contributor

Add python api's for execution provider registration.
default to the existing static registration priority order. the new api's allow user to override the order.
-global api's
get_all_providers() : returns list of all known ORT execution provider type strings.
get_available_providers(): returns list of available ORT execution provider type strings. (ones that have been enabled in the build/package) This list is ordered by the default execution provider priority.
-session api's
get_providers(): returns list of registered ORT execution provider type strings. the providers are ordered from highest priority to lowest.
set_providers(): register the list of provider names. the list is ordered from highest priority to lowest.
e.g.
sess.set_providers(['TensorrtExecutionProvider', 'CUDAExecutionProvider', 'CPUExecutionProvider'])
means TensorRT has highest priority, followed by CUDA followed by CPU.

sess.set_providers([]) # empty list means register using default priority order.

The underlying session is reset and recreated when the set_providers() api is used.

These api's set up the ability to implement a CPU provider fallback retry when there is an underlying Execution Provider runtime failure. (TBD in upcoming PR)

-added some unit tests.
tested that no more than one session object is kept in memory at one time to minimize peak memory usage, when using the registration api's.
-documentation for these api's will need to be added to appropriate ORT python api MS docs location.

Comment thread onnxruntime/python/session.py Outdated
onnxruntime::common::Status status;
// ordered by default priority. highest to lowest.
const std::vector<std::string>& GetAllProviders() {
static std::vector<std::string> all_providers = {kTensorrtExecutionProvider, kCudaExecutionProvider, kMklDnnExecutionProvider,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

all_providers [](start = 34, length = 13)

This creates one more place with a list of providers that one needs to edit when adding one. Can we make some consolidation? Perhaps, put inside execution _providers.cc?

@jywu-mysoft jywu-mysoft Sep 13, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sounds like a good thing to do but let's do it in a different PR.
the reason is, one would have to modify onnxruntime_pybind_state.cc anyway to support a new provider in python.
also, currently the string constants are all defined in core/graph/constants.h which is wrong. (the provider constants have nothing to do with graph)
to clean that up will require a bunch of files to change. I think we should define the constants and add GetAllProviders() to execution_providers.h

Comment thread onnxruntime/python/onnxruntime_pybind_state.cc Outdated
@yuslepukhin

yuslepukhin commented Sep 12, 2019

Copy link
Copy Markdown
Contributor

using namespace std;

nit: Perhaps, we could try and remove this line. There are only a few places.


Refers to: onnxruntime/python/onnxruntime_pybind_state.cc:115 in 03f4797. [](commit_id = 03f4797, deletion_comment = False)

@jywu-mysoft

Copy link
Copy Markdown
Contributor Author

using namespace std;

Perhaps, we could try and remove this line. There are only a few places.

Refers to: onnxruntime/python/onnxruntime_pybind_state.cc:115 in 03f4797. [](commit_id = 03f4797, deletion_comment = False)

yeah, we shouldn't have using namespace std;
if there's really few places to change, i'll do it. otherwise it's out of the scope of this PR.

"Return list of registered execution providers."
return self._providers

def set_providers(self, providers):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we call this register_providers since that's what it is essentially doing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

possibly. i'm open to changing it. but i thought get/set made more sense here. (because of the get api which exists because of the default registration)

Comment thread onnxruntime/python/session.py
Comment thread onnxruntime/core/framework/execution_providers.h
Comment thread onnxruntime/python/onnxruntime_pybind_state.cc
@pranavsharma

Copy link
Copy Markdown
Contributor

@jywu-msft mentioned that he'll fix some of the comments in a follow up PR.

onnxruntime::CreateExecutionProviderFactory_OpenVINO("CPU"),
#endif
#ifdef USE_TENSORRT
onnxruntime::CreateExecutionProviderFactory_Tensorrt()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @jywu-msft , this line of code won't work. Because the CreateExecutionProviderFactory_Tensorrt function need an argument.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I didn't notice it because in my PR it was just an automatic clang format change.
The issue was actually introduced in #1678
I guess it's not common to hit it because you would need to build with python enabled, TensorRT enabled and onnxruntime_PYBIND_EXPORT_OPSCHEMA

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