Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions csharp/src/Microsoft.ML.OnnxRuntime/NativeMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,22 @@ public struct OrtApi
public IntPtr ModelMetadataGetGraphDescription;
}

#region ORT Provider options

[StructLayout(LayoutKind.Sequential)]
public struct OrtCUDAProviderOptionsNative
{
public int device_id; // cuda device with id=0 as default device.

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.

int [](start = 15, length = 3)

I would want to verify that this is being marshalled correctly. In C# int is always 4 bytes. In the native code on Windows it is 4 bytes but on Linux it is 8 bytes. So it might work OK on windows but we need it work everywhere.

@snnn snnn Apr 14, 2021

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.

No. Even on Linux int is still 4 bytes. You were talking 'long' #Resolved

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.

You are correct.


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

@yuslepukhin yuslepukhin Apr 15, 2021

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.

We may have an issue here. The new C API was added without giving a though how this is going to interact with other languages even though the purpose of having C API is language binding. So you will need to verify the fact in the native debugger that the marshalling occurs correctly and make the test test.


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

public OrtCudnnConvAlgoSearch cudnn_conv_algo_search; // cudnn conv algo search option
public UIntPtr gpu_mem_limit; // default cuda memory limitation to maximum finite value of size_t.
public int arena_extend_strategy; // default area extend strategy to KNextPowerOfTwo.
public int do_copy_in_default_stream; // Whether to do copies in the default stream or use separate streams.
public int has_user_compute_stream; // indicator of user specified CUDA compute stream.
public IntPtr user_compute_stream; // user specified CUDA compute stream.
}

#endregion

