Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to bind arbitrary memory using raw pointers #10428

Merged
merged 7 commits into from
Feb 2, 2022

Conversation

yuslepukhin
Copy link
Member

@yuslepukhin yuslepukhin commented Jan 28, 2022

Description:
Introduce OrtExternalAllocation class that allows to supply type, shape and raw memory pointer for binding and other purposes. Introduce reasonable shape checks against the supplied size.

Could not find a suitable framework class that would give its memory back as a raw Ptr.

Motivation and Context
OrtIoBinding does not currently allow binding of raw native and/or device-based buffers that were user-allocated or potentially by another framework. Those are usually expressed by IntPtr.

Re: #10180

/// <param name="elementType">element type</param>
/// <param name="pointer">the actual pointer to memory</param>
/// <param name="sizeInBytes">size of the allocation in bytes</param>
public OrtExternalAllocation(OrtMemoryInfo memInfo, long[] shape, Tensors.TensorElementType elementType, IntPtr pointer, long sizeInBytes)
Copy link
Contributor

@skottmckay skottmckay Jan 31, 2022

Choose a reason for hiding this comment

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

Do we need this to be provided separately to the size that can be inferred from the shape? #Closed

Copy link
Member Author

@yuslepukhin yuslepukhin Jan 31, 2022

Choose a reason for hiding this comment

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

I was going back and forth on this. Finally, I decided to supply this argument/property to validate the user knows what they are doing. It is a common mistake to make a wrong choice betwen then number of elements and the buffer size in bytes.

if (width < 1)
{
throw new OnnxRuntimeException(ErrorCode.InvalidArgument, "Unsupported data type (such as string)");
}
Copy link
Contributor

@skottmckay skottmckay Jan 31, 2022

Choose a reason for hiding this comment

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

nit: include elementType in the error message #Closed

int width;
TensorElementTypeConverter.GetTypeAndWidth(elementType, out type, out width);
if (width < 1)
{
Copy link
Contributor

@skottmckay skottmckay Jan 31, 2022

Choose a reason for hiding this comment

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

nit: can we update GetTypeAndWidth to return true/false if successful? if not, would checking type==null be a little more obvious than width < 1? #Closed

}

/// <summary>
/// Bind externally allocated memory as input
Copy link
Member

@hariharans29 hariharans29 Jan 31, 2022

Choose a reason for hiding this comment

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

nit: Should this description also include a line similar to the other BindInput() overload (OrtMemoryAllocation continues to own the chunk of native memory and should be alive until the end of execution.) for completeness ? #Resolved

@@ -102,6 +109,23 @@ public void TestIOBindingWithOrtAllocation()
Assert.Equal(outputData, tensor.ToArray<float>(), new FloatComparer());
}
}
// 3. Pretend we are using external allocation which is currently on CPU
Copy link
Contributor

@edgchen1 edgchen1 Jan 31, 2022

Choose a reason for hiding this comment

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

Pretend we are using

perhaps actually use a separately allocated buffer?
then there's also an example that we can refer users to. #Resolved

throw new NotSupportedException(nameof(NativeOnnxTensorMemory<T>) + " does not support T = " + nameof(T));
{
var message = String.Format("The NativeOnnxTensorMemory<T> type being instantiated for T = : {0} while supplied OrtValue contains T = {1}",
nameof(T), nameof(type));
Copy link
Contributor

@edgchen1 edgchen1 Feb 1, 2022

Choose a reason for hiding this comment

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

nameof(T)

does this print "T" and "type"? I assume we want the actual types #Pending

/// The model will read the specified input from that memory
/// possibly avoiding the need to copy between devices. The user code continues to own
/// the chunk of externally allocated memory, and the allocation should be alive until the end of execution.
/// by the Tensor of the given size.
Copy link
Contributor

@edgchen1 edgchen1 Feb 1, 2022

Choose a reason for hiding this comment

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

by the Tensor of the given size.

missing part of the sentence. also appears in other method docs #Pending

if (elementType == TensorElementType.String)
{
throw new OnnxRuntimeException(ErrorCode.InvalidArgument,
"Can not use map managed strings buffer to native OrtValue");
Copy link
Contributor

@edgchen1 edgchen1 Feb 1, 2022

Choose a reason for hiding this comment

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

Can not use map

"Cannot map" #Pending

edgchen1
edgchen1 previously approved these changes Feb 1, 2022
Copy link
Contributor

@edgchen1 edgchen1 left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@skottmckay skottmckay left a comment

Choose a reason for hiding this comment

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

:shipit:

@yuslepukhin yuslepukhin merged commit 91b8ad5 into master Feb 2, 2022
@yuslepukhin yuslepukhin deleted the yuslepukhin/bind_external branch February 2, 2022 02:09
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.

4 participants