Deprecate Python global configuration functions [Part 1] #5923

edgchen1 · 2020-11-25T01:10:02Z

Description
Enable options to be set via execution provider (EP)-specific options and log deprecation warning from current global configuration functions.

TODO: Come up with convenient way to set EP-specific options (possibly refine existing APIs), update documentation and tests with new usage.

Motivation and Context
Deprecate global configuration functions in favor of EP-specific options.

…options to be set via EP-specific options and output deprecation warning from current global configuration functions.

…options

edgchen1 · 2020-11-25T20:00:15Z

/azp run orttraining-amd-gpu-ci-pipeline #Resolved

azure-pipelines · 2020-11-25T20:00:43Z

Azure Pipelines successfully started running 1 pipeline(s). #Resolved

pranavsharma · 2020-12-04T04:57:51Z

onnxruntime/python/onnxruntime_pybind_state.cc

-                                                                    cuda_provider_options.cuda_mem_limit,
-                                                                    cuda_provider_options.arena_extend_strategy,
-                                                                    cuda_provider_options.do_copy_in_default_stream));
+            sess, *onnxruntime::CreateExecutionProviderFactory_CUDA(info.device_id,


I know this is not the objective of this PR. Can CUDAExecutionProviderInfo be stored in the factory obj as-is instead of re-create it in CreateProvider() later? This will also keep the signature of this factory function clean - no need to keep adding more parameters as more options are added. Similar comment applies to other EPs like ROCM. #Resolved

ok, i can update the CUDA and ROCM EP factories for now

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

pranavsharma · 2020-12-04T04:57:51Z

onnxruntime/python/onnxruntime_pybind_state.cc

+      const auto nuphar_provider_options_it = provider_options_map.find(type);
+      if (nuphar_provider_options_it != provider_options_map.end()) {
+        const auto& nuphar_provider_options = nuphar_provider_options_it->second;
+        const auto nuphar_settings_it = nuphar_provider_options.find("nuphar_settings");


Can ReadProviderOption be used here? #Resolved

pranavsharma · 2020-12-04T04:57:51Z

onnxruntime/python/onnxruntime_pybind_state.cc

+        const auto& nuphar_provider_options = nuphar_provider_options_it->second;
+        const auto nuphar_settings_it = nuphar_provider_options.find("nuphar_settings");
+        if (nuphar_settings_it != nuphar_provider_options.end()) {
+          nuphar_settings = nuphar_settings_it->second;


nit: can avoid the copy. #Resolved

pranavsharma · 2020-12-04T04:57:51Z

include/onnxruntime/core/framework/provider_options_utils.h

+ * Returns true if the option is present, false otherwise.
+ */
+template <typename T>
+bool ReadProviderOption(const ProviderOptions& options, const std::string& key, T& value) {


Is using optional better instead of returning true/false? #Resolved

good idea

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

actually this signature is useful for the existing usage:

CUDAExecutionProviderInfo info{}; ReadProviderOption(options, provider_option_names::kMemLimit, info.cuda_mem_limit);

but i can add another function that returns an optional

In reply to: 536446380 [](ancestors = 536446380,535835891)

Sure. One other thing I noticed is that we validate values returned by this function for only some of the keys. GetOrDefault might be better I guess instead of returning optional as the user will not have the burden to check for the value in optional. Also, it should satisfy all usages. #Resolved

Ah ok, i can change it to ReadProviderOptionOrDefault()

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

pranavsharma · 2020-12-04T05:00:36Z

include/onnxruntime/core/common/string_utils.h

+ * Tries to parse a value from an entire string.
+ */
+template <typename T>
+bool TryParse(const std::string& str, T& value) {


Would using stol family of functions be simpler?

not sure if it would. one nice thing about operator>> is that i don't need to call different functions explicitly. also can specify a particular locale without having to update the global one.

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

azure-pipelines · 2020-12-04T23:39:53Z

No pipelines are associated with this pull request. #Resolved

…options

edgchen1 · 2020-12-08T22:59:59Z

/azp run orttraining-amd-gpu-ci-pipeline

azure-pipelines · 2020-12-08T23:00:05Z

No pipelines are associated with this pull request.

suffiank · 2020-12-09T00:00:30Z

onnxruntime/core/providers/cuda/cuda_execution_provider_info.cc

+        "Invalid ", provider_option_names::kDeviceId, " value: ", info.device_id,
+        ", must be between 0 (inclusive) and ", num_devices, " (exclusive).");
+  }
+  ReadProviderOption(options, provider_option_names::kMemLimit, info.cuda_mem_limit);


I think it just sets the initial arena size. Maybe it needs a clearer name. @codemzs

i think it is the maximum, looks like the value eventually goes to

onnxruntime/include/onnxruntime/core/framework/allocator.h

Line 15 in 7a43fa0

size_t max_mem; // use 0 to allow ORT to choose the default

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

OK, makes sense then. It looks like an unset value defaults to the numeric limit.

suffiank · 2020-12-09T21:04:35Z

onnxruntime/core/providers/cuda/cuda_execution_provider_info.cc

+constexpr const char* kMemLimit = "cuda_mem_limit";
+constexpr const char* kArenaExtendStrategy = "arena_extend_strategy";
+constexpr const char* kCudnnConvAlgo = "cudnn_conv_algo";
+constexpr const char* kDoCopyInDefaultStream = "do_copy_in_default_stream";


Are there descriptors for each of these?

hm, not sure if there are. will add documentation if needed in part 2

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

suffiank · 2020-12-09T21:04:35Z

onnxruntime/python/onnxruntime_pybind_state.cc

-  m.def("set_cuda_device_id", [](const int id) { cuda_device_id = static_cast<OrtDevice::DeviceId>(id); });
+  // TODO remove deprecated global config
+  m.def("set_cuda_device_id", [](const int id) {
+    LogDeprecationWarning("set_cuda_device_id", "CUDA/ROCM execution provider option \"device_id\"");


What's the correct way to set these options? Is that for Part 2?

through the EP specific options. part 2 will refine the existing way of setting EP specific options

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

edgchen1 · 2020-12-15T03:46:56Z

/azp run Linux CPU CI Pipeline

edgchen1 · 2020-12-15T03:48:39Z

/azp run Linux CPU x64 NoContribops CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,MacOS NoContribops CI Pipeline,Windows CPU CI Pipeline,orttraining-linux-gpu-ci-pipeline

edgchen1 · 2020-12-15T14:38:38Z

/azp run Linux CPU x64 NoContribops CI Pipeline

edgchen1 · 2020-12-15T14:39:22Z

/azp run Linux GPU CI Pipeline

edgchen1 · 2020-12-15T14:39:37Z

/azp run Linux GPU TensorRT CI Pipeline

edgchen1 · 2020-12-15T14:39:53Z

/azp run MacOS NoContribops CI Pipeline

edgchen1 · 2020-12-15T14:40:07Z

/azp run Windows CPU CI Pipeline

edgchen1 · 2020-12-15T14:40:21Z

/azp run orttraining-linux-gpu-ci-pipeline

edgchen1 added 3 commits Nov 25, 2020

Part 1 of deprecating Python global configuration functions - enable …

921c90f

…options to be set via EP-specific options and output deprecation warning from current global configuration functions.

Merge remote-tracking branch 'origin/master' into edgchen1/python_ep_…

697e44a

…options

Update isspace call.

Loading status checks…

dddc83d

edgchen1 requested review from pranavsharma, hariharans29, KeDengMS, jywu-msft and chilo-ms Nov 25, 2020

edgchen1 added 4 commits Nov 25, 2020

Use initialization compatible with GCC 4.8.

eb70f43

Fix unused function warning.

Loading status checks…

1033928

Fix build errors.

Loading status checks…

6e74d4c

Try to fix build error.

Loading status checks…

768fb54

edgchen1 marked this pull request as ready for review Nov 25, 2020

edgchen1 requested a review from microsoft/onnxruntime as a code owner Nov 25, 2020

edgchen1 added 4 commits Nov 25, 2020

Add include.

Loading status checks…

1a3c00c

Move CUDA and ROCM execution provider info classes into separate files.

Loading status checks…

0281978

Make build.py executable again.

2e477bf

Add exclusions to amd_hipify.py.

Loading status checks…

2e350a6

edgchen1 requested review from weixingzhang and suffiank Nov 26, 2020

pranavsharma reviewed Dec 4, 2020

View changes

edgchen1 added 4 commits Dec 5, 2020

Address PR comments.

Loading status checks…

b8522f7

ReadProviderOptionOrDefault()

Loading status checks…

e89f697

Add pybind enum for OrtCudnnConvAlgoSearch.

585066b

Move provider factory creator functions into separate headers.

Loading status checks…

21a0715

pranavsharma reviewed Dec 8, 2020

View changes

Merge remote-tracking branch 'origin/master' into edgchen1/python_ep_…

Loading status checks…

5fe05df

…options

suffiank reviewed Dec 9, 2020

View changes

edgchen1 added 4 commits Dec 10, 2020

Put ProviderOptions in separate header file.

bd5c349

Fix build.

7573a2f

Reorder includes alphabetically.

93c181d

Fix string literals.

Loading status checks…

a0da40c

edgchen1 dismissed stale reviews from suffiank and pranavsharma via a0da40c Dec 12, 2020

Merge branch 'master' into edgchen1/python_ep_options

Loading status checks…

cd04c6b

pranavsharma approved these changes Dec 15, 2020

View changes

edgchen1 deleted the edgchen1/python_ep_options branch Dec 15, 2020

microsoft / onnxruntime

Deprecate Python global configuration functions [Part 1] #5923

Deprecate Python global configuration functions [Part 1] #5923

edgchen1 commented Nov 25, 2020

edgchen1 commented Nov 25, 2020 •

edited

azure-pipelines bot commented Nov 25, 2020 •

edited by edgchen1

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

azure-pipelines bot commented Dec 4, 2020 •

edited by edgchen1

edgchen1 commented Dec 8, 2020

azure-pipelines bot commented Dec 8, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

microsoft / onnxruntime

Join GitHub today

GitHub is where the world builds software

Deprecate Python global configuration functions [Part 1] #5923

Deprecate Python global configuration functions [Part 1] #5923

Conversation

edgchen1 commented Nov 25, 2020

edgchen1 commented Nov 25, 2020 • edited

azure-pipelines bot commented Nov 25, 2020 • edited by edgchen1

This comment has been minimized.

pranavsharma Dec 4, 2020 • edited by edgchen1 Contributor

This comment has been minimized.

edgchen1 Dec 4, 2020 Author Contributor

This comment has been minimized.

pranavsharma Dec 4, 2020 • edited by edgchen1 Contributor

This comment has been minimized.

pranavsharma Dec 4, 2020 • edited by edgchen1 Contributor

This comment has been minimized.

pranavsharma Dec 4, 2020 • edited by edgchen1 Contributor

This comment has been minimized.

edgchen1 Dec 4, 2020 Author Contributor

This comment has been minimized.

edgchen1 Dec 4, 2020 Author Contributor

This comment has been minimized.

pranavsharma Dec 5, 2020 • edited by edgchen1 Contributor

This comment has been minimized.

edgchen1 Dec 5, 2020 Author Contributor

This comment has been minimized.

pranavsharma Dec 4, 2020 Contributor

This comment has been minimized.

edgchen1 Dec 4, 2020 Author Contributor

azure-pipelines bot commented Dec 4, 2020 • edited by edgchen1

edgchen1 commented Dec 8, 2020

azure-pipelines bot commented Dec 8, 2020

This comment has been minimized.

suffiank Dec 9, 2020 Contributor

This comment has been minimized.

edgchen1 Dec 9, 2020 Author Contributor

This comment has been minimized.

suffiank Dec 9, 2020 Contributor

This comment has been minimized.

suffiank Dec 9, 2020 Contributor

This comment has been minimized.

edgchen1 Dec 9, 2020 Author Contributor

This comment has been minimized.

suffiank Dec 9, 2020 Contributor

This comment has been minimized.

edgchen1 Dec 9, 2020 Author Contributor

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Dec 15, 2020

edgchen1 commented Nov 25, 2020 •

edited

azure-pipelines bot commented Nov 25, 2020 •

edited by edgchen1

pranavsharma Dec 4, 2020 •

edited by edgchen1

Contributor

edgchen1 Dec 4, 2020
Author Contributor

pranavsharma Dec 4, 2020 •

edited by edgchen1

Contributor

pranavsharma Dec 4, 2020 •

edited by edgchen1

Contributor

pranavsharma Dec 4, 2020 •

edited by edgchen1

Contributor

edgchen1 Dec 4, 2020
Author Contributor

edgchen1 Dec 4, 2020
Author Contributor

pranavsharma Dec 5, 2020 •

edited by edgchen1

Contributor

edgchen1 Dec 5, 2020
Author Contributor

pranavsharma Dec 4, 2020
Contributor

edgchen1 Dec 4, 2020
Author Contributor

azure-pipelines bot commented Dec 4, 2020 •

edited by edgchen1

suffiank Dec 9, 2020
Contributor

edgchen1 Dec 9, 2020
Author Contributor

suffiank Dec 9, 2020
Contributor

suffiank Dec 9, 2020
Contributor

edgchen1 Dec 9, 2020
Author Contributor

suffiank Dec 9, 2020
Contributor

edgchen1 Dec 9, 2020
Author Contributor