internal static class NativeMethods
{
private const string nativeLib = "onnxruntime";
Expand Down Expand Up @@ -256,6 +272,8 @@ static NativeMethods()
OrtRegisterCustomOpsLibrary = (DOrtRegisterCustomOpsLibrary)Marshal.GetDelegateForFunctionPointer(api_.RegisterCustomOpsLibrary, typeof(DOrtRegisterCustomOpsLibrary));
OrtAddSessionConfigEntry = (DOrtAddSessionConfigEntry)Marshal.GetDelegateForFunctionPointer(api_.AddSessionConfigEntry, typeof(DOrtAddSessionConfigEntry));
OrtAddInitializer = (DOrtAddInitializer)Marshal.GetDelegateForFunctionPointer(api_.AddInitializer, typeof(DOrtAddInitializer));
SessionOptionsAppendExecutionProvider_CUDA = (DSessionOptionsAppendExecutionProvider_CUDA)Marshal.GetDelegateForFunctionPointer(
api_.SessionOptionsAppendExecutionProvider_CUDA, typeof(DSessionOptionsAppendExecutionProvider_CUDA));

OrtCreateRunOptions = (DOrtCreateRunOptions)Marshal.GetDelegateForFunctionPointer(api_.CreateRunOptions, typeof(DOrtCreateRunOptions));
OrtReleaseRunOptions = (DOrtReleaseRunOptions)Marshal.GetDelegateForFunctionPointer(api_.ReleaseRunOptions, typeof(DOrtReleaseRunOptions));
Expand Down Expand Up @@ -561,6 +579,17 @@ IntPtr[] outputValues /* An array of output value pointers. Array must be alloca
[DllImport(nativeLib, CharSet = charSet)]
public static extern IntPtr /*(OrtStatus*)*/ OrtSessionOptionsAppendExecutionProvider_CUDA(IntPtr /*(OrtSessionOptions*) */ options, int device_id);

/// <summary>
/// Append a CUDA EP instance (configured based on given provider options) to the native OrtSessionOptions instance
/// </summary>
/// <param name="options">Native OrtSessionOptions instance</param>
/// <param name="cudaProviderOptions">Native OrtCUDAProviderOptions instance</param>
public delegate IntPtr /*(OrtStatus*)*/DSessionOptionsAppendExecutionProvider_CUDA(
IntPtr /*(OrtSessionOptions*)*/ options,
ref OrtCUDAProviderOptionsNative cudaProviderOptions);

public static DSessionOptionsAppendExecutionProvider_CUDA SessionOptionsAppendExecutionProvider_CUDA;

[DllImport(nativeLib, CharSet = charSet)]
public static extern IntPtr /*(OrtStatus*)*/ OrtSessionOptionsAppendExecutionProvider_ROCM(IntPtr /*(OrtSessionOptions*) */ options, int device_id);

Expand Down
108 changes: 108 additions & 0 deletions csharp/src/Microsoft.ML.OnnxRuntime/ProviderOptions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.InteropServices;

namespace Microsoft.ML.OnnxRuntime
{
/// <summary>
/// Search Algorithm for Cudnn Conv.
/// </summary>
public enum OrtCudnnConvAlgoSearch
{
EXHAUSTIVE, //!< expensive exhaustive benchmarking using cudnnFindConvolutionForwardAlgorithmEx
HEURISTIC, //!< lightweight heuristic based search using cudnnGetConvolutionForwardAlgorithm_v7
DEFAULT, //!< default algorithm using CUDNN_CONVOLUTION_FWD_ALGO_IMPLICIT_PRECOMP_GEMM
}

/// <summary>
/// Provider options for CUDA EP.
/// </summary>
public struct OrtCUDAProviderOptions

@yuslepukhin yuslepukhin Apr 14, 2021

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.

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

This seems to be a duplicate class. Have you considered to use the same class everywhere? #Resolved

@chilo-ms chilo-ms Apr 15, 2021

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.

Made only one OrtCUDAProviderOptions exist. #Resolved

{
public int device_id; //!< cuda device with id=0 as default device.
public OrtCudnnConvAlgoSearch cudnn_conv_algo_search; //!< cudnn conv algo search option
public UIntPtr gpu_mem_limit; //!< default cuda memory limitation to maximum finite value of size_t.
public int arena_extend_strategy; //!< default area extend strategy to KNextPowerOfTwo.
public int do_copy_in_default_stream; //!< Whether to do copies in the default stream or use separate streams.
}

/// <summary>
/// Holds provider options configuration for creating an InferenceSession.
/// </summary>
public class ProviderOptions : SafeHandle

@yuslepukhin yuslepukhin Apr 12, 2021

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.

SafeHandle [](start = 35, length = 10)

You made this class IDisposable while the code does not have a real handle. My understanding that you did it for the future. However, this means, that even now it must be disposed.
Even if it currently does not have a real handle to dispose, in the future, it might have it, but the code will set a bad example of not disposing it and will leak. #Resolved

@chilo-ms chilo-ms Apr 13, 2021

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 for reminding the importance of presence of dispose function.
I've added dispose function in this class, even though it currently doesn't have a real handle as you found. #Resolved

@yuslepukhin yuslepukhin Apr 14, 2021

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.

I think you need to read on how to implement IDisposable and what SafeHandle is.

