Skip to content

Conversation

@hariharans29
Copy link
Member

@hariharans29 hariharans29 commented Jan 8, 2021

Description: Add support in C# to configure a CUDA EP instance

Support for configuring a CUDA EP instance was added to C/C++/Python APIs recently. This change adds support for doing the same via the C# API.

TODO: Make changes to the C/C++ API section of the documentation once #6253 is checked-in

Motivation and Context
C# -> C API parity

@hariharans29 hariharans29 requested a review from a team as a code owner January 8, 2021 13:13
/// <param name="names">names to convert to zero terminated utf8 and pin</param>
/// <param name="cleanupList">list to add pinned memory to for later disposal</param>
/// <returns></returns>
private IntPtr[] ConvertNamesToUtf8<T>(IReadOnlyCollection<T> inputs, NameExtractor<T> extractor,
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to a utility class to share code

/// <param name="cudaProviderOptions">CUDA EP provider options to configure the CUDA EP instance</param>
public void AppendExecutionProvider_CUDA(OrtCUDAProviderOptions cudaProviderOptions)
{
NativeApiStatus.VerifySuccess(NativeMethods.SessionOptionsAppendExecutionProvider_CUDA(handle, cudaProviderOptions.Handle));
Copy link
Member Author

Choose a reason for hiding this comment

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

Main interface being made available to a user - configure and append a CUDA EP to SessionOptions

/// <summary>
/// Options for the CUDA provider that are passed to SessionOptionsAppendExecutionProvider_CUDA
/// </summary>
typedef struct OrtCUDAProviderOptions {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this struct to an internal header - this introduces only source incompatibility and ABI should still be preserved

Copy link
Contributor

@edgchen1 edgchen1 Jan 8, 2021

Choose a reason for hiding this comment

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

can we do this? what if a user is using SessionOptionsAppendExecutionProvider_CUDA() and directly using this struct?
also, if this can be moved, then how about moving OrtCudnnConvAlgoSearch out as well?


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

Copy link
Member Author

@hariharans29 hariharans29 Jan 9, 2021

Choose a reason for hiding this comment

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

  1. Can we do this ?
    I believe we can. In their app, if people are constructing OrtCUDAProviderOptions and are calling SessionOptionsAppendExecutionProvider_CUDA() by passing in a pointer to the constructed instance, that app should still work with the latest version of ORT, wouldn't it ? (i.e.) ABI is preserved. They won't be able to re-build their app with the latest ORT binaries because the latest header will now be missing the definition of OrtCUDAProviderOptions , so source compatibility doesn't exist - something we never guaranteed. They would have to use the API to create an instance of OrtCUDAProviderOptions to pass into SessionOptionsAppendExecutionProvider_CUDA(). We are trying to remove ambiguity in the way OrtCUDAProviderOptions can be constructed. We did something similar in Expose knobs to create and share (CPU) allocators across sessions in C# and Python #5634 for OrtArenaCfg.
    What do you think about this ?
    CC: @pranavsharma

  2. Can we move OrtCudnnConvAlgoSearch out as well ?
    Yes, I think so, moved it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

SessionOptionsAppendExecutionProvider_CUDA takes a pointer to OrtCUDAProviderOptions. The position and the signature of this function is not changing between v6 and v7. Hence in both old and new DLL cases, the correct function will be called. If the user is using the old header with the new DLL, she supplies the OrtCUDAProviderOptions ptr and all is good. New header with old DLL won't work with the same code (obviously) - unlikely scenario as there's no reason a user would upgrade without using the new DLL.

/**
* Use this API to create the configuration of a CUDA Execution Provider
*/
ORT_API2_STATUS(CreateCUDAProviderOptions, _Outptr_ OrtCUDAProviderOptions** out);
Copy link
Member Author

Choose a reason for hiding this comment

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

Expose an API to create native OrtCUDAProviderOptions to be invoked from C#

/**
* Use this API to set the appropriate configuration knobs of a CUDA Execution Provider
* Please refer to the following on different key/value pairs to configure a CUDA EP and their meaning:
* https://github.com/microsoft/onnxruntime/blob/gh-pages/docs/reference/execution-providers/CUDA-ExecutionProvider.md
Copy link
Member Author

Choose a reason for hiding this comment

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

The doc file will be checked-in in #6253

* Please refer to the following on different key/value pairs to configure a CUDA EP and their meaning:
* https://github.com/microsoft/onnxruntime/blob/gh-pages/docs/reference/execution-providers/CUDA-ExecutionProvider.md
*/
ORT_API2_STATUS(UpdateCUDAProviderOptions, _Inout_ OrtCUDAProviderOptions* cuda_provider_options,
Copy link
Member Author

Choose a reason for hiding this comment

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

Can provide options as key/value pairs - the same way it is done in Python

cuda_options_values.push_back("kNextPowerOfTwo");

// cuda mem limit
cuda_options_values.push_back(std::to_string(std::numeric_limits<size_t>::max()).c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

std::to_string(std::numeric_limits<size_t>::max()).c_str() [](start = 34, length = 58)

this appears to store a pointer to memory within a temporary variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gosh - yes, thanks for spotting that. I created a new var that holds the string equivalent of std::numeric_limits<size_t>::max() and its lifetime should now be the lifetime of this method and passed in the pointer to memory held by it to cuda_options_values.

public delegate IntPtr /*(OrtStatus*)*/DSessionOptionsAppendExecutionProvider_CUDA(
IntPtr /*(OrtSessionOptions*)*/ options,
IntPtr /*(const OrtCUDAProviderOptions*)*/ cudaProviderOptions);
public static DSessionOptionsAppendExecutionProvider_CUDA SessionOptionsAppendExecutionProvider_CUDA;
Copy link
Contributor

Choose a reason for hiding this comment

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

SessionOptionsAppendExecutionProvider_CUDA [](start = 66, length = 42)

this function's name looks different from the other append EP functions. can it be more consistent?

Copy link
Member Author

@hariharans29 hariharans29 Jan 9, 2021

Choose a reason for hiding this comment

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

That is because they are fundamentally different. OrtSessionOptionsAppendExecutionProvider_CUDA is a symbol available in the shared library if the CUDA EP is built. SessionOptionsAppendExecutionProvider_CUDA is available via the C API struct always (whether built with CUDA support or not).

Also NativeMethods is an "internal concept". If you take a look at the public interface in SessionOptions.cs - the method made available to the user is consistent. There are two overloads of AppendExecutionProvider_CUDA - one calls the OrtSessionOptionsAppendExecutionProvider_CUDA and the other calls SessionOptionsAppendExecutionProvider_CUDA and all these details are abstracted from the user.

Unfortunately, I couldn't think of a way to make the naming here more consistent given that we already have a OrtSessionOptionsAppendExecutionProvider_CUDA() defined here. Open to suggestions though.

/// <summary>
/// Updates the configuration knobs of OrtCUDAProviderOptions that will eventually be used to configure a CUDA EP
/// Please refer to the following on different key/value pairs to configure a CUDA EP and their meaning:
/// https://github.com/microsoft/onnxruntime/blob/gh-pages/docs/reference/execution-providers/CUDA-ExecutionProvider.md
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

@hariharans29 hariharans29 Jan 9, 2021

Choose a reason for hiding this comment

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

That seems right. I will use the link where it will be published.

/// <param name="keys">keys of all the configuration knobs of a CUDA Execution Provider</param>
/// <param name="values">values of all the configuration knobs of a CUDA Execution Provider (must match number of keys)</param>

public void UpdateOptions(string[] keys, string[] values)
Copy link
Contributor

Choose a reason for hiding this comment

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

string[] keys, string[] values [](start = 34, length = 30)

would a Dictionary<string, string> be a friendlier way to pass the config options?

/// This is CUDA provider specific but needs to live in a header that is build-flavor agnostic
/// Options for the CUDA provider that are passed to SessionOptionsAppendExecutionProvider_CUDA
/// </summary>
typedef struct OrtCUDAProviderOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

OrtCUDAProviderOptions [](start = 15, length = 22)

perhaps there is some overloaded naming of "provider options". i'd suggest putting this in a separate header (cuda_provider_options.h?) so we can keep the includes more finely-grained, i.e. only including OrtCUDAProviderOptions where it's needed. if you can come up with a better name for the map<string, string> used to store provider option configs that'd be great too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. I moved the struct to cuda_provider_options.h, but it can't be moved to a cuda folder and has to remain in the same folder as provider_options.h because it needs to be available even in non-CUDA builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't quite understand the second part of the comment - the one about map<string, string>

Copy link
Contributor

Choose a reason for hiding this comment

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

it's fine to leave it as ProviderOptions (the map<string, string>). we use "ProviderOptions" in the names of that typedef and the EP struct so i was just thinking it would be nice if the names were more different.

size_t num_keys) {
API_IMPL_BEGIN
#ifdef USE_CUDA
std::unordered_map<std::string, std::string> provider_options_map;
Copy link
Contributor

Choose a reason for hiding this comment

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

std::unordered_map<std::string, std::string [](start = 2, length = 43)

aka ProviderOptions

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done

provider_options_values == nullptr || provider_options_values[i][0] == '\0') {
return OrtApis::CreateStatus(ORT_INVALID_ARGUMENT, "key/value cannot be empty");
}
provider_options_map[std::string(provider_options_keys[i])] = std::string(provider_options_values[i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

std::string [](start = 25, length = 11)

nit: how about provider_options_map[provider_options_keys[i]] = provider_options_values[i];

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done

@edgchen1
Copy link
Contributor

edgchen1 commented Jan 8, 2021

i wonder if we should just move to supporting EP creation with config as string key-value pairs from the C API. would be nice to have a uniform way of specifying config for all EPs, and a single place to validate values and specify defaults. and fewer configuration structs in the public API. what do you think?

/// <summary>
/// Options for the CUDA provider that are passed to SessionOptionsAppendExecutionProvider_CUDA
/// </summary>
typedef struct OrtCUDAProviderOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

SessionOptionsAppendExecutionProvider_CUDA takes a pointer to OrtCUDAProviderOptions. The position and the signature of this function is not changing between v6 and v7. Hence in both old and new DLL cases, the correct function will be called. If the user is using the old header with the new DLL, she supplies the OrtCUDAProviderOptions ptr and all is good. New header with old DLL won't work with the same code (obviously) - unlikely scenario as there's no reason a user would upgrade without using the new DLL.


#pragma once

typedef enum OrtCudnnConvAlgoSearch {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need the typedef?

@edgchen1
Copy link
Contributor

i wonder if we should just move to supporting EP creation with config as string key-value pairs from the C API. would be nice to have a uniform way of specifying config for all EPs, and a single place to validate values and specify defaults. and fewer configuration structs in the public API. what do you think?

@hariharans29, @pranavsharma
could we have a single EP creation function like this?

SessionOptionsAppendExecutionProvider(
  OrtSessionOptions* session_options,
  const char* ep_name,
  size_t ep_config_count, const char* const* ep_config_keys, const char* const* ep_config_values)

@pranavsharma
Copy link
Contributor

i wonder if we should just move to supporting EP creation with config as string key-value pairs from the C API. would be nice to have a uniform way of specifying config for all EPs, and a single place to validate values and specify defaults. and fewer configuration structs in the public API. what do you think?

@hariharans29, @pranavsharma
could we have a single EP creation function like this?

SessionOptionsAppendExecutionProvider(
  OrtSessionOptions* session_options,
  const char* ep_name,
  size_t ep_config_count, const char* const* ep_config_keys, const char* const* ep_config_values)

Your proposal is fine, albeit a bit less convenient for users since they now have to worry about serializing the values. Not sure about the single place of validation; I don't think you'll able to validate much centrally besides matching the number of keys and values. Even before I didn't want EP specific config options to be exposed in the main C API header since it's unnecessary stuff that users get even when they're not using that specific EP, for e.g. see how we've both CUDA and OpenVino options exposed in c_api.h.

As a side note, we also have config keys in session options that we could use given that all the EP append functions accept a ptr to session options.


/**
* Use this API to set the appropriate configuration knobs of a CUDA Execution Provider
* Please refer to the following on different key/value pairs to configure a CUDA EP and their meaning:
Copy link
Member

Choose a reason for hiding this comment

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

Please refer to the following on [](start = 4, length = 32)

Sure, but we need to specifically state the format and the content for the params. Strings are UTF8, do they contain zero terminators, etc.

if (provider_options_keys[i] == nullptr || provider_options_keys[i][0] == '\0' ||
provider_options_values == nullptr || provider_options_values[i][0] == '\0') {
return OrtApis::CreateStatus(ORT_INVALID_ARGUMENT, "key/value cannot be empty");
}
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps, we can somehow add an index of the problematic pair?

OrtCUDAProviderOptions* cuda_options;
Ort::ThrowOnError(api.CreateCUDAProviderOptions(&cuda_options));
std::unique_ptr<OrtCUDAProviderOptions, decltype(api.ReleaseCUDAProviderOptions)> rel_cuda_options(cuda_options, api.ReleaseCUDAProviderOptions);

Copy link
Member

Choose a reason for hiding this comment

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

Would not be easier to provide a C++ interface that would make sure that we don't leak?

/**
* Use this API to create the configuration of a CUDA Execution Provider
*/
ORT_API2_STATUS(CreateCUDAProviderOptions, _Outptr_ OrtCUDAProviderOptions** out);
Copy link
Member

Choose a reason for hiding this comment

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

I vote for C++ interface


using Microsoft.ML.OnnxRuntime.Tensors;
using System;
using System.Linq;
Copy link
Member

Choose a reason for hiding this comment

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

System.Linq; [](start = 6, length = 12)

Check if this is really needed


using Microsoft.ML.OnnxRuntime.Tensors;
using System;
using System.Linq;
Copy link
Member

Choose a reason for hiding this comment

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

System.Linq;

Is this needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm referencing the C# code here.
I think is for ElementAt() and if I don't include System.Linq, I will get:
error CS1061: 'IReadOnlyCollection' does not contain a definition for 'ElementAt' and no accessible extension method 'ElementAt' accepting a first argument of type 'IReadOnlyCollection' could be found

providerOptionsDict["cudnn_conv_algo_search"] = "HEURISTIC";
providerOptionsDict["do_copy_in_default_stream"] = "1";

cudaProviderOptions.UpdateOptions(providerOptionsDict);
Copy link
Member

Choose a reason for hiding this comment

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

cudaProviderOptions.UpdateOptions(providerOptionsDict);

Is there any way to make sure that they actually had effect? Also can we add some negative test?

ORT_API2_STATUS(CreateCUDAProviderOptions, _Outptr_ OrtCUDAProviderOptions** out);

/**
* Use this API to set the appropriate configuration knobs of a CUDA Execution Provider
Copy link
Member

Choose a reason for hiding this comment

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

se this API to set the ap

The encoding must be mentioned. Also we should always document Doxygen params, although reference to external doc is also helpful.

@yuslepukhin
Copy link
Member

I am not seeing C++ API which is usually super helpful.

ProviderOptions provider_options_map;
for (size_t i = 0; i != num_keys; ++i) {
if (provider_options_keys[i] == nullptr || provider_options_keys[i][0] == '\0' ||
provider_options_values == nullptr || provider_options_values[i][0] == '\0') {
Copy link
Contributor

Choose a reason for hiding this comment

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

provider_options_values

provider_options_values[i]

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale due to inactivity and will be closed in 7 days if no further activity occurs. If further support is needed, please provide an update and/or more details.

@stale stale bot added the stale issues that have not been addressed in a while; categorized by a bot label Apr 16, 2022
@hariharans29 hariharans29 deleted the hari/cSharpApi branch August 26, 2022 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale issues that have not been addressed in a while; categorized by a bot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants