Add a generic collection of session configurations to the SessionOptions #4718

guoyu-wang · 2020-08-06T04:57:24Z

Description:
Add generic configurations for session options

Motivation and Context
Right now in order to change some setting within an inference sessions, we will need to either

Use ORTAPI, add entry if necessary, (will bloat the c/cxx_api)
Use environmental variables, (not really work for some environments, such as mobile)
Hardcode the settings (not really applicable to end users )

This proposal is to add a single function OrtApis::SetSessionConfiguration to add different configurations for the inference session

Exposing new options will not require updating the ORTAPI any more
Use a std::unordered_map as the collection of the configurations
Define an enum as the config key, and a std::string as the config value, the consumer of the config will be responsible to parse the string
EP will still using constructor for EP specific configurations

guoyu-wang · 2020-08-06T05:37:59Z

include/onnxruntime/core/session/onnxruntime_c_api.h


+typedef enum OrtSessionConfigKey {
+  ORT_SESSION_CONFIG_MAXIMUM,  // Always leave this as the last value
+} OrtSessionConfigKey;


Since I don't have any EP setting to add here anymore, so I will leave this as a stub
Maybe we want to add a header file for this enum with clear naming convention and string format for each config
#WontFix

Should we call this OrtSessionOptionKey? Adding a new vocab might confuse folks. #Resolved