  1. What is it that you are disposing? Handle is never initialized to any meaningful value and it is not clear what it represents. Your Dispose() simply sets it to zero and does nothing.
  2. SafeHandle already implements Dispose() and it calls ReleaseHandle() so the imlpementor of the class does the custom thing that they need to do. In this case your Dispose() overrides SafeHandle implementation and prevents ReleaseHandle() to be ever called. If it was it would be a never ending recursion.
  3. Your handle always has a zero value so SafeHandle would never calls ReleaseHandle() because it thinks it is already disposed.

This class as far as I can see should not inherit from SafeHandle.


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

@chilo-ms chilo-ms Apr 15, 2021

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.

The reason I made this class a derived class from SafeHandle is for future use where the class needs to call ort native api and should be released once it's done.
Agree with you that so far this class doesn't need to inherit from SafeHandle. For simplicity, I made this class a simple static class.

BTW, just curious regarding second point you mentioned that it would be a never ending recursion. Since ReleaseHandle will never be called, and my implementation of Dispose() will be called once, how can recursion happen? #Resolved

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.

You should not override Dispose(). If that is removed and ReleaseHandle() continues to call Dispose() it would recurse.


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

{
internal IntPtr Handle
{
get
{
return handle;
}
}

#region Constructor and Factory methods

/// <summary>
/// Constructs an empty ProviderOptions
/// </summary>
public ProviderOptions()
: base(IntPtr.Zero, true)
{
}

#endregion

#region Public Methods

/// <summary>
/// Get CUDA provider options with default setting.
/// </summary>
/// <returns> CUDA provider options instance. </returns>
public static OrtCUDAProviderOptions GetDefaultCUDAProviderOptions()
{
OrtCUDAProviderOptions cuda_options;
cuda_options.device_id = 0;
cuda_options.cudnn_conv_algo_search = OrtCudnnConvAlgoSearch.EXHAUSTIVE;
if (IntPtr.Size == 8)
{
cuda_options.gpu_mem_limit = (UIntPtr)UInt64.MaxValue;
}
else
{
cuda_options.gpu_mem_limit = (UIntPtr)UInt32.MaxValue;
}
cuda_options.arena_extend_strategy = 0;
cuda_options.do_copy_in_default_stream = 1;

return cuda_options;
}
#endregion

#region Public Properties

/// <summary>
/// Overrides SafeHandle.IsInvalid
/// </summary>
/// <value>returns true if handle is equal to Zero</value>
public override bool IsInvalid { get { return handle == IntPtr.Zero; } }

@yuslepukhin yuslepukhin Apr 12, 2021

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.

IsInvalid [](start = 29, length = 9)

My understanding is that handle is never initialized and therefore is never valid. Is this ok? #Resolved

@chilo-ms chilo-ms Apr 13, 2021

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.

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.

Correct. I will always be invalid. What is the purpose of this class being SafeHandle?


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

@chilo-ms chilo-ms Apr 15, 2021

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.

For future use. But agree with you that it's not needed to inherit from SafeHandle. Already made this class simple static class. #Resolved


#endregion

#region SafeHandle
/// <summary>
/// Overrides SafeHandle.ReleaseHandle() to properly dispose of
/// the native instance of SessionOptions
/// </summary>
/// <returns>always returns true</returns>
protected override bool ReleaseHandle()
{
handle = IntPtr.Zero;
return true;
}

#endregion
}
}
43 changes: 43 additions & 0 deletions csharp/src/Microsoft.ML.OnnxRuntime/SessionOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,30 @@ public static SessionOptions MakeSessionOptionWithCudaProvider(int deviceId = 0)
return options;
}

/// <summary>
/// A helper method to construct a SessionOptions object for CUDA execution.
/// Use only if CUDA is installed and you have the onnxruntime package specific to this Execution Provider.
/// </summary>
/// <param name="cuda_options">CUDA EP provider options to configure the CUDA EP instance</param>>
/// <returns>A SessionsOptions() object configured for execution with CUDA provider options.</returns>
public static SessionOptions MakeSessionOptionWithCudaProvider(OrtCUDAProviderOptions cuda_options)
{
CheckCudaExecutionProviderDLLs();
SessionOptions options = new SessionOptions();

@yuslepukhin yuslepukhin Apr 12, 2021

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.

options [](start = 27, length = 7)

SessionOptions is a IDisposable. What happens to this instance when either of the below native methods return an error VerifySuccess throws?
What happens to cuda_options_native handle when the second call to native method throws? #Resolved

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.

Put native methods inside using-block. Since SessionOptions is an IDisposable, dispose function will be called once errors/exceptions happen.

BTW, I found out that our SessionOptions class didn't contain dispose function. Just added.

@yuslepukhin yuslepukhin Apr 14, 2021

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.

The implementation of SessionOptions is correct. You do not need to change it. #Resolved

@chilo-ms chilo-ms Apr 15, 2021

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.

I've undo my change. #Resolved


OrtCUDAProviderOptionsNative cuda_options_native;

@yuslepukhin yuslepukhin Apr 12, 2021

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.

cuda_options_native [](start = 41, length = 19)

Where is the code that disposes of cuda_options_native? #Resolved

@chilo-ms chilo-ms Apr 13, 2021

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.

cuda_options_native is managed object, so it will be reclaimed by gc. Correct me if I'm wrong. #Resolved

@yuslepukhin yuslepukhin Apr 14, 2021

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.

Sure it is. I suggest renaming the class so it does not include native in its name.


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

@chilo-ms chilo-ms Apr 15, 2021

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.

Renamed it to avoid confusion. #Resolved

cuda_options_native.device_id = cuda_options.device_id;
cuda_options_native.cudnn_conv_algo_search = cuda_options.cudnn_conv_algo_search;
cuda_options_native.gpu_mem_limit = cuda_options.gpu_mem_limit;
cuda_options_native.arena_extend_strategy = cuda_options.arena_extend_strategy;
cuda_options_native.do_copy_in_default_stream = cuda_options.do_copy_in_default_stream;
cuda_options_native.has_user_compute_stream = 0;
cuda_options_native.user_compute_stream = IntPtr.Zero;
NativeApiStatus.VerifySuccess(NativeMethods.SessionOptionsAppendExecutionProvider_CUDA(options.Handle, ref cuda_options_native));
NativeApiStatus.VerifySuccess(NativeMethods.OrtSessionOptionsAppendExecutionProvider_CPU(options.Handle, 1));
return options;
}

