Skip to content

Change session option values if they don't work with EPs being registered for the session#4991

Merged
hariharans29 merged 3 commits intomasterfrom
cleanupInferenceSessionErrors
Sep 2, 2020
Merged

Change session option values if they don't work with EPs being registered for the session#4991
hariharans29 merged 3 commits intomasterfrom
cleanupInferenceSessionErrors

Conversation

@hariharans29
Copy link
Member

@hariharans29 hariharans29 commented Sep 1, 2020

Description:

  • Some session option values (default or user provided) may not work with some EPs. Rather than put the onus on the user to know these, make the appropriate choice for the session option while logging the change.
    • If the issue is with user provided options, it is fair to throw, but in cases where the default session option value (not user provided) does not work with some EPs, it doesn't seem fair to throw and have the user change it to the non-default working value. So picking the approach of changing the faulty session option value - default or user provided.

Motivation and Context
Takeaway from #4630

@hariharans29 hariharans29 requested a review from a team as a code owner September 1, 2020 02:30
@pranavsharma
Copy link
Contributor

The C API returns an error for this kind of scenario. https://github.com/microsoft/onnxruntime/blob/master/onnxruntime/core/session/onnxruntime_c_api.cc#L384 . As discussed, let's be consistent - either swallow the error or return an error.

@hariharans29
Copy link
Member Author

The C API returns an error for this kind of scenario. https://github.com/microsoft/onnxruntime/blob/master/onnxruntime/core/session/onnxruntime_c_api.cc#L384 . As discussed, let's be consistent - either swallow the error or return an error.

Removed the duplicate handling logic in the C API layer (was superfluous to begin with given that eventually RegisterExecutionProvider() did the exact same thing and there is a call to RegisterExecutionProvider()). With this all handling is done in a centralized place and keeps all APIs consistent.

@hariharans29 hariharans29 merged commit 4fd4b74 into master Sep 2, 2020
@hariharans29 hariharans29 deleted the cleanupInferenceSessionErrors branch September 2, 2020 22:13
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.

3 participants