API Compat tool in ML.NET #3623

artidoro · 2019-04-30T20:57:49Z

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.

Directory.Build.props

src/Directory.Build.targets

tools-local/Microsoft.ML.StableApi/Microsoft.ML.StableApi.csproj

src/Microsoft.ML.Analyzer/Microsoft.ML.Analyzer.csproj

ericstj · 2019-04-30T21:13:44Z

src/Directory.Build.targets

@@ -26,4 +26,29 @@

  </Target>

+  <!-- API Compat -->


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

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 · 2019-04-30T21:16:09Z

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

src/Microsoft.ML.Core/Microsoft.ML.Core.csproj

eerhardt · 2019-04-30T21:23:01Z

tools-local/Microsoft.ML.StableApi/Microsoft.ML.StableApi.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="Microsoft.ML" Version="1.0.0-preview-27625-16"/>


(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

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

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

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

ericstj · 2019-04-30T22:18:21Z

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 · 2019-04-30T22:30:49Z

That would be great! Thank you

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

artidoro · 2019-04-30T23:14:12Z

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 · 2019-05-01T04:42:32Z

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 · 2019-05-01T13:52:58Z

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

src/Microsoft.ML.Core/Microsoft.ML.Core.csproj

src/Microsoft.ML.Analyzer/Microsoft.ML.Analyzer.csproj

tools-local/Microsoft.ML.StableApi/Microsoft.ML.StableApi.csproj

eerhardt · 2019-05-01T15:21:18Z

tools-local/Microsoft.ML.StableApi/Microsoft.ML.StableApi.csproj

@@ -0,0 +1,31 @@
+<Project Sdk="Microsoft.NET.Sdk">


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.

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">  </Target>

to the .csproj file.

What is the correct way to overwrite it?

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

Yes, I think that is fine.

Directory.Build.props

artidoro · 2019-05-03T00:10:51Z

Thank you that seems to make it work!

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

codecov · 2019-05-03T01:46:09Z

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 · 2019-05-03T04:16:36Z

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

artidoro · 2019-05-03T05:23:48Z

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 · 2019-05-03T05:47:55Z

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

artidoro · 2019-05-03T13:49:59Z

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 · 2019-05-03T14:27:28Z

Microsoft.ML.sln

@@ -1,6 +1,6 @@
 Microsoft Visual Studio Solution File, Format Version 12.00
-# Visual Studio 15
-VisualStudioVersion = 15.0.27130.2026
+# Visual Studio Version 16


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 · 2019-05-03T16:35:11Z

tools-local/Microsoft.ML.StableApi/Microsoft.ML.StableApi.csproj

+  <PropertyGroup>
+    <!-- needs to contain all frameworks which src projects wish to restore -->
+    <TargetFramework Condition="'$(UseIntrinsics)' != 'true'">netstandard2.0</TargetFramework>
+    <TargetFrameworks Condition="'$(UseIntrinsics)' == 'true'">netstandard2.0;netcoreapp3.0</TargetFrameworks>


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

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.

I see, makes sense.

ericstj · 2019-05-03T16:51:55Z

tools-local/Microsoft.ML.StableApi/Microsoft.ML.StableApi.csproj

+    <Error Condition="'@(_contractReferencePath)' == ''" Text="Could not locate $(ContractName)" />
+  </Target>
+
+  <Target Name="Build">


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.

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 · 2019-05-03T17:00:11Z

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

artidoro · 2019-05-03T23:16:17Z

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

RussKie · 2019-10-28T11:49:46Z

tools-local/Microsoft.ML.StableApi/Microsoft.ML.StableApi.csproj

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


Lines 24 and 26 are identical... 😕

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

artidoro added the Build Build related issue label Apr 30, 2019

artidoro requested review from shauheen, eerhardt, TomFinley and ericstj April 30, 2019 20:57

artidoro self-assigned this Apr 30, 2019

introduce API Compat tool in ML.NET

449c8df

artidoro force-pushed the apicompat branch from 0647d98 to 449c8df Compare April 30, 2019 21:02

ericstj reviewed Apr 30, 2019

View reviewed changes

eerhardt reviewed Apr 30, 2019

View reviewed changes

src/Microsoft.ML.Core/Microsoft.ML.Core.csproj Outdated Show resolved Hide resolved

eerhardt reviewed Apr 30, 2019

View reviewed changes

addressing review comments

f0ebe31

artidoro force-pushed the apicompat branch from 6429029 to f0ebe31 Compare April 30, 2019 22:31

remove redundant IsStableProject definition

4e93cf3

artidoro force-pushed the apicompat branch from 8eea12c to 4e93cf3 Compare April 30, 2019 23:26

ericstj mentioned this pull request May 1, 2019

Make APICompat friendlier dotnet/arcade#2297

Closed

4 tasks

eerhardt reviewed May 1, 2019

View reviewed changes

src/Microsoft.ML.Core/Microsoft.ML.Core.csproj Outdated Show resolved Hide resolved

eerhardt reviewed May 1, 2019

View reviewed changes

src/Microsoft.ML.Analyzer/Microsoft.ML.Analyzer.csproj Outdated Show resolved Hide resolved

eerhardt reviewed May 1, 2019

View reviewed changes

tools-local/Microsoft.ML.StableApi/Microsoft.ML.StableApi.csproj Outdated Show resolved Hide resolved

eerhardt reviewed May 1, 2019

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

review comments

1ba081c

artidoro changed the title ~~WIP: API Compat tool in ML.NET~~ API Compat tool in ML.NET May 3, 2019

target framework 3.0

c113ef3

artidoro force-pushed the apicompat branch from 5c1506a to c113ef3 Compare May 3, 2019 01:08

fix dotnet not found

455ab08

artidoro force-pushed the apicompat branch from 68a801e to 455ab08 Compare May 3, 2019 05:02

eerhardt reviewed May 3, 2019

View reviewed changes

eerhardt approved these changes May 3, 2019

View reviewed changes

ericstj approved these changes May 3, 2019

View reviewed changes

revert solution file change to version

b9d2fac

artidoro merged commit 4bcc94c into dotnet:master May 4, 2019

This was referenced May 6, 2019

Official build failure due to the API Compat tool PR #3666

Closed

Fix to the official build (API Compat tool) #3667

Merged

RussKie reviewed Oct 28, 2019

View reviewed changes

ghost locked as resolved and limited conversation to collaborators Mar 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Compat tool in ML.NET #3623

API Compat tool in ML.NET #3623

artidoro commented Apr 30, 2019 •

edited

Loading

ericstj left a comment

ericstj Apr 30, 2019 •

edited by artidoro

Loading

artidoro Apr 30, 2019

ericstj commented Apr 30, 2019 •

edited by artidoro

Loading

eerhardt Apr 30, 2019 •

edited by artidoro

Loading

artidoro Apr 30, 2019

eerhardt May 1, 2019 •

edited by artidoro

Loading

ericstj commented Apr 30, 2019 •

edited by artidoro

Loading

artidoro commented Apr 30, 2019

artidoro commented Apr 30, 2019 •

edited

Loading

ericstj commented May 1, 2019 •

edited by artidoro

Loading

ericstj commented May 1, 2019 •

edited by artidoro

Loading

eerhardt May 1, 2019

artidoro May 3, 2019 •

edited by eerhardt

Loading

eerhardt May 3, 2019

artidoro commented May 3, 2019

codecov bot commented May 3, 2019 •

edited

Loading

ericstj commented May 3, 2019

artidoro commented May 3, 2019

ericstj commented May 3, 2019

artidoro commented May 3, 2019

eerhardt May 3, 2019

eerhardt left a comment

ericstj May 3, 2019

eerhardt May 3, 2019

ericstj May 3, 2019

ericstj May 3, 2019

artidoro May 3, 2019

ericstj commented May 3, 2019

artidoro commented May 3, 2019

RussKie Oct 28, 2019

eerhardt Oct 28, 2019

API Compat tool in ML.NET #3623

API Compat tool in ML.NET #3623

Conversation

artidoro commented Apr 30, 2019 • edited Loading

ericstj left a comment

Choose a reason for hiding this comment

ericstj Apr 30, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj commented Apr 30, 2019 • edited by artidoro Loading

eerhardt Apr 30, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt May 1, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

ericstj commented Apr 30, 2019 • edited by artidoro Loading

artidoro commented Apr 30, 2019

artidoro commented Apr 30, 2019 • edited Loading

ericstj commented May 1, 2019 • edited by artidoro Loading

ericstj commented May 1, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

artidoro May 3, 2019 • edited by eerhardt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro commented May 3, 2019

codecov bot commented May 3, 2019 • edited Loading

Codecov Report

ericstj commented May 3, 2019

artidoro commented May 3, 2019

ericstj commented May 3, 2019

artidoro commented May 3, 2019

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj commented May 3, 2019

artidoro commented May 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro commented Apr 30, 2019 •

edited

Loading

ericstj Apr 30, 2019 •

edited by artidoro

Loading

ericstj commented Apr 30, 2019 •

edited by artidoro

Loading

eerhardt Apr 30, 2019 •

edited by artidoro

Loading

eerhardt May 1, 2019 •

edited by artidoro

Loading

ericstj commented Apr 30, 2019 •

edited by artidoro

Loading

artidoro commented Apr 30, 2019 •

edited

Loading

ericstj commented May 1, 2019 •

edited by artidoro

Loading

ericstj commented May 1, 2019 •

edited by artidoro

Loading

artidoro May 3, 2019 •

edited by eerhardt

Loading

codecov bot commented May 3, 2019 •

edited

Loading