/// <summary>
/// A helper method to construct a SessionOptions object for Nuphar execution.
/// Use only if you have the onnxruntime package specific to this Execution Provider.
Expand Down Expand Up @@ -148,6 +172,24 @@ public void AppendExecutionProvider_CUDA(int deviceId)
NativeApiStatus.VerifySuccess(NativeMethods.OrtSessionOptionsAppendExecutionProvider_CUDA(handle, deviceId));
}

/// <summary>
/// Append a CUDA EP instance (based on specified configuration) to the SessionOptions instance
/// Use only if you have the onnxruntime package specific to this Execution Provider.
/// </summary>
/// <param name="cuda_options">CUDA EP provider options to configure the CUDA EP instance</param>
public void AppendExecutionProvider_CUDA(OrtCUDAProviderOptions cuda_options)
{
OrtCUDAProviderOptionsNative cuda_options_native;
cuda_options_native.device_id = cuda_options.device_id;
cuda_options_native.cudnn_conv_algo_search = cuda_options.cudnn_conv_algo_search;
cuda_options_native.gpu_mem_limit = cuda_options.gpu_mem_limit;
cuda_options_native.arena_extend_strategy = cuda_options.arena_extend_strategy;
cuda_options_native.do_copy_in_default_stream = cuda_options.do_copy_in_default_stream;
cuda_options_native.has_user_compute_stream = 0;
cuda_options_native.user_compute_stream = IntPtr.Zero;
NativeApiStatus.VerifySuccess(NativeMethods.SessionOptionsAppendExecutionProvider_CUDA(handle, ref cuda_options_native));
}

/// <summary>
/// Use only if you have the onnxruntime package specific to this Execution Provider.
/// </summary>
Expand Down Expand Up @@ -325,6 +367,7 @@ public void AddFreeDimensionOverrideByName(string dimName, long dimValue)
NativeApiStatus.VerifySuccess(NativeMethods.OrtAddFreeDimensionOverrideByName(handle, pinnedDimName.Pointer, dimValue));
}
}

#endregion

internal IntPtr Handle
Expand Down
12 changes: 12 additions & 0 deletions csharp/test/Microsoft.ML.OnnxRuntime.Tests/InferenceTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,18 @@ public void CanCreateAndDisposeSessionWithModelPath()
}
}
}
#if USE_CUDA
[Fact]
private void TestCUDAProviderOptions()
{

OrtCUDAProviderOptions cuda_options = ProviderOptions.GetDefaultCUDAProviderOptions();
using (var sessionOptions = new SessionOptions())

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.

This does not test anything. Let's find a way to verify that this indeed happened

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.

BTW, currently I forgot to pass USE_CUDA in.

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.

You can see "Microsoft.ML.OnnxRuntime.Tests.InferenceTest.TestGpu" was skipped in your test run.

{
sessionOptions.AppendExecutionProvider_CUDA(cuda_options);
}
}
#endif

[Theory]
[InlineData(GraphOptimizationLevel.ORT_DISABLE_ALL, true)]
Expand Down