Could go either way on this.
We have SessionOptions and we're adding essentially a collection of key:value pairs to that (so a collection within a collection).
We somewhat hide the session options in parts of the API (e.g. it's SetSessionLogId not SetSessionOptionX although the parameter is a SessionOptions instance) but make it obvious in others (CloneSessionOptions).
Due to that, OrtSessionOptionKey could potentially be confused as something to set any 'key' in SessionOptions.

Possibly the clearest is OrtSessionOptionsConfigKey.

In reply to: 466683249 [](ancestors = 466683249)

Went with OrtSessionOptionsConfigKey

In reply to: 466725770 [](ancestors = 466725770,466683249)

onnxruntime/core/session/abi_session_options.cc

pranavsharma · 2020-08-06T20:59:50Z

include/onnxruntime/core/session/onnxruntime_c_api.h


+typedef enum OrtSessionConfigKey {
+  ORT_SESSION_CONFIG_MAXIMUM,  // Always leave this as the last value
+} OrtSessionConfigKey;


Should we call this OrtSessionOptionKey? Adding a new vocab might confuse folks. #Resolved

pranavsharma · 2020-08-06T21:22:57Z

include/onnxruntime/core/session/onnxruntime_c_api.h

+     * \param config_key An enum as the key of the configuration
+     * \param config_value A null terminated string representation of the config value
+     */
+  ORT_API2_STATUS(SetSessionConfiguration, _Inout_ OrtSessionOptions* options,


Should we call this AddSessionOption()? With the enum change from OrtSessionConfigKey -> OrtSessionOptionKey, this would be clearer. And config_value can be option_value. #Resolved

pranavsharma · 2020-08-06T21:24:03Z

include/onnxruntime/core/session/onnxruntime_c_api.h

+     * \param config_value A null terminated string representation of the config value
+     */
+  ORT_API2_STATUS(SetSessionConfiguration, _Inout_ OrtSessionOptions* options,
+                  _In_ OrtSessionConfigKey config_key, _In_ const char* config_value);


Should we allow users to set existing session options as well using this method? #WontFix

We'd have to implement logic to handle converting the string to the correct type including error checking for all options. I think it would also be confusing to have two different ways to set one value in SessionOptions in the API.

In reply to: 466694258 [](ancestors = 466694258)

I wanted to convert logid (which is a good candidate to use the map<enum, string>) into the the map, but it is used in too many places

In reply to: 466726344 [](ancestors = 466726344,466694258)

onnxruntime/core/framework/session_options.h

include/onnxruntime/core/session/onnxruntime_c_api.h

include/onnxruntime/core/session/onnxruntime_cxx_api.h

skottmckay · 2020-08-06T23:11:54Z

C# SessionOptions class needs update as does python API #Resolved

guoyu-wang · 2020-08-06T23:21:36Z

This will be updated once the c/cxx api is settled

In reply to: 670235316 [](ancestors = 670235316)

include/onnxruntime/core/session/onnxruntime_c_api.h

…void build break

guoyu-wang · 2020-08-12T06:15:23Z

include/onnxruntime/core/graph/constants.h

+
+// Key for disable MatMul PrePacking,
+// if the config value is set to "1" then the prepacking is disabled, otherwise prepacking is enabled
+constexpr const char* kDisablePrePacking = "Math.MatMul.DisablePrePacking";


This is probably not the best place for these keys, any suggestion?
#Resolved

I'd be tempted to have them under include/onnxruntime/core/common.

We should have some naming guidelines. I'd prefer an 'area' first - like 'op' for operators. Having Math and MatMul seems unnecessary (also I didn't think pre-packing would be limited to a specific operator, and that we would enable it more globally as that's what happens in SessionState currently).

In reply to: 469027840 [](ancestors = 469027840)

guoyu-wang · 2020-08-12T20:20:08Z

This part I have no idea, there are multiple checkins adding new apis in the past week

In reply to: 672825623 [](ancestors = 672825623)

Refers to: onnxruntime/core/session/onnxruntime_c_api.cc:1826 in 96d36fa. [](commit_id = 96d36fa, deletion_comment = False)

guoyu-wang · 2020-08-13T00:07:01Z

include/onnxruntime/core/common/common.h

 #endif

+#pragma region Session_Options_Configs
+


Keep these config definitions here for now.
When we have more configs defined, we can move them into a separated head file
#Closed

nit: Would prefer a separate header file now. We can put the strings in an appropriate namespace like onnxruntime::config_keys, and it won't break user's builds later when we move it. #Resolved

onnxruntime/core/framework/session_options.h

onnxruntime/core/framework/finalize_session_state.cc

onnxruntime/core/framework/session_options.cc

onnxruntime/python/onnxruntime_pybind_state.cc

onnxruntime/test/python/onnxruntime_test_python.py

pranavsharma · 2020-08-17T19:37:19Z

include/onnxruntime/core/common/common.h

 inline std::string ToWideString(const std::string& s) { return s; }
 #endif

+// Naming Convention for a Session Options Config Key


How will the C API users discover these keys? This is a C++ file and won't be shipped as part of our packages. I suggest either (1) creating a separate file called session_options_config_keys.h and including it from the top level c_api.h or (2) adding the keys directly in c_api.h. I prefer 1 as it's a bit cleaner. #Resolved

guoyu-wang · 2020-08-17T22:52:24Z

include/onnxruntime/core/session/session_options_config_keys.h

+
+// Key for disable PrePacking,
+// If the config value is set to "1" then the prepacking is disabled, otherwise prepacking is enabled (default value)
+#define ORT_SESSION_OPTIONS_CONFIG_DISABLEPREPACKING "session_state.disable_prepacking"


Since this is pure c api, changed to use macro instead of const char*

Didn't follow this. What was wrong with const char*?

skottmckay

adding generic configurations for session options

b5426eb

guoyu-wang requested a review from a team as a code owner August 6, 2020 04:57

fix a build break on linux

84b368b

guoyu-wang commented Aug 6, 2020

View reviewed changes

onnxruntime/core/session/abi_session_options.cc Outdated Show resolved Hide resolved

guoyu-wang added 2 commits August 5, 2020 23:48

fix training ci build break

d3bf639

fix training ci build break

ed8bc9b

pranavsharma reviewed Aug 6, 2020

View reviewed changes

skottmckay reviewed Aug 6, 2020

View reviewed changes

include/onnxruntime/core/session/onnxruntime_c_api.h Outdated Show resolved Hide resolved

skottmckay reviewed Aug 6, 2020

View reviewed changes

include/onnxruntime/core/session/onnxruntime_cxx_api.h Show resolved Hide resolved

pranavsharma mentioned this pull request Aug 6, 2020

Add the framework to support prepack #4413

Merged

guoyu-wang added 4 commits August 6, 2020 17:22

addressed CR comments

1ed413a

merge master

7783efb

fix traning ci build break

7242476

move config_key from enum to string

2f6dd5d

guoyu-wang marked this pull request as draft August 11, 2020 01:45

guoyu-wang added 3 commits August 10, 2020 18:59

add c# api

f0e41e4

merge master

5b6cb5d

add python api

5ea91ca

guoyu-wang changed the title ~~[Proposal] Add a generic collection of session configurations to the SessionOptions~~ [WIP] Add a generic collection of session configurations to the SessionOptions Aug 11, 2020

guoyu-wang marked this pull request as ready for review August 11, 2020 05:48

guoyu-wang added 3 commits August 10, 2020 23:34

fix build break

19f118f

move prepacking from 2 new api entries to session options configs

828c7e9

fix traning ci build break

ba14831

guoyu-wang requested a review from yufenglee August 11, 2020 21:17

guoyu-wang commented Aug 11, 2020

View reviewed changes

include/onnxruntime/core/session/onnxruntime_c_api.h Outdated Show resolved Hide resolved

add python test, update some comments, move const key definition to a…

96d36fa

…void build break

guoyu-wang commented Aug 12, 2020

View reviewed changes

guoyu-wang added 2 commits August 12, 2020 14:12

addressed comments

3f4e6c3

move definitions of keys to common.h

81169e2

guoyu-wang commented Aug 13, 2020

View reviewed changes

guoyu-wang added 4 commits August 14, 2020 17:35

Merge branch 'master' into onnx_session_configs_1

91cabaa

move api to version 5

229c7cd

remove accidental change in build.py

e219a35

remove pragma to avoid build break

be72f1e

guoyu-wang changed the title ~~[WIP] Add a generic collection of session configurations to the SessionOptions~~ Add a generic collection of session configurations to the SessionOptions Aug 15, 2020

skottmckay reviewed Aug 17, 2020

View reviewed changes

onnxruntime/core/framework/session_options.h Show resolved Hide resolved

skottmckay reviewed Aug 17, 2020

View reviewed changes

onnxruntime/core/framework/finalize_session_state.cc Outdated Show resolved Hide resolved

skottmckay reviewed Aug 17, 2020

View reviewed changes

onnxruntime/core/framework/session_options.cc Outdated Show resolved Hide resolved

skottmckay reviewed Aug 17, 2020

View reviewed changes

onnxruntime/python/onnxruntime_pybind_state.cc Show resolved Hide resolved

skottmckay reviewed Aug 17, 2020

View reviewed changes

onnxruntime/test/python/onnxruntime_test_python.py Show resolved Hide resolved

pranavsharma reviewed Aug 17, 2020

View reviewed changes

guoyu-wang added 2 commits August 17, 2020 14:44

addressed CR comments

2e50add

fix the python build break, and move location of config keys definition

737248b

guoyu-wang commented Aug 17, 2020

View reviewed changes

skottmckay previously approved these changes Aug 18, 2020

View reviewed changes

small typo changes

da3585a

guoyu-wang dismissed skottmckay’s stale review via da3585a August 18, 2020 17:44

yufenglee approved these changes Aug 18, 2020

View reviewed changes

guoyu-wang merged commit dee7596 into master Aug 18, 2020

guoyu-wang deleted the onnx_session_configs_1 branch August 18, 2020 20:40

guoyu-wang mentioned this pull request Aug 19, 2020

Nuget build break fix #4854

Merged

guoyu-wang restored the onnx_session_configs_1 branch August 20, 2020 05:16

guoyu-wang deleted the onnx_session_configs_1 branch September 10, 2020 09:48

jywu-msft mentioned this pull request Sep 11, 2020

[OpenVINO-EP] Enable EP config options for VPU hardware #5119

Merged

This was referenced May 14, 2022

[Snyk] Security upgrade mocha from 8.3.2 to 9.1.2 ekmixon/onnxruntime#75

Open

[Snyk] Fix for 1 vulnerabilities ekmixon/onnxruntime#76

Open

Add a generic collection of session configurations to the SessionOptions #4718

Add a generic collection of session configurations to the SessionOptions #4718

Uh oh!

Conversation

guoyu-wang commented Aug 6, 2020

Uh oh!

guoyu-wang Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavsharma Aug 6, 2020 • edited by guoyu-wang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skottmckay Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guoyu-wang Aug 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pranavsharma Aug 6, 2020 • edited by guoyu-wang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavsharma Aug 6, 2020 • edited by guoyu-wang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavsharma Aug 6, 2020 • edited by guoyu-wang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skottmckay Aug 6, 2020

Choose a reason for hiding this comment

Uh oh!

guoyu-wang Aug 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skottmckay commented Aug 6, 2020 • edited by guoyu-wang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guoyu-wang commented Aug 6, 2020

Uh oh!

Uh oh!

guoyu-wang Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skottmckay Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

guoyu-wang commented Aug 12, 2020

Uh oh!

guoyu-wang Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skottmckay Aug 17, 2020 • edited by guoyu-wang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pranavsharma Aug 17, 2020 • edited by guoyu-wang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guoyu-wang Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

pranavsharma Aug 18, 2020

guoyu-wang Aug 6, 2020 •

edited

Loading

pranavsharma Aug 6, 2020 •

edited by guoyu-wang

Loading

skottmckay Aug 6, 2020 •

edited

Loading

pranavsharma Aug 6, 2020 •

edited by guoyu-wang

Loading

pranavsharma Aug 6, 2020 •

edited by guoyu-wang

Loading

pranavsharma Aug 6, 2020 •

edited by guoyu-wang

Loading

skottmckay commented Aug 6, 2020 •

edited by guoyu-wang

Loading

guoyu-wang Aug 12, 2020 •

edited

Loading

guoyu-wang Aug 13, 2020 •

edited

Loading

skottmckay Aug 17, 2020 •

edited by guoyu-wang

Loading

pranavsharma Aug 17, 2020 •

edited by guoyu-wang

Loading