API Compat tool in ML.NET

[artidoro]

Fixes#3602.

We need to ensure that future changes to ML.NET will not break the stable API released in 1.0.0.

This PR introduces the API Compat tool from dotnet/Arcade. The API Compat tool runs as part of the build process and compares the assemblies with those found in the stable nugets referenced in the Microsoft.ML.StableAPI project.

The tool is only run for the assemblies which will be part of the stable nugets. Here is a list of those assemblies and the relative nugets:

Stable Nuget	Stable Assemblies
Microsoft.ML	Microsoft.ML.Core, Microsoft.ML.Data, Microsoft.ML.KMeansClustering, Microsoft.ML.PCA, Microsoft.ML.StandardTrainers, MIcrosoft.ML.Transforms, Microsoft.ML.Analyzer
Microsoft.ML.DataView	Microsoft.ML.DataView
Microsoft.ML.CpuMath	Microsoft.ML.CpuMath
Microsoft.ML.FastTree	Microsoft.ML.FastTree
Microsoft.ML.LightGbm	Microsoft.ML.LightGbm
Microsoft.ML.ImageAnalytics	Microsoft.ML.ImageAnalytics
Microsoft.ML.MklComponents	Microsoft.ML.Mkl.Components

Note: the tool does not run on Microsoft.ML.Analyzer as it does not have a public API, so no need to check backwards compatibility.

Still to do:

• Fix possible bug in API Compat on handling of Attributes.
• Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes. Not needed see comments below.
• Update the version of the ML.NET nugets to 1.0.0 when available

[ericstj]

Sorry if some of these were issues with my initial sample.

[ericstj]

Consider moving these to a seperate targets file if that is a convention you'd like to follow in this repo. #Pending

[artidoro]

I think we only have one for the src repo. But let me know if there is a better way.

---

In reply to: 279938766 [](ancestors = 279938766)

[ericstj]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version. #Resolved

[eerhardt]

(nit) you can use the latest 1.0.0-preview build: 1.0.0-preview-27630-5. We just spun it and will have the official 1.0.0 up soon. #Resolved

[artidoro]

Will update to the new build and change to 1.0.0 as soon as it is available.

---

In reply to: 279941849 [](ancestors = 279941849)

[eerhardt]

It's available on myget now. You should be able to use 1.0.0. #Resolved

[ericstj]

D:\a\1\s\packages\microsoft.dotnet.apicompat\1.0.0-beta.19225.5\build\Microsoft.DotNet.ApiCompat.targets(72,5): error : CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.ML.ExtensionBaseAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation. [D:\a\1\s\src\Microsoft.ML.Core\Microsoft.ML.Core.csproj]

This is happening because APICompat is not resolving the System.AttributeTargets enum from the contract. It's not resolving because you aren't providing the dependencies. Today these can be passed in via the $(ContractOutputPath) property but that isn't very sensible. I'll submit a change to APICompat to make that easier.
https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L46-L52 #Pending

[artidoro]

That would be great! Thank you

---

In reply to: 488135991 [](ancestors = 488135991)

[artidoro]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version.

@ericstj is it possible to specify the path for the generated file?
Also, how would we specify known diffs going forward? I would like to write some documentation on how to do that. #Resolved

[ericstj]

by default the generated file gets written to the same directory as the project with the name, but you can customize it: https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L22-L23

how would we specify known diffs going forward?

You can use the same option I mentioned when building individual projects, you can also copy the errors and paste them into the baseline file. I want to stress that these aren't "known diffs" these are compatibility bugs that break your customers and you really shouldn't be baselining them or introducing the suppressions. #Resolved

[ericstj]

To provide paths to contract dependencies, add the following in your GetContract target:

 <_allReferenceDirectories Include="%(ReferencePath.RootDir)%(ReferencePath.Directory)" />
<_contractReferencePath Include="@(ReferencePath)" Condition="'%(FileName)' == '$(ContractName)'" />
<_contractReferencePath DependencyPaths="@(_allReferenceDirectories)" />

Then in ResolveMatchingContract add the following after you get the ResolvedMatchingContract item:

<PropertyGroup>
<ContractOutputPath>%(ResolvedMatchingContract.DependencyPaths)</ContractOutputPath>
</PropertyGroup>

I'm putting a feature into the APICompat targets that will make the latter property unnecessary. #Resolved

[eerhardt]

This project will actually build a .dll when we build the .sln file. That seems unnecessary. Maybe we should override the Build target or maybe not name it .csproj and instead just .proj? It's mission in life isn't to build .cs files into a .dll, but instead pull down external packages and provide the GetContract target.

[artidoro]

