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

Mark the new tensor APIs as experimental for .NET 9 #105268

Merged
merged 7 commits into from
Jul 24, 2024

Conversation

tannergooding
Copy link
Member

Marking Tensor<T> as [Experimental]

Expanding .NET to provide built-in types for exchanging tensor data across libraries and allowing accelerated handling for core operations is an important but large undertaking. The work done in .NET 9 currently encompasses 8 types and nearly 600 new public APIs, many of which are generic and can support arbitrary T or T constrained to one of the generic math interfaces (see https://learn.microsoft.com/en-us/dotnet/standard/generics/math).

Due to the size of this work and the recognized importance of it to the long term growth of the .NET ecosystem, it has been decided to mark these new APIs as [Experimental] (see https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.experimentalattribute) for .NET 9 and to plan on supporting them officially in .NET 10 instead. This is not being done due to a lack of confidence in the direction we're going, but rather is explictly being done to allow time for additional feedback from the community and important libraries in the .NET ecosystem that plan on taking advantage of the provided functionality. It also gives us additional time to coordinate with improvements to the C# language to ensure a great end-to-end experience.

Call to Action

Please try out the new APIs and provide feedback on the overall experience. This includes (but is not limited to) the naming or shape of APIs, any core functionality that appears to be missing or that behaves unexpectedly, and how the general user experience was.

Some additional callouts include:

  • Despite being marked [Experimental] much of the surface is relatively stable and isn't expectied to change, there isn't much you can get wrong for an API like Add(x, y) after all. This isn't a guarantee that we won't change such APIs, but the expected churn for these ones is minimal.
  • There's known missing functionality such as no operator support given that we want Tensor<T> to work over all T but operator + can only be defined for types that implement the generic math IAdditionOperators interface. This requires language support for extension operators to achieve.
  • The TensorSpan<T> types can wrap native allocations, but we don't have a non-ref struct type equivalent. Such a type would need to be disposable and needs additional design consideration around how ownership/lifetime tracking works. If this is an important scenario to you, we'd like to hear about it.
  • The names/concepts used for some APIs can be very inconsistent across the entire machine learning ecosystem (both inside and outside .NET) and in many cases we opted choose the name that was most consistent with existing .NET concepts or behavior. This decision is typically matching the same semantics given to other major tensor libraries, but if there are any quirks or unexpected behaviors found then we'd like to hear about it.

TensorPrimitives is stable and improved

As a final note, the TensorPrimitives class which we shipped in .NET 8 is stable and has been expanded in .NET 9 with additional API surface that is also considered stable. It is the class that contains most of the accelerated algorithms that underpin the Tensor<T> type and so they can still be used to accelerate your code where applicable. There are many potential applications for these algorithms including in machine learning/AI, image processing, games, and beyond.

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@@ -6,6 +6,7 @@

namespace System.Buffers
{
[System.Diagnostics.CodeAnalysis.Experimental("SNTEXP0001")]
Copy link
Member Author

Choose a reason for hiding this comment

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

CC. @terrajobst, I don't think we have an existing format for these. So I've somewhat just followed what some other users (ASP.NET/Azure) are doing here.

Namely, SNT for System.Numerics.Tensors, then EXP for Experimental, then 0001 since this is the first ID.

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 wording we get from this resembles:

SNTEXP0001: 'System.Numerics.Tensors.IReadOnlyTensor<System.Numerics.Tensors.Tensor<T>, T>' is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.

Copy link
Member

@jeffhandley jeffhandley Jul 22, 2024

Choose a reason for hiding this comment

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

@terrajobst Should we instead use SYSLIB5### to follow suit of Obsoletions (SYSLIB0###) and Analyzers (SYSLIB1###) and leave plenty of room for analyzers?

Copy link
Member

@terrajobst terrajobst Jul 22, 2024

Choose a reason for hiding this comment

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

I think we should stick to SYSLIB for all diagnostics (obsoletions, experimental, and analyzers).

My concern with having separate prefixes is that it will be very inconsistent across all the parties. It's hard enough to come up with a prefix that is short enough and identifies a single party that can keep track of the numbers to avoid duplicates. Asking folks to have separate prefixes for obsoletions, experimental, and general purposes analyzers and have that scheme make sense across all parties, is a bit too much IMHO. And frankly it doesn't seem to add much more value to the consumer as the way a given diagnostic needs to be addressed often differs between different cases within the same category anyway. So the expectation is: read the message and follow the link if you need more information.

I don't have strong opinions on whether we should use numeric ranges for the different categories; if having separate ranges makes our lives easier (or is more consistent with what we have done before), then let's do that. If not, I don't think it's worth introducing though.

@tannergooding tannergooding marked this pull request as ready for review July 22, 2024 18:26
@tannergooding
Copy link
Member Author

Copy link
Member

@michaelgsharp michaelgsharp left a comment

Choose a reason for hiding this comment

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

LGTM.

@michaelgsharp
Copy link
Member

Though it does seem to be failing some builds having that annotation there for some reason.

// "aka.ms/dotnet-warnings/{0}" URL points to documentation for the API.
// The diagnostic IDs reserved for experimental APIs are SYSLIB5### (SYSLIB5001 - SYSLIB5999).

// When an API is no longer marked as experimental, the diagnostic ID should be removed from this file
Copy link
Member

Choose a reason for hiding this comment

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

If we're using the markdown file as the source of truth for claiming new diagnostic IDs, what's the need for also consolidating them in source? IIRC we don't do this for other diagnostics.

Copy link
Member

Choose a reason for hiding this comment

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

We do this with Obsoletions as well. I assert that it has helped keep the process discoverable and consistent.

using System.Linq;
using System.Runtime.InteropServices;

namespace System.Numerics.Tensors
{
[Experimental(Experimentals.TensorTDiagId, UrlFormat = Experimentals.SharedUrlFormat)]
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to mark internal types?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's just the way the analyzer currently works. Presumably to ensure that consumers of those internal types are also aware that its using experimental features

@tannergooding tannergooding merged commit 5998f88 into dotnet:main Jul 24, 2024
83 of 85 checks passed
@tannergooding tannergooding deleted the tensors-experimental branch July 24, 2024 01:59
@tannergooding
Copy link
Member Author

/backport to release/9.0-preview7

Copy link
Contributor

Started backporting to release/9.0-preview7: https://github.com/dotnet/runtime/actions/runs/10069224280

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-numerics-tensors
See info in area-owners.md if you want to be subscribed.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants