-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Update Projects to .NET 8 in MLNET 4.0 Branch #6749
base: feature/4.0
Are you sure you want to change the base?
Conversation
@@ -19,7 +19,7 @@ | |||
using System.Runtime.Intrinsics; | |||
using System.Runtime.Intrinsics.X86; | |||
using Microsoft.ML.Internal.CpuMath.Core; | |||
using nuint = System.UInt64; | |||
using nUInt = System.UInt64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on name? It didn't like all lowercase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer lowercase - because this is standard for .net
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait. .net have nuint native type. why nuint = uint64??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right - I'm not sure why this is here. I think the alias should be removed since it's not actually behaving like a native integer.
// Generated by the protocol buffer compiler. DO NOT EDIT! | ||
// source: onnx-ml.proto3 | ||
// </auto-generated> | ||
#pragma warning disable 1591, 0612, 3021 | ||
#region Designer generated code | ||
|
||
#pragma warning disable CS8981 // The type name only contains lower-cased ascii characters. Such names may become reserved for the language. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be moved up to the other disables
buildScript: ./build.sh | ||
container: UbuntuCrossArmContainer | ||
customMatrixes: | ||
Debug_Build: | ||
_configuration: Debug | ||
_config_short: DI | ||
_includeBenchmarkData: false | ||
_targetFramework: net6.0 | ||
_targetFramework: net8.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Try to remove _targetFramework stuff
Codecov Report
@@ Coverage Diff @@
## feature/4.0 #6749 +/- ##
===============================================
+ Coverage 69.64% 69.87% +0.23%
===============================================
Files 1237 1367 +130
Lines 247617 256527 +8910
Branches 25436 26325 +889
===============================================
+ Hits 172446 179247 +6801
- Misses 68561 70376 +1815
- Partials 6610 6904 +294
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ad1243a
to
4157319
Compare
@@ -15,7 +15,7 @@ | |||
using System.Runtime.Intrinsics; | |||
using System.Runtime.Intrinsics.X86; | |||
using Microsoft.ML.Internal.CpuMath.Core; | |||
using nuint = System.UInt64; | |||
using nUInt = System.UInt64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just as soon remove this type alias to avoid confusion. Doesn't seem all that useful since it's not ifdef'ed.
@@ -19,7 +19,7 @@ | |||
using System.Runtime.Intrinsics; | |||
using System.Runtime.Intrinsics.X86; | |||
using Microsoft.ML.Internal.CpuMath.Core; | |||
using nuint = System.UInt64; | |||
using nUInt = System.UInt64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right - I'm not sure why this is here. I think the alias should be removed since it's not actually behaving like a native integer.
} | ||
|
||
if (Directory.Exists(folder)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason you duplicated this check?
@@ -248,9 +248,17 @@ public override string[] Save(string path, string? prefix = null) | |||
internal static (Dictionary<string, int>?, Vec<(string, string)>) ReadFile(string? vocab, string? merges) | |||
{ | |||
Dictionary<string, int>? dic; | |||
using (Stream stream = File.OpenRead(vocab)) | |||
|
|||
if (vocab != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we intend to support null here? Before I bet File.OpenRead would throw. Should we throw ANE instead?
- Disabled Nullable on FuncInstanceMethodInfos - Updated web request to use HTTPClient - Resolved some "WindowsOnly" issues. - Remove .NET Standard only related things
…ing to resolve difference in Enum serialization between .NET 6 and .NET 8.
4157319
to
684fae9
Compare
@@ -2,7 +2,7 @@ | |||
<Import Project="$(RepoRoot)eng/pkg/Pack.props" /> | |||
|
|||
<PropertyGroup> | |||
<TargetFramework>netstandard2.0</TargetFramework> | |||
<TargetFramework>net8.0</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this project we probably want to keep netstandard2.0. We could consider cross-targeting if we want to benefit from net6.0+/net8.0+ APIs.
@JakeRadMSFT / @michaelgsharp -- any reason to keep this one open? |
TODO:
https://github.com/search?q=repo%3Adotnet%2Fruntime%20ApiCompatSuppressionFile&type=code