I tried renaming, but that did not work (it gave an error saying that project.assets.json was not generated.
I have also tried to overwrite the Build target, but I must have made some mistake since I still found the generated .dll.

What I did was adding:

 <Target Name="Build">
<!-- This will override the default Build target. -->
</Target>

to the .csproj file.

What is the correct way to overwrite it?

---

In reply to: 280105436 [](ancestors = 280105436)

[eerhardt]

Yes, I think that is fine.

[artidoro]

Thank you that seems to make it work!

---

In reply to: 488287870 [](ancestors = 488287870)

[codecov]

Codecov Report

Merging #3623 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #3623 +/- ##
==========================================
+ Coverage 72.78% 72.78% +<.01% 
==========================================
Files 808 808 Lines 145588 145588 Branches 16250 16250 ==========================================
+ Hits 105960 105968 +8 + Misses 35205 35198 -7 + Partials 4423 4422 -1

Flag	Coverage Δ
#Debug	72.78% <ø> (ø)	⬆️
#production	68.28% <ø> (ø)	⬆️
#test	89.04% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
...StandardTrainers/Standard/LinearModelParameters.cs	60.05% <0%> (-0.27%)	⬇️
...icrosoft.ML.TensorFlow/TensorFlow/TensorGeneric.cs	44.21% <0%> (ø)	⬆️
...c/Microsoft.ML.FastTree/Utils/BufferPoolManager.cs	0% <0%> (ø)	⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	86.26% <0%> (+0.15%)	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	84.9% <0%> (+0.2%)	⬆️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs	73.93% <0%> (+0.32%)	⬆️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs	89.89% <0%> (+0.62%)	⬆️

[ericstj]

failing on linux because dotnet is not on the path and the $(ToolHostCmd) is not set to point to it.

[artidoro]

Thank you @ericstj and @eerhardt for looking at the PR, I have addressed your comments.
Let me know if there are other things I should look into!

[ericstj]

Cool, this is looking pretty good. You may want to wait for my changes dotnet/arcade#2672 which help simplify some of this.

[artidoro]

I would prefer checking in this change as soon as possible and updating it with the new buyer as soon as it is available.

Since we have released yesterday, it would be great to have the API Compat tool in the repo.

[eerhardt]

You may want to revert these two lines. I don't know what happens if someone tries opening the solution with VS 2017 (which is version 15) and this .sln says it is for VS 2019.

[eerhardt]

I think this looks good. But I'd wait for @ericstj to give the thumbs up as he's the expert here.

[ericstj]

Why should we have the condition here? Is there any harm in always resolving for both TFMs?

[eerhardt]

Usually netcoreapp3.0 causes problems when you are building in VS 2017 where the SDK throws an error saying I don't support netcoreapp3.0.

[ericstj]

I see, makes sense.

[ericstj]

This could be improved by changing the project extension and defining a couple targets. We do that elsewhere:
dotnet/project-system#4647

IOW: call this a .proj, or a .restoreproj or something. SLN entry looks the same (you have to do it manually in text editor, IDE won't let you). Add the workaround I linked, define stub targets for anything that fails, and it should avoid
the confusion around a CSProj that doesn't build anything.
You don't need to do that now, but I think it would make this more sensible.

[artidoro]

I have tried to add the lines found in the issue to the new .restoreproj file but that did not work.
I will keep it as is for now then.

[ericstj]

Have you made sure you see a failure when making a breaking API change?

[artidoro]

Yes I have done a few tests where I rename methods and such and the build was failing.

[RussKie]

Lines 24 and 26 are identical... 😕

[eerhardt]

@RussKie - can you log an issue (or even submit a PR for the fix)?

[artidoro]

Fixes#3602.

We need to ensure that future changes to ML.NET will not break the stable API released in 1.0.0.

This PR introduces the API Compat tool from dotnet/Arcade. The API Compat tool runs as part of the build process and compares the assemblies with those found in the stable nugets referenced in the Microsoft.ML.StableAPI project.

The tool is only run for the assemblies which will be part of the stable nugets. Here is a list of those assemblies and the relative nugets:

Stable Nuget	Stable Assemblies
Microsoft.ML	Microsoft.ML.Core, Microsoft.ML.Data, Microsoft.ML.KMeansClustering, Microsoft.ML.PCA, Microsoft.ML.StandardTrainers, MIcrosoft.ML.Transforms, Microsoft.ML.Analyzer
Microsoft.ML.DataView	Microsoft.ML.DataView
Microsoft.ML.CpuMath	Microsoft.ML.CpuMath
Microsoft.ML.FastTree	Microsoft.ML.FastTree
Microsoft.ML.LightGbm	Microsoft.ML.LightGbm
Microsoft.ML.ImageAnalytics	Microsoft.ML.ImageAnalytics
Microsoft.ML.MklComponents	Microsoft.ML.Mkl.Components

Note: the tool does not run on Microsoft.ML.Analyzer as it does not have a public API, so no need to check backwards compatibility.

Still to do:

• Fix possible bug in API Compat on handling of Attributes.
• Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes. Not needed see comments below.
• Update the version of the ML.NET nugets to 1.0.0 when available

[ericstj]

Sorry if some of these were issues with my initial sample.

[ericstj]

Consider moving these to a seperate targets file if that is a convention you'd like to follow in this repo. #Pending

[artidoro]

I think we only have one for the src repo. But let me know if there is a better way.

---

In reply to: 279938766 [](ancestors = 279938766)

[ericstj]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version. #Resolved

[eerhardt]

(nit) you can use the latest 1.0.0-preview build: 1.0.0-preview-27630-5. We just spun it and will have the official 1.0.0 up soon. #Resolved

[artidoro]

Will update to the new build and change to 1.0.0 as soon as it is available.

---

In reply to: 279941849 [](ancestors = 279941849)

[eerhardt]

It's available on myget now. You should be able to use 1.0.0. #Resolved

[ericstj]

D:\a\1\s\packages\microsoft.dotnet.apicompat\1.0.0-beta.19225.5\build\Microsoft.DotNet.ApiCompat.targets(72,5): error : CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.ML.ExtensionBaseAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation. [D:\a\1\s\src\Microsoft.ML.Core\Microsoft.ML.Core.csproj]

This is happening because APICompat is not resolving the System.AttributeTargets enum from the contract. It's not resolving because you aren't providing the dependencies. Today these can be passed in via the $(ContractOutputPath) property but that isn't very sensible. I'll submit a change to APICompat to make that easier.
https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L46-L52 #Pending

[artidoro]

That would be great! Thank you

---

In reply to: 488135991 [](ancestors = 488135991)

[artidoro]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version.

@ericstj is it possible to specify the path for the generated file?
Also, how would we specify known diffs going forward? I would like to write some documentation on how to do that. #Resolved

[ericstj]

by default the generated file gets written to the same directory as the project with the name, but you can customize it: https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L22-L23

how would we specify known diffs going forward?

You can use the same option I mentioned when building individual projects, you can also copy the errors and paste them into the baseline file. I want to stress that these aren't "known diffs" these are compatibility bugs that break your customers and you really shouldn't be baselining them or introducing the suppressions. #Resolved

[ericstj]

To provide paths to contract dependencies, add the following in your GetContract target:

 <_allReferenceDirectories Include="%(ReferencePath.RootDir)%(ReferencePath.Directory)" />
<_contractReferencePath Include="@(ReferencePath)" Condition="'%(FileName)' == '$(ContractName)'" />
<_contractReferencePath DependencyPaths="@(_allReferenceDirectories)" />

Then in ResolveMatchingContract add the following after you get the ResolvedMatchingContract item:

<PropertyGroup>
<ContractOutputPath>%(ResolvedMatchingContract.DependencyPaths)</ContractOutputPath>
</PropertyGroup>

I'm putting a feature into the APICompat targets that will make the latter property unnecessary. #Resolved

[eerhardt]

This project will actually build a .dll when we build the .sln file. That seems unnecessary. Maybe we should override the Build target or maybe not name it .csproj and instead just .proj? It's mission in life isn't to build .cs files into a .dll, but instead pull down external packages and provide the GetContract target.

[artidoro]

I tried renaming, but that did not work (it gave an error saying that project.assets.json was not generated.
I have also tried to overwrite the Build target, but I must have made some mistake since I still found the generated .dll.

What I did was adding:

 <Target Name="Build">
<!-- This will override the default Build target. -->
</Target>

to the .csproj file.

What is the correct way to overwrite it?

---

In reply to: 280105436 [](ancestors = 280105436)

[eerhardt]

Yes, I think that is fine.

[artidoro]

Thank you that seems to make it work!

---

In reply to: 488287870 [](ancestors = 488287870)

[codecov]

Codecov Report

Merging #3623 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #3623 +/- ##
==========================================
+ Coverage 72.78% 72.78% +<.01% 
==========================================
Files 808 808 Lines 145588 145588 Branches 16250 16250 ==========================================
+ Hits 105960 105968 +8 + Misses 35205 35198 -7 + Partials 4423 4422 -1

Flag	Coverage Δ
#Debug	72.78% <ø> (ø)	⬆️
#production	68.28% <ø> (ø)	⬆️
#test	89.04% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
...StandardTrainers/Standard/LinearModelParameters.cs	60.05% <0%> (-0.27%)	⬇️
...icrosoft.ML.TensorFlow/TensorFlow/TensorGeneric.cs	44.21% <0%> (ø)	⬆️
...c/Microsoft.ML.FastTree/Utils/BufferPoolManager.cs	0% <0%> (ø)	⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	86.26% <0%> (+0.15%)	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	84.9% <0%> (+0.2%)	⬆️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs	73.93% <0%> (+0.32%)	⬆️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs	89.89% <0%> (+0.62%)	⬆️

[ericstj]

failing on linux because dotnet is not on the path and the $(ToolHostCmd) is not set to point to it.

[artidoro]

Thank you @ericstj and @eerhardt for looking at the PR, I have addressed your comments.
Let me know if there are other things I should look into!

[ericstj]

Cool, this is looking pretty good. You may want to wait for my changes dotnet/arcade#2672 which help simplify some of this.

[artidoro]

I would prefer checking in this change as soon as possible and updating it with the new buyer as soon as it is available.

Since we have released yesterday, it would be great to have the API Compat tool in the repo.

[eerhardt]

You may want to revert these two lines. I don't know what happens if someone tries opening the solution with VS 2017 (which is version 15) and this .sln says it is for VS 2019.

[eerhardt]

I think this looks good. But I'd wait for @ericstj to give the thumbs up as he's the expert here.

[ericstj]

Why should we have the condition here? Is there any harm in always resolving for both TFMs?

[eerhardt]

Usually netcoreapp3.0 causes problems when you are building in VS 2017 where the SDK throws an error saying I don't support netcoreapp3.0.

[ericstj]

I see, makes sense.

[ericstj]

This could be improved by changing the project extension and defining a couple targets. We do that elsewhere:
dotnet/project-system#4647

IOW: call this a .proj, or a .restoreproj or something. SLN entry looks the same (you have to do it manually in text editor, IDE won't let you). Add the workaround I linked, define stub targets for anything that fails, and it should avoid
the confusion around a CSProj that doesn't build anything.
You don't need to do that now, but I think it would make this more sensible.

[artidoro]

I have tried to add the lines found in the issue to the new .restoreproj file but that did not work.
I will keep it as is for now then.

[ericstj]

Have you made sure you see a failure when making a breaking API change?

[artidoro]

Yes I have done a few tests where I rename methods and such and the build was failing.

[RussKie]

Lines 24 and 26 are identical... 😕

[eerhardt]

@RussKie - can you log an issue (or even submit a PR for the fix)?

[artidoro]

Fixes#3602.

We need to ensure that future changes to ML.NET will not break the stable API released in 1.0.0.

This PR introduces the API Compat tool from dotnet/Arcade. The API Compat tool runs as part of the build process and compares the assemblies with those found in the stable nugets referenced in the Microsoft.ML.StableAPI project.

The tool is only run for the assemblies which will be part of the stable nugets. Here is a list of those assemblies and the relative nugets:

Stable Nuget	Stable Assemblies
Microsoft.ML	Microsoft.ML.Core, Microsoft.ML.Data, Microsoft.ML.KMeansClustering, Microsoft.ML.PCA, Microsoft.ML.StandardTrainers, MIcrosoft.ML.Transforms, Microsoft.ML.Analyzer
Microsoft.ML.DataView	Microsoft.ML.DataView
Microsoft.ML.CpuMath	Microsoft.ML.CpuMath
Microsoft.ML.FastTree	Microsoft.ML.FastTree
Microsoft.ML.LightGbm	Microsoft.ML.LightGbm
Microsoft.ML.ImageAnalytics	Microsoft.ML.ImageAnalytics
Microsoft.ML.MklComponents	Microsoft.ML.Mkl.Components

Note: the tool does not run on Microsoft.ML.Analyzer as it does not have a public API, so no need to check backwards compatibility.

Still to do:

• Fix possible bug in API Compat on handling of Attributes.
• Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes. Not needed see comments below.
• Update the version of the ML.NET nugets to 1.0.0 when available

[ericstj]

Sorry if some of these were issues with my initial sample.

[ericstj]

Consider moving these to a seperate targets file if that is a convention you'd like to follow in this repo. #Pending

[artidoro]

I think we only have one for the src repo. But let me know if there is a better way.

---

In reply to: 279938766 [](ancestors = 279938766)

[ericstj]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version. #Resolved

[eerhardt]

(nit) you can use the latest 1.0.0-preview build: 1.0.0-preview-27630-5. We just spun it and will have the official 1.0.0 up soon. #Resolved

[artidoro]

Will update to the new build and change to 1.0.0 as soon as it is available.

---

In reply to: 279941849 [](ancestors = 279941849)

[eerhardt]

It's available on myget now. You should be able to use 1.0.0. #Resolved

[ericstj]

D:\a\1\s\packages\microsoft.dotnet.apicompat\1.0.0-beta.19225.5\build\Microsoft.DotNet.ApiCompat.targets(72,5): error : CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.ML.ExtensionBaseAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation. [D:\a\1\s\src\Microsoft.ML.Core\Microsoft.ML.Core.csproj]

This is happening because APICompat is not resolving the System.AttributeTargets enum from the contract. It's not resolving because you aren't providing the dependencies. Today these can be passed in via the $(ContractOutputPath) property but that isn't very sensible. I'll submit a change to APICompat to make that easier.
https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L46-L52 #Pending

[artidoro]

That would be great! Thank you

---

In reply to: 488135991 [](ancestors = 488135991)

[artidoro]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version.

@ericstj is it possible to specify the path for the generated file?
Also, how would we specify known diffs going forward? I would like to write some documentation on how to do that. #Resolved

[ericstj]

by default the generated file gets written to the same directory as the project with the name, but you can customize it: https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L22-L23

how would we specify known diffs going forward?

You can use the same option I mentioned when building individual projects, you can also copy the errors and paste them into the baseline file. I want to stress that these aren't "known diffs" these are compatibility bugs that break your customers and you really shouldn't be baselining them or introducing the suppressions. #Resolved

[ericstj]

To provide paths to contract dependencies, add the following in your GetContract target:

 <_allReferenceDirectories Include="%(ReferencePath.RootDir)%(ReferencePath.Directory)" />
<_contractReferencePath Include="@(ReferencePath)" Condition="'%(FileName)' == '$(ContractName)'" />
<_contractReferencePath DependencyPaths="@(_allReferenceDirectories)" />

Then in ResolveMatchingContract add the following after you get the ResolvedMatchingContract item:

<PropertyGroup>
<ContractOutputPath>%(ResolvedMatchingContract.DependencyPaths)</ContractOutputPath>
</PropertyGroup>

I'm putting a feature into the APICompat targets that will make the latter property unnecessary. #Resolved

[eerhardt]

This project will actually build a .dll when we build the .sln file. That seems unnecessary. Maybe we should override the Build target or maybe not name it .csproj and instead just .proj? It's mission in life isn't to build .cs files into a .dll, but instead pull down external packages and provide the GetContract target.

[artidoro]

I tried renaming, but that did not work (it gave an error saying that project.assets.json was not generated.
I have also tried to overwrite the Build target, but I must have made some mistake since I still found the generated .dll.

What I did was adding:

 <Target Name="Build">
<!-- This will override the default Build target. -->
</Target>

to the .csproj file.

What is the correct way to overwrite it?

---

In reply to: 280105436 [](ancestors = 280105436)

[eerhardt]

Yes, I think that is fine.

[artidoro]

Thank you that seems to make it work!

---

In reply to: 488287870 [](ancestors = 488287870)

[codecov]

Codecov Report

Merging #3623 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #3623 +/- ##
==========================================
+ Coverage 72.78% 72.78% +<.01% 
==========================================
Files 808 808 Lines 145588 145588 Branches 16250 16250 ==========================================
+ Hits 105960 105968 +8 + Misses 35205 35198 -7 + Partials 4423 4422 -1

Flag	Coverage Δ
#Debug	72.78% <ø> (ø)	⬆️
#production	68.28% <ø> (ø)	⬆️
#test	89.04% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
...StandardTrainers/Standard/LinearModelParameters.cs	60.05% <0%> (-0.27%)	⬇️
...icrosoft.ML.TensorFlow/TensorFlow/TensorGeneric.cs	44.21% <0%> (ø)	⬆️
...c/Microsoft.ML.FastTree/Utils/BufferPoolManager.cs	0% <0%> (ø)	⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	86.26% <0%> (+0.15%)	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	84.9% <0%> (+0.2%)	⬆️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs	73.93% <0%> (+0.32%)	⬆️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs	89.89% <0%> (+0.62%)	⬆️

[ericstj]

failing on linux because dotnet is not on the path and the $(ToolHostCmd) is not set to point to it.

[artidoro]

Thank you @ericstj and @eerhardt for looking at the PR, I have addressed your comments.
Let me know if there are other things I should look into!

[ericstj]

Cool, this is looking pretty good. You may want to wait for my changes dotnet/arcade#2672 which help simplify some of this.

[artidoro]

I would prefer checking in this change as soon as possible and updating it with the new buyer as soon as it is available.

Since we have released yesterday, it would be great to have the API Compat tool in the repo.

[eerhardt]

You may want to revert these two lines. I don't know what happens if someone tries opening the solution with VS 2017 (which is version 15) and this .sln says it is for VS 2019.

[eerhardt]

I think this looks good. But I'd wait for @ericstj to give the thumbs up as he's the expert here.

[ericstj]

Why should we have the condition here? Is there any harm in always resolving for both TFMs?

[eerhardt]

Usually netcoreapp3.0 causes problems when you are building in VS 2017 where the SDK throws an error saying I don't support netcoreapp3.0.

[ericstj]

I see, makes sense.

[ericstj]

This could be improved by changing the project extension and defining a couple targets. We do that elsewhere:
dotnet/project-system#4647

IOW: call this a .proj, or a .restoreproj or something. SLN entry looks the same (you have to do it manually in text editor, IDE won't let you). Add the workaround I linked, define stub targets for anything that fails, and it should avoid
the confusion around a CSProj that doesn't build anything.
You don't need to do that now, but I think it would make this more sensible.

[artidoro]

I have tried to add the lines found in the issue to the new .restoreproj file but that did not work.
I will keep it as is for now then.

[ericstj]

Have you made sure you see a failure when making a breaking API change?

[artidoro]

Yes I have done a few tests where I rename methods and such and the build was failing.

[RussKie]

Lines 24 and 26 are identical... 😕

[eerhardt]

@RussKie - can you log an issue (or even submit a PR for the fix)?

[artidoro]

Fixes#3602.

We need to ensure that future changes to ML.NET will not break the stable API released in 1.0.0.

This PR introduces the API Compat tool from dotnet/Arcade. The API Compat tool runs as part of the build process and compares the assemblies with those found in the stable nugets referenced in the Microsoft.ML.StableAPI project.

The tool is only run for the assemblies which will be part of the stable nugets. Here is a list of those assemblies and the relative nugets:

Stable Nuget	Stable Assemblies
Microsoft.ML	Microsoft.ML.Core, Microsoft.ML.Data, Microsoft.ML.KMeansClustering, Microsoft.ML.PCA, Microsoft.ML.StandardTrainers, MIcrosoft.ML.Transforms, Microsoft.ML.Analyzer
Microsoft.ML.DataView	Microsoft.ML.DataView
Microsoft.ML.CpuMath	Microsoft.ML.CpuMath
Microsoft.ML.FastTree	Microsoft.ML.FastTree
Microsoft.ML.LightGbm	Microsoft.ML.LightGbm
Microsoft.ML.ImageAnalytics	Microsoft.ML.ImageAnalytics
Microsoft.ML.MklComponents	Microsoft.ML.Mkl.Components

Note: the tool does not run on Microsoft.ML.Analyzer as it does not have a public API, so no need to check backwards compatibility.

Still to do:

• Fix possible bug in API Compat on handling of Attributes.
• Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes. Not needed see comments below.
• Update the version of the ML.NET nugets to 1.0.0 when available

[ericstj]

Sorry if some of these were issues with my initial sample.

[ericstj]

Consider moving these to a seperate targets file if that is a convention you'd like to follow in this repo. #Pending

[artidoro]

I think we only have one for the src repo. But let me know if there is a better way.

---

In reply to: 279938766 [](ancestors = 279938766)

[ericstj]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version. #Resolved

[eerhardt]

(nit) you can use the latest 1.0.0-preview build: 1.0.0-preview-27630-5. We just spun it and will have the official 1.0.0 up soon. #Resolved

[artidoro]

Will update to the new build and change to 1.0.0 as soon as it is available.

---

In reply to: 279941849 [](ancestors = 279941849)

[eerhardt]

It's available on myget now. You should be able to use 1.0.0. #Resolved

[ericstj]

D:\a\1\s\packages\microsoft.dotnet.apicompat\1.0.0-beta.19225.5\build\Microsoft.DotNet.ApiCompat.targets(72,5): error : CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.ML.ExtensionBaseAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation. [D:\a\1\s\src\Microsoft.ML.Core\Microsoft.ML.Core.csproj]

This is happening because APICompat is not resolving the System.AttributeTargets enum from the contract. It's not resolving because you aren't providing the dependencies. Today these can be passed in via the $(ContractOutputPath) property but that isn't very sensible. I'll submit a change to APICompat to make that easier.
https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L46-L52 #Pending

[artidoro]

That would be great! Thank you

---

In reply to: 488135991 [](ancestors = 488135991)

[artidoro]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version.

@ericstj is it possible to specify the path for the generated file?
Also, how would we specify known diffs going forward? I would like to write some documentation on how to do that. #Resolved

[ericstj]

by default the generated file gets written to the same directory as the project with the name, but you can customize it: https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L22-L23

how would we specify known diffs going forward?

You can use the same option I mentioned when building individual projects, you can also copy the errors and paste them into the baseline file. I want to stress that these aren't "known diffs" these are compatibility bugs that break your customers and you really shouldn't be baselining them or introducing the suppressions. #Resolved

[ericstj]

To provide paths to contract dependencies, add the following in your GetContract target:

 <_allReferenceDirectories Include="%(ReferencePath.RootDir)%(ReferencePath.Directory)" />
<_contractReferencePath Include="@(ReferencePath)" Condition="'%(FileName)' == '$(ContractName)'" />
<_contractReferencePath DependencyPaths="@(_allReferenceDirectories)" />

Then in ResolveMatchingContract add the following after you get the ResolvedMatchingContract item:

<PropertyGroup>
<ContractOutputPath>%(ResolvedMatchingContract.DependencyPaths)</ContractOutputPath>
</PropertyGroup>

I'm putting a feature into the APICompat targets that will make the latter property unnecessary. #Resolved

[eerhardt]

This project will actually build a .dll when we build the .sln file. That seems unnecessary. Maybe we should override the Build target or maybe not name it .csproj and instead just .proj? It's mission in life isn't to build .cs files into a .dll, but instead pull down external packages and provide the GetContract target.

[artidoro]

I tried renaming, but that did not work (it gave an error saying that project.assets.json was not generated.
I have also tried to overwrite the Build target, but I must have made some mistake since I still found the generated .dll.

What I did was adding:

 <Target Name="Build">
<!-- This will override the default Build target. -->
</Target>

to the .csproj file.

What is the correct way to overwrite it?

---

In reply to: 280105436 [](ancestors = 280105436)

[eerhardt]

Yes, I think that is fine.

[artidoro]

Thank you that seems to make it work!

---

In reply to: 488287870 [](ancestors = 488287870)

[codecov]

Codecov Report

Merging #3623 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #3623 +/- ##
==========================================
+ Coverage 72.78% 72.78% +<.01% 
==========================================
Files 808 808 Lines 145588 145588 Branches 16250 16250 ==========================================
+ Hits 105960 105968 +8 + Misses 35205 35198 -7 + Partials 4423 4422 -1

Flag	Coverage Δ
#Debug	72.78% <ø> (ø)	⬆️
#production	68.28% <ø> (ø)	⬆️
#test	89.04% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
...StandardTrainers/Standard/LinearModelParameters.cs	60.05% <0%> (-0.27%)	⬇️
...icrosoft.ML.TensorFlow/TensorFlow/TensorGeneric.cs	44.21% <0%> (ø)	⬆️
...c/Microsoft.ML.FastTree/Utils/BufferPoolManager.cs	0% <0%> (ø)	⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	86.26% <0%> (+0.15%)	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	84.9% <0%> (+0.2%)	⬆️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs	73.93% <0%> (+0.32%)	⬆️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs	89.89% <0%> (+0.62%)	⬆️

[ericstj]

failing on linux because dotnet is not on the path and the $(ToolHostCmd) is not set to point to it.

[artidoro]

Thank you @ericstj and @eerhardt for looking at the PR, I have addressed your comments.
Let me know if there are other things I should look into!

[ericstj]

Cool, this is looking pretty good. You may want to wait for my changes dotnet/arcade#2672 which help simplify some of this.

[artidoro]

I would prefer checking in this change as soon as possible and updating it with the new buyer as soon as it is available.

Since we have released yesterday, it would be great to have the API Compat tool in the repo.

[eerhardt]

You may want to revert these two lines. I don't know what happens if someone tries opening the solution with VS 2017 (which is version 15) and this .sln says it is for VS 2019.

[eerhardt]

I think this looks good. But I'd wait for @ericstj to give the thumbs up as he's the expert here.

[ericstj]

Why should we have the condition here? Is there any harm in always resolving for both TFMs?

[eerhardt]

Usually netcoreapp3.0 causes problems when you are building in VS 2017 where the SDK throws an error saying I don't support netcoreapp3.0.

[ericstj]

I see, makes sense.

[ericstj]

This could be improved by changing the project extension and defining a couple targets. We do that elsewhere:
dotnet/project-system#4647

IOW: call this a .proj, or a .restoreproj or something. SLN entry looks the same (you have to do it manually in text editor, IDE won't let you). Add the workaround I linked, define stub targets for anything that fails, and it should avoid
the confusion around a CSProj that doesn't build anything.
You don't need to do that now, but I think it would make this more sensible.

[artidoro]

I have tried to add the lines found in the issue to the new .restoreproj file but that did not work.
I will keep it as is for now then.

[ericstj]

Have you made sure you see a failure when making a breaking API change?

[artidoro]

Yes I have done a few tests where I rename methods and such and the build was failing.

[RussKie]

Lines 24 and 26 are identical... 😕

[eerhardt]

@RussKie - can you log an issue (or even submit a PR for the fix)?

[artidoro]

Fixes#3602.

We need to ensure that future changes to ML.NET will not break the stable API released in 1.0.0.

This PR introduces the API Compat tool from dotnet/Arcade. The API Compat tool runs as part of the build process and compares the assemblies with those found in the stable nugets referenced in the Microsoft.ML.StableAPI project.

The tool is only run for the assemblies which will be part of the stable nugets. Here is a list of those assemblies and the relative nugets:

Stable Nuget	Stable Assemblies
Microsoft.ML	Microsoft.ML.Core, Microsoft.ML.Data, Microsoft.ML.KMeansClustering, Microsoft.ML.PCA, Microsoft.ML.StandardTrainers, MIcrosoft.ML.Transforms, Microsoft.ML.Analyzer
Microsoft.ML.DataView	Microsoft.ML.DataView
Microsoft.ML.CpuMath	Microsoft.ML.CpuMath
Microsoft.ML.FastTree	Microsoft.ML.FastTree
Microsoft.ML.LightGbm	Microsoft.ML.LightGbm
Microsoft.ML.ImageAnalytics	Microsoft.ML.ImageAnalytics
Microsoft.ML.MklComponents	Microsoft.ML.Mkl.Components

Note: the tool does not run on Microsoft.ML.Analyzer as it does not have a public API, so no need to check backwards compatibility.

Still to do:

• Fix possible bug in API Compat on handling of Attributes.
• Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes. Not needed see comments below.
• Update the version of the ML.NET nugets to 1.0.0 when available

[ericstj]

Sorry if some of these were issues with my initial sample.

[ericstj]

Consider moving these to a seperate targets file if that is a convention you'd like to follow in this repo. #Pending

[artidoro]

I think we only have one for the src repo. But let me know if there is a better way.

---

In reply to: 279938766 [](ancestors = 279938766)

[ericstj]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version. #Resolved

[eerhardt]

(nit) you can use the latest 1.0.0-preview build: 1.0.0-preview-27630-5. We just spun it and will have the official 1.0.0 up soon. #Resolved

[artidoro]

Will update to the new build and change to 1.0.0 as soon as it is available.

---

In reply to: 279941849 [](ancestors = 279941849)

[eerhardt]

It's available on myget now. You should be able to use 1.0.0. #Resolved

[ericstj]

D:\a\1\s\packages\microsoft.dotnet.apicompat\1.0.0-beta.19225.5\build\Microsoft.DotNet.ApiCompat.targets(72,5): error : CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.ML.ExtensionBaseAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation. [D:\a\1\s\src\Microsoft.ML.Core\Microsoft.ML.Core.csproj]

This is happening because APICompat is not resolving the System.AttributeTargets enum from the contract. It's not resolving because you aren't providing the dependencies. Today these can be passed in via the $(ContractOutputPath) property but that isn't very sensible. I'll submit a change to APICompat to make that easier.
https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L46-L52 #Pending

[artidoro]

That would be great! Thank you

---

In reply to: 488135991 [](ancestors = 488135991)

[artidoro]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version.

@ericstj is it possible to specify the path for the generated file?
Also, how would we specify known diffs going forward? I would like to write some documentation on how to do that. #Resolved

[ericstj]

by default the generated file gets written to the same directory as the project with the name, but you can customize it: https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L22-L23

how would we specify known diffs going forward?

You can use the same option I mentioned when building individual projects, you can also copy the errors and paste them into the baseline file. I want to stress that these aren't "known diffs" these are compatibility bugs that break your customers and you really shouldn't be baselining them or introducing the suppressions. #Resolved

[ericstj]

To provide paths to contract dependencies, add the following in your GetContract target:

 <_allReferenceDirectories Include="%(ReferencePath.RootDir)%(ReferencePath.Directory)" />
<_contractReferencePath Include="@(ReferencePath)" Condition="'%(FileName)' == '$(ContractName)'" />
<_contractReferencePath DependencyPaths="@(_allReferenceDirectories)" />

Then in ResolveMatchingContract add the following after you get the ResolvedMatchingContract item:

<PropertyGroup>
<ContractOutputPath>%(ResolvedMatchingContract.DependencyPaths)</ContractOutputPath>
</PropertyGroup>

I'm putting a feature into the APICompat targets that will make the latter property unnecessary. #Resolved

[eerhardt]

This project will actually build a .dll when we build the .sln file. That seems unnecessary. Maybe we should override the Build target or maybe not name it .csproj and instead just .proj? It's mission in life isn't to build .cs files into a .dll, but instead pull down external packages and provide the GetContract target.

[artidoro]

I tried renaming, but that did not work (it gave an error saying that project.assets.json was not generated.
I have also tried to overwrite the Build target, but I must have made some mistake since I still found the generated .dll.

What I did was adding:

 <Target Name="Build">
<!-- This will override the default Build target. -->
</Target>

to the .csproj file.

What is the correct way to overwrite it?

---

In reply to: 280105436 [](ancestors = 280105436)

[eerhardt]

Yes, I think that is fine.

[artidoro]

Thank you that seems to make it work!

---

In reply to: 488287870 [](ancestors = 488287870)

[codecov]

Codecov Report

Merging #3623 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #3623 +/- ##
==========================================
+ Coverage 72.78% 72.78% +<.01% 
==========================================
Files 808 808 Lines 145588 145588 Branches 16250 16250 ==========================================
+ Hits 105960 105968 +8 + Misses 35205 35198 -7 + Partials 4423 4422 -1

Flag	Coverage Δ
#Debug	72.78% <ø> (ø)	⬆️
#production	68.28% <ø> (ø)	⬆️
#test	89.04% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
...StandardTrainers/Standard/LinearModelParameters.cs	60.05% <0%> (-0.27%)	⬇️
...icrosoft.ML.TensorFlow/TensorFlow/TensorGeneric.cs	44.21% <0%> (ø)	⬆️
...c/Microsoft.ML.FastTree/Utils/BufferPoolManager.cs	0% <0%> (ø)	⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	86.26% <0%> (+0.15%)	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	84.9% <0%> (+0.2%)	⬆️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs	73.93% <0%> (+0.32%)	⬆️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs	89.89% <0%> (+0.62%)	⬆️

[ericstj]

failing on linux because dotnet is not on the path and the $(ToolHostCmd) is not set to point to it.

[artidoro]

Thank you @ericstj and @eerhardt for looking at the PR, I have addressed your comments.
Let me know if there are other things I should look into!

[ericstj]

Cool, this is looking pretty good. You may want to wait for my changes dotnet/arcade#2672 which help simplify some of this.

[artidoro]

I would prefer checking in this change as soon as possible and updating it with the new buyer as soon as it is available.

Since we have released yesterday, it would be great to have the API Compat tool in the repo.

[eerhardt]

You may want to revert these two lines. I don't know what happens if someone tries opening the solution with VS 2017 (which is version 15) and this .sln says it is for VS 2019.

[eerhardt]

I think this looks good. But I'd wait for @ericstj to give the thumbs up as he's the expert here.

[ericstj]

Why should we have the condition here? Is there any harm in always resolving for both TFMs?

[eerhardt]

Usually netcoreapp3.0 causes problems when you are building in VS 2017 where the SDK throws an error saying I don't support netcoreapp3.0.

[ericstj]

I see, makes sense.

[ericstj]

This could be improved by changing the project extension and defining a couple targets. We do that elsewhere:
dotnet/project-system#4647

IOW: call this a .proj, or a .restoreproj or something. SLN entry looks the same (you have to do it manually in text editor, IDE won't let you). Add the workaround I linked, define stub targets for anything that fails, and it should avoid
the confusion around a CSProj that doesn't build anything.
You don't need to do that now, but I think it would make this more sensible.

[artidoro]

I have tried to add the lines found in the issue to the new .restoreproj file but that did not work.
I will keep it as is for now then.

[ericstj]

Have you made sure you see a failure when making a breaking API change?

[artidoro]

Yes I have done a few tests where I rename methods and such and the build was failing.

[RussKie]

Lines 24 and 26 are identical... 😕

[eerhardt]

@RussKie - can you log an issue (or even submit a PR for the fix)?

[artidoro]

Fixes#3602.

We need to ensure that future changes to ML.NET will not break the stable API released in 1.0.0.

This PR introduces the API Compat tool from dotnet/Arcade. The API Compat tool runs as part of the build process and compares the assemblies with those found in the stable nugets referenced in the Microsoft.ML.StableAPI project.

The tool is only run for the assemblies which will be part of the stable nugets. Here is a list of those assemblies and the relative nugets:

Stable Nuget	Stable Assemblies
Microsoft.ML	Microsoft.ML.Core, Microsoft.ML.Data, Microsoft.ML.KMeansClustering, Microsoft.ML.PCA, Microsoft.ML.StandardTrainers, MIcrosoft.ML.Transforms, Microsoft.ML.Analyzer
Microsoft.ML.DataView	Microsoft.ML.DataView
Microsoft.ML.CpuMath	Microsoft.ML.CpuMath
Microsoft.ML.FastTree	Microsoft.ML.FastTree
Microsoft.ML.LightGbm	Microsoft.ML.LightGbm
Microsoft.ML.ImageAnalytics	Microsoft.ML.ImageAnalytics
Microsoft.ML.MklComponents	Microsoft.ML.Mkl.Components

Note: the tool does not run on Microsoft.ML.Analyzer as it does not have a public API, so no need to check backwards compatibility.

Still to do:

• Fix possible bug in API Compat on handling of Attributes.
• Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes. Not needed see comments below.
• Update the version of the ML.NET nugets to 1.0.0 when available

[ericstj]

Sorry if some of these were issues with my initial sample.

[ericstj]

Consider moving these to a seperate targets file if that is a convention you'd like to follow in this repo. #Pending

[artidoro]

I think we only have one for the src repo. But let me know if there is a better way.

---

In reply to: 279938766 [](ancestors = 279938766)

[ericstj]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version. #Resolved

[eerhardt]

(nit) you can use the latest 1.0.0-preview build: 1.0.0-preview-27630-5. We just spun it and will have the official 1.0.0 up soon. #Resolved

[artidoro]

Will update to the new build and change to 1.0.0 as soon as it is available.

---

In reply to: 279941849 [](ancestors = 279941849)

[eerhardt]

It's available on myget now. You should be able to use 1.0.0. #Resolved

[ericstj]

D:\a\1\s\packages\microsoft.dotnet.apicompat\1.0.0-beta.19225.5\build\Microsoft.DotNet.ApiCompat.targets(72,5): error : CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.ML.ExtensionBaseAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation. [D:\a\1\s\src\Microsoft.ML.Core\Microsoft.ML.Core.csproj]

This is happening because APICompat is not resolving the System.AttributeTargets enum from the contract. It's not resolving because you aren't providing the dependencies. Today these can be passed in via the $(ContractOutputPath) property but that isn't very sensible. I'll submit a change to APICompat to make that easier.
https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L46-L52 #Pending

[artidoro]

That would be great! Thank you

---

In reply to: 488135991 [](ancestors = 488135991)

[artidoro]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version.

@ericstj is it possible to specify the path for the generated file?
Also, how would we specify known diffs going forward? I would like to write some documentation on how to do that. #Resolved

[ericstj]

by default the generated file gets written to the same directory as the project with the name, but you can customize it: https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L22-L23

how would we specify known diffs going forward?

You can use the same option I mentioned when building individual projects, you can also copy the errors and paste them into the baseline file. I want to stress that these aren't "known diffs" these are compatibility bugs that break your customers and you really shouldn't be baselining them or introducing the suppressions. #Resolved

[ericstj]

To provide paths to contract dependencies, add the following in your GetContract target:

 <_allReferenceDirectories Include="%(ReferencePath.RootDir)%(ReferencePath.Directory)" />
<_contractReferencePath Include="@(ReferencePath)" Condition="'%(FileName)' == '$(ContractName)'" />
<_contractReferencePath DependencyPaths="@(_allReferenceDirectories)" />

Then in ResolveMatchingContract add the following after you get the ResolvedMatchingContract item:

<PropertyGroup>
<ContractOutputPath>%(ResolvedMatchingContract.DependencyPaths)</ContractOutputPath>
</PropertyGroup>

I'm putting a feature into the APICompat targets that will make the latter property unnecessary. #Resolved

[eerhardt]

This project will actually build a .dll when we build the .sln file. That seems unnecessary. Maybe we should override the Build target or maybe not name it .csproj and instead just .proj? It's mission in life isn't to build .cs files into a .dll, but instead pull down external packages and provide the GetContract target.

[artidoro]

I tried renaming, but that did not work (it gave an error saying that project.assets.json was not generated.
I have also tried to overwrite the Build target, but I must have made some mistake since I still found the generated .dll.

What I did was adding:

 <Target Name="Build">
<!-- This will override the default Build target. -->
</Target>

to the .csproj file.

What is the correct way to overwrite it?

---

In reply to: 280105436 [](ancestors = 280105436)

[eerhardt]

Yes, I think that is fine.

[artidoro]

Thank you that seems to make it work!

---

In reply to: 488287870 [](ancestors = 488287870)

[codecov]

Codecov Report

Merging #3623 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #3623 +/- ##
==========================================
+ Coverage 72.78% 72.78% +<.01% 
==========================================
Files 808 808 Lines 145588 145588 Branches 16250 16250 ==========================================
+ Hits 105960 105968 +8 + Misses 35205 35198 -7 + Partials 4423 4422 -1

Flag	Coverage Δ
#Debug	72.78% <ø> (ø)	⬆️
#production	68.28% <ø> (ø)	⬆️
#test	89.04% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
...StandardTrainers/Standard/LinearModelParameters.cs	60.05% <0%> (-0.27%)	⬇️
...icrosoft.ML.TensorFlow/TensorFlow/TensorGeneric.cs	44.21% <0%> (ø)	⬆️
...c/Microsoft.ML.FastTree/Utils/BufferPoolManager.cs	0% <0%> (ø)	⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	86.26% <0%> (+0.15%)	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	84.9% <0%> (+0.2%)	⬆️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs	73.93% <0%> (+0.32%)	⬆️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs	89.89% <0%> (+0.62%)	⬆️

[ericstj]

failing on linux because dotnet is not on the path and the $(ToolHostCmd) is not set to point to it.

[artidoro]

Thank you @ericstj and @eerhardt for looking at the PR, I have addressed your comments.
Let me know if there are other things I should look into!

[ericstj]

Cool, this is looking pretty good. You may want to wait for my changes dotnet/arcade#2672 which help simplify some of this.

[artidoro]

I would prefer checking in this change as soon as possible and updating it with the new buyer as soon as it is available.

Since we have released yesterday, it would be great to have the API Compat tool in the repo.

[eerhardt]

You may want to revert these two lines. I don't know what happens if someone tries opening the solution with VS 2017 (which is version 15) and this .sln says it is for VS 2019.

[eerhardt]

I think this looks good. But I'd wait for @ericstj to give the thumbs up as he's the expert here.

[ericstj]

Why should we have the condition here? Is there any harm in always resolving for both TFMs?

[eerhardt]

Usually netcoreapp3.0 causes problems when you are building in VS 2017 where the SDK throws an error saying I don't support netcoreapp3.0.

[ericstj]

I see, makes sense.

[ericstj]

This could be improved by changing the project extension and defining a couple targets. We do that elsewhere:
dotnet/project-system#4647

IOW: call this a .proj, or a .restoreproj or something. SLN entry looks the same (you have to do it manually in text editor, IDE won't let you). Add the workaround I linked, define stub targets for anything that fails, and it should avoid
the confusion around a CSProj that doesn't build anything.
You don't need to do that now, but I think it would make this more sensible.

[artidoro]

I have tried to add the lines found in the issue to the new .restoreproj file but that did not work.
I will keep it as is for now then.

[ericstj]

Have you made sure you see a failure when making a breaking API change?

[artidoro]

Yes I have done a few tests where I rename methods and such and the build was failing.

[RussKie]

Lines 24 and 26 are identical... 😕

[eerhardt]

@RussKie - can you log an issue (or even submit a PR for the fix)?

[artidoro]

Fixes#3602.

We need to ensure that future changes to ML.NET will not break the stable API released in 1.0.0.

This PR introduces the API Compat tool from dotnet/Arcade. The API Compat tool runs as part of the build process and compares the assemblies with those found in the stable nugets referenced in the Microsoft.ML.StableAPI project.

The tool is only run for the assemblies which will be part of the stable nugets. Here is a list of those assemblies and the relative nugets:

Stable Nuget	Stable Assemblies
Microsoft.ML	Microsoft.ML.Core, Microsoft.ML.Data, Microsoft.ML.KMeansClustering, Microsoft.ML.PCA, Microsoft.ML.StandardTrainers, MIcrosoft.ML.Transforms, Microsoft.ML.Analyzer
Microsoft.ML.DataView	Microsoft.ML.DataView
Microsoft.ML.CpuMath	Microsoft.ML.CpuMath
Microsoft.ML.FastTree	Microsoft.ML.FastTree
Microsoft.ML.LightGbm	Microsoft.ML.LightGbm
Microsoft.ML.ImageAnalytics	Microsoft.ML.ImageAnalytics
Microsoft.ML.MklComponents	Microsoft.ML.Mkl.Components

Note: the tool does not run on Microsoft.ML.Analyzer as it does not have a public API, so no need to check backwards compatibility.

Still to do:

• Fix possible bug in API Compat on handling of Attributes.
• Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes. Not needed see comments below.
• Update the version of the ML.NET nugets to 1.0.0 when available

[ericstj]

Sorry if some of these were issues with my initial sample.

[ericstj]

Consider moving these to a seperate targets file if that is a convention you'd like to follow in this repo. #Pending

[artidoro]

I think we only have one for the src repo. But let me know if there is a better way.

---

In reply to: 279938766 [](ancestors = 279938766)

[ericstj]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version. #Resolved

[eerhardt]

(nit) you can use the latest 1.0.0-preview build: 1.0.0-preview-27630-5. We just spun it and will have the official 1.0.0 up soon. #Resolved

[artidoro]

Will update to the new build and change to 1.0.0 as soon as it is available.

---

In reply to: 279941849 [](ancestors = 279941849)

[eerhardt]

It's available on myget now. You should be able to use 1.0.0. #Resolved

[ericstj]

D:\a\1\s\packages\microsoft.dotnet.apicompat\1.0.0-beta.19225.5\build\Microsoft.DotNet.ApiCompat.targets(72,5): error : CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.ML.ExtensionBaseAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation. [D:\a\1\s\src\Microsoft.ML.Core\Microsoft.ML.Core.csproj]

This is happening because APICompat is not resolving the System.AttributeTargets enum from the contract. It's not resolving because you aren't providing the dependencies. Today these can be passed in via the $(ContractOutputPath) property but that isn't very sensible. I'll submit a change to APICompat to make that easier.
https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L46-L52 #Pending

[artidoro]

That would be great! Thank you

---

In reply to: 488135991 [](ancestors = 488135991)

[artidoro]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version.

@ericstj is it possible to specify the path for the generated file?
Also, how would we specify known diffs going forward? I would like to write some documentation on how to do that. #Resolved

[ericstj]

by default the generated file gets written to the same directory as the project with the name, but you can customize it: https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L22-L23

how would we specify known diffs going forward?

You can use the same option I mentioned when building individual projects, you can also copy the errors and paste them into the baseline file. I want to stress that these aren't "known diffs" these are compatibility bugs that break your customers and you really shouldn't be baselining them or introducing the suppressions. #Resolved

[ericstj]

To provide paths to contract dependencies, add the following in your GetContract target:

 <_allReferenceDirectories Include="%(ReferencePath.RootDir)%(ReferencePath.Directory)" />
<_contractReferencePath Include="@(ReferencePath)" Condition="'%(FileName)' == '$(ContractName)'" />
<_contractReferencePath DependencyPaths="@(_allReferenceDirectories)" />

Then in ResolveMatchingContract add the following after you get the ResolvedMatchingContract item:

<PropertyGroup>
<ContractOutputPath>%(ResolvedMatchingContract.DependencyPaths)</ContractOutputPath>
</PropertyGroup>

I'm putting a feature into the APICompat targets that will make the latter property unnecessary. #Resolved

[eerhardt]

This project will actually build a .dll when we build the .sln file. That seems unnecessary. Maybe we should override the Build target or maybe not name it .csproj and instead just .proj? It's mission in life isn't to build .cs files into a .dll, but instead pull down external packages and provide the GetContract target.

[artidoro]

I tried renaming, but that did not work (it gave an error saying that project.assets.json was not generated.
I have also tried to overwrite the Build target, but I must have made some mistake since I still found the generated .dll.

What I did was adding:

 <Target Name="Build">
<!-- This will override the default Build target. -->
</Target>

to the .csproj file.

What is the correct way to overwrite it?

---

In reply to: 280105436 [](ancestors = 280105436)

[eerhardt]

Yes, I think that is fine.

[artidoro]

Thank you that seems to make it work!

---

In reply to: 488287870 [](ancestors = 488287870)

[codecov]

Codecov Report

Merging #3623 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #3623 +/- ##
==========================================
+ Coverage 72.78% 72.78% +<.01% 
==========================================
Files 808 808 Lines 145588 145588 Branches 16250 16250 ==========================================
+ Hits 105960 105968 +8 + Misses 35205 35198 -7 + Partials 4423 4422 -1

Flag	Coverage Δ
#Debug	72.78% <ø> (ø)	⬆️
#production	68.28% <ø> (ø)	⬆️
#test	89.04% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
...StandardTrainers/Standard/LinearModelParameters.cs	60.05% <0%> (-0.27%)	⬇️
...icrosoft.ML.TensorFlow/TensorFlow/TensorGeneric.cs	44.21% <0%> (ø)	⬆️
...c/Microsoft.ML.FastTree/Utils/BufferPoolManager.cs	0% <0%> (ø)	⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	86.26% <0%> (+0.15%)	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	84.9% <0%> (+0.2%)	⬆️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs	73.93% <0%> (+0.32%)	⬆️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs	89.89% <0%> (+0.62%)	⬆️

[ericstj]

failing on linux because dotnet is not on the path and the $(ToolHostCmd) is not set to point to it.

[artidoro]

Thank you @ericstj and @eerhardt for looking at the PR, I have addressed your comments.
Let me know if there are other things I should look into!

[ericstj]

Cool, this is looking pretty good. You may want to wait for my changes dotnet/arcade#2672 which help simplify some of this.

[artidoro]

I would prefer checking in this change as soon as possible and updating it with the new buyer as soon as it is available.

Since we have released yesterday, it would be great to have the API Compat tool in the repo.

[eerhardt]

You may want to revert these two lines. I don't know what happens if someone tries opening the solution with VS 2017 (which is version 15) and this .sln says it is for VS 2019.

[eerhardt]

I think this looks good. But I'd wait for @ericstj to give the thumbs up as he's the expert here.

[ericstj]

Why should we have the condition here? Is there any harm in always resolving for both TFMs?

[eerhardt]

Usually netcoreapp3.0 causes problems when you are building in VS 2017 where the SDK throws an error saying I don't support netcoreapp3.0.

[ericstj]

I see, makes sense.

[ericstj]

This could be improved by changing the project extension and defining a couple targets. We do that elsewhere:
dotnet/project-system#4647

IOW: call this a .proj, or a .restoreproj or something. SLN entry looks the same (you have to do it manually in text editor, IDE won't let you). Add the workaround I linked, define stub targets for anything that fails, and it should avoid
the confusion around a CSProj that doesn't build anything.
You don't need to do that now, but I think it would make this more sensible.

[artidoro]

I have tried to add the lines found in the issue to the new .restoreproj file but that did not work.
I will keep it as is for now then.

[ericstj]

Have you made sure you see a failure when making a breaking API change?

[artidoro]

Yes I have done a few tests where I rename methods and such and the build was failing.

[RussKie]

Lines 24 and 26 are identical... 😕

[eerhardt]

@RussKie - can you log an issue (or even submit a PR for the fix)?

[artidoro]

Fixes#3602.

We need to ensure that future changes to ML.NET will not break the stable API released in 1.0.0.

This PR introduces the API Compat tool from dotnet/Arcade. The API Compat tool runs as part of the build process and compares the assemblies with those found in the stable nugets referenced in the Microsoft.ML.StableAPI project.

The tool is only run for the assemblies which will be part of the stable nugets. Here is a list of those assemblies and the relative nugets:

Stable Nuget	Stable Assemblies
Microsoft.ML	Microsoft.ML.Core, Microsoft.ML.Data, Microsoft.ML.KMeansClustering, Microsoft.ML.PCA, Microsoft.ML.StandardTrainers, MIcrosoft.ML.Transforms, Microsoft.ML.Analyzer
Microsoft.ML.DataView	Microsoft.ML.DataView
Microsoft.ML.CpuMath	Microsoft.ML.CpuMath
Microsoft.ML.FastTree	Microsoft.ML.FastTree
Microsoft.ML.LightGbm	Microsoft.ML.LightGbm
Microsoft.ML.ImageAnalytics	Microsoft.ML.ImageAnalytics
Microsoft.ML.MklComponents	Microsoft.ML.Mkl.Components

Note: the tool does not run on Microsoft.ML.Analyzer as it does not have a public API, so no need to check backwards compatibility.

Still to do:

• Fix possible bug in API Compat on handling of Attributes.
• Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes. Not needed see comments below.
• Update the version of the ML.NET nugets to 1.0.0 when available

[ericstj]

Sorry if some of these were issues with my initial sample.

[ericstj]

Consider moving these to a seperate targets file if that is a convention you'd like to follow in this repo. #Pending

[artidoro]

I think we only have one for the src repo. But let me know if there is a better way.

---

In reply to: 279938766 [](ancestors = 279938766)

[ericstj]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version. #Resolved

[eerhardt]

(nit) you can use the latest 1.0.0-preview build: 1.0.0-preview-27630-5. We just spun it and will have the official 1.0.0 up soon. #Resolved

[artidoro]

Will update to the new build and change to 1.0.0 as soon as it is available.

---

In reply to: 279941849 [](ancestors = 279941849)

[eerhardt]

It's available on myget now. You should be able to use 1.0.0. #Resolved

[ericstj]

D:\a\1\s\packages\microsoft.dotnet.apicompat\1.0.0-beta.19225.5\build\Microsoft.DotNet.ApiCompat.targets(72,5): error : CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.ML.ExtensionBaseAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation. [D:\a\1\s\src\Microsoft.ML.Core\Microsoft.ML.Core.csproj]

This is happening because APICompat is not resolving the System.AttributeTargets enum from the contract. It's not resolving because you aren't providing the dependencies. Today these can be passed in via the $(ContractOutputPath) property but that isn't very sensible. I'll submit a change to APICompat to make that easier.
https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L46-L52 #Pending

[artidoro]

That would be great! Thank you

---

In reply to: 488135991 [](ancestors = 488135991)

[artidoro]

Add a build setting to disable the API Compat tool from command line. This will allow people to work on breaking changes.

The right way to do this is to build once specifying /p:BaselineAllAPICompatError=true. This will generate text files in the repo that list all the errors and let the build succeed despite the compatibility issues. Typically you then file issues to address all the baselines by the time you ship the next version.

@ericstj is it possible to specify the path for the generated file?
Also, how would we specify known diffs going forward? I would like to write some documentation on how to do that. #Resolved

[ericstj]

by default the generated file gets written to the same directory as the project with the name, but you can customize it: https://github.com/dotnet/arcade/blob/ac8d88df02d246d3147338fcfb03b1b93dc84b53/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L22-L23

how would we specify known diffs going forward?

You can use the same option I mentioned when building individual projects, you can also copy the errors and paste them into the baseline file. I want to stress that these aren't "known diffs" these are compatibility bugs that break your customers and you really shouldn't be baselining them or introducing the suppressions. #Resolved

[ericstj]

To provide paths to contract dependencies, add the following in your GetContract target:

 <_allReferenceDirectories Include="%(ReferencePath.RootDir)%(ReferencePath.Directory)" />
<_contractReferencePath Include="@(ReferencePath)" Condition="'%(FileName)' == '$(ContractName)'" />
<_contractReferencePath DependencyPaths="@(_allReferenceDirectories)" />

Then in ResolveMatchingContract add the following after you get the ResolvedMatchingContract item:

<PropertyGroup>
<ContractOutputPath>%(ResolvedMatchingContract.DependencyPaths)</ContractOutputPath>
</PropertyGroup>

I'm putting a feature into the APICompat targets that will make the latter property unnecessary. #Resolved

[eerhardt]

This project will actually build a .dll when we build the .sln file. That seems unnecessary. Maybe we should override the Build target or maybe not name it .csproj and instead just .proj? It's mission in life isn't to build .cs files into a .dll, but instead pull down external packages and provide the GetContract target.

[artidoro]

I tried renaming, but that did not work (it gave an error saying that project.assets.json was not generated.
I have also tried to overwrite the Build target, but I must have made some mistake since I still found the generated .dll.

What I did was adding:

 <Target Name="Build">
<!-- This will override the default Build target. -->
</Target>

to the .csproj file.

What is the correct way to overwrite it?

---

In reply to: 280105436 [](ancestors = 280105436)

[eerhardt]

Yes, I think that is fine.

[artidoro]

Thank you that seems to make it work!

---

In reply to: 488287870 [](ancestors = 488287870)

[codecov]

Codecov Report

Merging #3623 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #3623 +/- ##
==========================================
+ Coverage 72.78% 72.78% +<.01% 
==========================================
Files 808 808 Lines 145588 145588 Branches 16250 16250 ==========================================
+ Hits 105960 105968 +8 + Misses 35205 35198 -7 + Partials 4423 4422 -1

Flag	Coverage Δ
#Debug	72.78% <ø> (ø)	⬆️
#production	68.28% <ø> (ø)	⬆️
#test	89.04% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
...StandardTrainers/Standard/LinearModelParameters.cs	60.05% <0%> (-0.27%)	⬇️
...icrosoft.ML.TensorFlow/TensorFlow/TensorGeneric.cs	44.21% <0%> (ø)	⬆️
...c/Microsoft.ML.FastTree/Utils/BufferPoolManager.cs	0% <0%> (ø)	⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	86.26% <0%> (+0.15%)	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	84.9% <0%> (+0.2%)	⬆️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs	73.93% <0%> (+0.32%)	⬆️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs	89.89% <0%> (+0.62%)	⬆️

[ericstj]

failing on linux because dotnet is not on the path and the $(ToolHostCmd) is not set to point to it.

[artidoro]

Thank you @ericstj and @eerhardt for looking at the PR, I have addressed your comments.
Let me know if there are other things I should look into!

[ericstj]

Cool, this is looking pretty good. You may want to wait for my changes dotnet/arcade#2672 which help simplify some of this.

[artidoro]

I would prefer checking in this change as soon as possible and updating it with the new buyer as soon as it is available.

Since we have released yesterday, it would be great to have the API Compat tool in the repo.

[eerhardt]

You may want to revert these two lines. I don't know what happens if someone tries opening the solution with VS 2017 (which is version 15) and this .sln says it is for VS 2019.

[eerhardt]

I think this looks good. But I'd wait for @ericstj to give the thumbs up as he's the expert here.

[ericstj]

Why should we have the condition here? Is there any harm in always resolving for both TFMs?

[eerhardt]

Usually netcoreapp3.0 causes problems when you are building in VS 2017 where the SDK throws an error saying I don't support netcoreapp3.0.

[ericstj]

I see, makes sense.

[ericstj]

This could be improved by changing the project extension and defining a couple targets. We do that elsewhere:
dotnet/project-system#4647

IOW: call this a .proj, or a .restoreproj or something. SLN entry looks the same (you have to do it manually in text editor, IDE won't let you). Add the workaround I linked, define stub targets for anything that fails, and it should avoid
the confusion around a CSProj that doesn't build anything.
You don't need to do that now, but I think it would make this more sensible.

[artidoro]

I have tried to add the lines found in the issue to the new .restoreproj file but that did not work.
I will keep it as is for now then.

[ericstj]

Have you made sure you see a failure when making a breaking API change?

[artidoro]

Yes I have done a few tests where I rename methods and such and the build was failing.

[RussKie]

Lines 24 and 26 are identical... 😕

[eerhardt]

@RussKie - can you log an issue (or even submit a PR for the fix)?