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

Adds the new System.Numerics.Tensors as an input/output type when using dotnet 8.0 and up. #23261

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

michaelgsharp
Copy link
Member

Description

Adds the new System.Numerics.Tensors as an input/output type when using dotnet 8.0 and up. It does not change/remove any of the existing API, only adds additional ones.

Motivation and Context

Now that C#/Dotnet has an official tensor type built into the language, we want to expand the places that it can be used.

@snnn snnn added the core runtime issues related to core runtime label Jan 8, 2025
@yuslepukhin yuslepukhin requested a review from skottmckay January 8, 2025 19:23
}
unsafe
{
var backingData = (T[])tensor.GetType().GetField("_values", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(tensor);
Copy link
Member

Choose a reason for hiding this comment

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

I was having a look at @tjwald's project and saw he has a clever trick for this: https://github.com/tjwald/high-perf-ML/blob/d5054ca0bf882570e59a5d960d45e34b64b81b5d/ML.Infra/TensorExtensions.cs#L28

It seems that generic support like this might only work in .NET 9 though, based on the docs https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.unsafeaccessorattribute?view=net-9.0

Copy link
Member

@yuslepukhin yuslepukhin Jan 17, 2025

Choose a reason for hiding this comment

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

One question about the compatibility worries me. We support Linux, MacOS, Android and iOS with the same codebase.
Typically, we looked at NETSTANDARD compatibility for our library.
Now with a transition to NET monikers it is sometimes hard to say what is compatible with what.

Copy link
Member Author

Choose a reason for hiding this comment

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

@yuslepukhin Using #if NET8_0_OR_GREATER should make sure the compatibility is fine. Or are there any edge cases or anything we need to worry about @ericstj ?

I'll wrap that section for just net9 or greater than. And i think the other extensions he has there would be something good to discuss to see if they are a good fit for the BCL as well.

Copy link

Choose a reason for hiding this comment

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

And i think the other extensions he has there would be something good to discuss

@michaelgsharp I am not sure where to discuss your comment (maybe here?)

Regarding the other extension:

public static Span<T> GetRowSpan<T>(this TensorSpan<T> tensor, int i)

It is only necessary because the tensor primitives don't support row by row operations.
I needed to run a softmax on each row of a tensor, but the softmax in tensor primitives does it on the whole tensor and not row by row.

Copy link
Member

Choose a reason for hiding this comment

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

One question about the compatibility worries me. We support Linux, MacOS, Android and iOS with the same codebase.
Typically, we looked at NETSTANDARD compatibility for our library.
Now with a transition to NET monikers it is sometimes hard to say what is compatible with what.

The only thing .NETStandard gives over .NET targeting is support for .NETFramework. You don't lose that by adding more targetframeworks - you can just specialize your builds that target newer frameworks.

@@ -605,6 +683,80 @@ public static OrtValue CreateTensorValueFromMemory<T>(T[] data, long[] shape) wh
return OrtValue.CreateTensorValueFromMemory(OrtMemoryInfo.DefaultInstance, new Memory<T>(data), shape);
}

#if NET8_0_OR_GREATER
/// <summary>
/// This is a factory method creates a native Onnxruntime OrtValue containing a tensor.
Copy link
Member

@yuslepukhin yuslepukhin Jan 22, 2025

Choose a reason for hiding this comment

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

OrtValue containing a tensor

On top of the existing tensor managed memory.

try
{
IntPtr dataBufferPointer = IntPtr.Zero;
unsafe
Copy link
Member

Choose a reason for hiding this comment

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

unsafe

This unsafe is nested. Do we need it?

@@ -1,12 +1,19 @@
using Microsoft.ML.OnnxRuntime.Tensors;
using Microsoft.VisualStudio.TestPlatform.Utilities;
Copy link
Member

Choose a reason for hiding this comment

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

using Microsoft.VisualStud

Can we ensure that usings are sorted? VS has a feature to do it.

cleanUp.Add(options);
options.GraphOptimizationLevel = graphOptimizationLevel;
if (enableParallelExecution) options.ExecutionMode = ExecutionMode.ORT_PARALLEL;

Copy link
Member

@yuslepukhin yuslepukhin Jan 22, 2025

Choose a reason for hiding this comment

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

We no longer optimize for it. It may disappear soon. Does not produce best perf so we should not put it out as an example.

}
}

session.Dispose();
Copy link
Member

@yuslepukhin yuslepukhin Jan 22, 2025

Choose a reason for hiding this comment

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

session

session may be disposed twice as it is in the cleanup list, it may cause an unexplained crashes.
Also an explicit call to Dispose() is usually an indication of exception safety problem and a memory leak.

cleanUp.Add(session);

using var runOptions = new RunOptions();
using var inputOrtValues = new DisposableListTest<DisposableTestPair<OrtValue>>(session.InputMetadata.Count);
Copy link
Member

Choose a reason for hiding this comment

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

using var inputOrtValue

Move it closer to where it is needed


float[] expectedOutput = TestDataLoader.LoadTensorFromEmbeddedResource("bench.expected_out");
long[] expectedDimensions = { 1, 1000, 1, 1 }; // hardcoded for now for the test data
ReadOnlySpan<long> expectedOutputDimensions = expectedDimensions;
Copy link
Member

Choose a reason for hiding this comment

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

expectedOutputDimensions

Is this used?

var ex = Assert.Throws<ArgumentException>(() => session.Run(runOptions, ["data_0"], [inputOrtValues[0].Value], ["softmaxout_1"], outputs));
Assert.StartsWith("Length of outputNames (1) must match that of outputValues (0).", ex.Message);
}
session.Dispose();
Copy link
Member

Choose a reason for hiding this comment

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

session.Dispose();

Should be disposed of in an exception safe manner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core runtime issues related to core runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants