-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Dependency version updates #7457
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR updates dependency versions and deprecates several test methods by adding [Obsolete] attributes and related warning suppressions. The key changes include:
- Marking diagnostic and test methods as obsolete to indicate deprecation.
- Adding SuppressMessage attributes with pending justifications in one test file.
- Updating a test to be skipped with an accompanying obsolete marker.
Reviewed Changes
Copilot reviewed 28 out of 38 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
test/Microsoft.ML.CodeAnalyzer.Tests/Helpers/CSharpCodeFixVerifier`2.cs | Added [Obsolete] attributes to the Diagnostic() overloads and applied warning suppression around an internal test class. |
test/Microsoft.ML.CodeAnalyzer.Tests/Code/TypeParamNameTest.cs | Added an [Obsolete] attribute on the test method. |
test/Microsoft.ML.CodeAnalyzer.Tests/Code/RelaxTestNamingTest.cs | Introduced multiple SuppressMessage attributes with "" justifications. |
Others | Similar [Obsolete] attributes were added across various test files to mark methods for future deprecation. |
Files not reviewed (10)
- docs/samples/Microsoft.ML.GenAI.Samples/Microsoft.ML.GenAI.Samples.csproj: Language not supported
- docs/samples/Microsoft.ML.Samples/Microsoft.ML.Samples.csproj: Language not supported
- eng/Versions.props: Language not supported
- src/Microsoft.ML.AutoML.Interactive/Microsoft.ML.AutoML.Interactive.csproj: Language not supported
- src/Microsoft.ML.AutoML/Microsoft.ML.AutoML.csproj: Language not supported
- src/Microsoft.ML.GenAI.Core/Microsoft.ML.GenAI.Core.csproj: Language not supported
- src/Microsoft.ML.GenAI.Mistral/Microsoft.ML.GenAI.Mistral.csproj: Language not supported
- src/Microsoft.ML.GenAI.Phi/Microsoft.ML.GenAI.Phi.csproj: Language not supported
- test/Microsoft.ML.GenAI.Core.Tests/Microsoft.ML.GenAI.Core.Tests.csproj: Language not supported
- test/Microsoft.ML.GenAI.LLaMA.Tests/Microsoft.ML.GenAI.LLaMA.Tests.csproj: Language not supported
[System.Obsolete] | ||
public static DiagnosticResult Diagnostic() | ||
=> CSharpCodeFixVerifier<TAnalyzer, TCodeFix, XUnitVerifier>.Diagnostic(); | ||
|
||
[System.Obsolete] | ||
public static DiagnosticResult Diagnostic(string diagnosticId) | ||
=> CSharpCodeFixVerifier<TAnalyzer, TCodeFix, XUnitVerifier>.Diagnostic(diagnosticId); | ||
|
||
[System.Obsolete] |
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.
Consider adding a descriptive message to the Obsolete attribute (e.g., Obsolete("Use Diagnostic() from ... instead")) to clearly communicate the intended replacement or removal plan.
[System.Obsolete] | |
public static DiagnosticResult Diagnostic() | |
=> CSharpCodeFixVerifier<TAnalyzer, TCodeFix, XUnitVerifier>.Diagnostic(); | |
[System.Obsolete] | |
public static DiagnosticResult Diagnostic(string diagnosticId) | |
=> CSharpCodeFixVerifier<TAnalyzer, TCodeFix, XUnitVerifier>.Diagnostic(diagnosticId); | |
[System.Obsolete] | |
[System.Obsolete("Use CSharpCodeFixVerifier<TAnalyzer, TCodeFix, XUnitVerifier>.Diagnostic() instead.")] | |
public static DiagnosticResult Diagnostic() | |
=> CSharpCodeFixVerifier<TAnalyzer, TCodeFix, XUnitVerifier>.Diagnostic(); | |
[System.Obsolete("Use CSharpCodeFixVerifier<TAnalyzer, TCodeFix, XUnitVerifier>.Diagnostic(string diagnosticId) instead.")] | |
public static DiagnosticResult Diagnostic(string diagnosticId) | |
=> CSharpCodeFixVerifier<TAnalyzer, TCodeFix, XUnitVerifier>.Diagnostic(diagnosticId); | |
[System.Obsolete("Use CSharpCodeFixVerifier<TAnalyzer, TCodeFix, XUnitVerifier>.Diagnostic(DiagnosticDescriptor descriptor) instead.")] |
Copilot uses AI. Check for mistakes.
[SuppressMessage("MicrosoftCodeAnalysisCorrectness", "RS1041:Compiler extensions should be implemented in assemblies targeting netstandard2.0", Justification = "<Pending>")] | ||
[SuppressMessage("MicrosoftCodeAnalysisCorrectness", "RS1038:Compiler extensions should be implemented in assemblies with compiler-provided references", Justification = "<Pending>")] | ||
[SuppressMessage("MicrosoftCodeAnalysisCorrectness", "RS1036:Specify analyzer banned API enforcement setting", Justification = "<Pending>")] |
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.
Replace the placeholder '' in the Justification fields with a concrete description detailing why the suppression is necessary.
[SuppressMessage("MicrosoftCodeAnalysisCorrectness", "RS1041:Compiler extensions should be implemented in assemblies targeting netstandard2.0", Justification = "<Pending>")] | |
[SuppressMessage("MicrosoftCodeAnalysisCorrectness", "RS1038:Compiler extensions should be implemented in assemblies with compiler-provided references", Justification = "<Pending>")] | |
[SuppressMessage("MicrosoftCodeAnalysisCorrectness", "RS1036:Specify analyzer banned API enforcement setting", Justification = "<Pending>")] | |
[SuppressMessage("MicrosoftCodeAnalysisCorrectness", "RS1041:Compiler extensions should be implemented in assemblies targeting netstandard2.0", Justification = "This test is not targeting netstandard2.0 because it is designed for a specific testing scenario.")] | |
[SuppressMessage("MicrosoftCodeAnalysisCorrectness", "RS1038:Compiler extensions should be implemented in assemblies with compiler-provided references", Justification = "This test does not require compiler-provided references due to its isolated nature.")] | |
[SuppressMessage("MicrosoftCodeAnalysisCorrectness", "RS1036:Specify analyzer banned API enforcement setting", Justification = "The banned API enforcement setting is not relevant for this test.")] |
Copilot uses AI. Check for mistakes.
@@ -14,6 +15,7 @@ namespace Microsoft.ML.InternalCodeAnalyzer.Tests | |||
public sealed class TypeParamNameTest | |||
{ | |||
[Fact] | |||
[Obsolete] |
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.
Consider adding a message to the Obsolete attribute to specify the deprecation rationale or suggest an alternative, enhancing clarity for developers.
[Obsolete] | |
[Obsolete("This test method is deprecated and is retained for backward compatibility testing purposes.")] |
Copilot uses AI. Check for mistakes.
<LightGBMVersion>4.6.0</LightGBMVersion> | ||
<JsonSchemaNetGenerationVersion>5.0.1</JsonSchemaNetGenerationVersion> | ||
<JsonSchemaNetVersion>7.3.4</JsonSchemaNetVersion> |
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.
What these used for? I mean why did we add these libraries dependency? These looks third party libraries, right?
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.
They were always dependencies of dependencies. I added them in directly here so we could get the latest version and fix the nuget warnings. Would another way be better?
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 am not sure this can cause issue the direct dependency expects lower version of these added references. @ericstj may advise about that
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.
We should use in conjunction with CPM + transitive pinning. Adding all the direct PackageReferences
are unnecessary that's what CPM will do for us - but we do need to list all package versions. @michaelgsharp Is there a reason we couldn't use CPM?
@@ -19,6 +19,7 @@ public sealed class BestFriendOnPublicDeclarationTest | |||
private readonly Lazy<string> _sourceDeclaration = TestUtils.LazySource("BestFriendOnPublicDeclaration.cs"); | |||
|
|||
[Fact] | |||
[Obsolete] | |||
public async Task BestFriendOnPublicDeclaration() |
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.
why we are obsoleting things in the tests? this is first time I see that :-)
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.
We are going to be removing most, if not all, of our internal analyzers for ML.NET. They can pretty much all be handled by the formatting tools built into runtime now.
The problem is that the version update made a bunch of our analyzers use deprecated code, but because we are going to be removing them it's not worth the time to go update all the code itself, so I just added the obsolete tag to fix the error. Is there a better way to do that? I just figured removing the analyzers in this same PR would make this PR blow up in size and so I was trying to split things off.
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.
Can't we suppress the analyzer warnings instead of adding obsolete?
@@ -17,12 +17,15 @@ internal static class CSharpCodeFixVerifier<TAnalyzer, TCodeFix> | |||
where TAnalyzer : DiagnosticAnalyzer, new() | |||
where TCodeFix : CodeFixProvider, new() | |||
{ | |||
[System.Obsolete] | |||
public static DiagnosticResult Diagnostic() |
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.
nit: add using System
above?
@@ -15,6 +15,7 @@ | |||
namespace Microsoft.ML.AutoML.SourceGenerator | |||
{ | |||
[Generator] | |||
[System.Diagnostics.CodeAnalysis.SuppressMessage("MicrosoftCodeAnalysisCompatibility", "RS1042:Implementations of this interface are not allowed", Justification = "<Pending>")] | |||
public sealed class EstimatorTypeGenerator : ISourceGenerator |
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.
What Justification = "<Pending>"
means?
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.
Yeah, Copilot called that out too. I'm getting something added in shortly.
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.
Write my name there 😄
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7457 +/- ##
==========================================
- Coverage 68.99% 68.98% -0.01%
==========================================
Files 1482 1482
Lines 273879 273879
Branches 28254 28254
==========================================
- Hits 188965 188944 -21
- Misses 77531 77547 +16
- Partials 7383 7388 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Update most dependency versions.