Skip to content

API Compat tool in ML.NET #3623

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 4, 2019
Merged

API Compat tool in ML.NET #3623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
449c8df
introduce API Compat tool in ML.NET
artidoro Apr 30, 2019
f0ebe31
addressing review comments
artidoro Apr 30, 2019
4e93cf3
remove redundant IsStableProject definition
artidoro Apr 30, 2019
1ba081c
review comments
artidoro May 3, 2019
c113ef3
target framework 3.0
artidoro May 3, 2019
455ab08
fix dotnet not found
artidoro May 3, 2019
b9d2fac
revert solution file change to version
artidoro May 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions Microsoft.ML.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Microsoft.ML.FastTree", "Mi
pkg\Microsoft.ML.FastTree\Microsoft.ML.FastTree.symbols.nupkgproj = pkg\Microsoft.ML.FastTree\Microsoft.ML.FastTree.symbols.nupkgproj
EndProjectSection
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.ML.StableApi", "tools-local\Microsoft.ML.StableApi\Microsoft.ML.StableApi.csproj", "{F308DC6B-7E59-40D7-A581-834E8CD99CFE}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -968,6 +970,18 @@ Global
{E02DA82D-3FEE-4C60-BD80-9EC3C3448DFC}.Release-Intrinsics|Any CPU.Build.0 = Release-Intrinsics|Any CPU
{E02DA82D-3FEE-4C60-BD80-9EC3C3448DFC}.Release-netfx|Any CPU.ActiveCfg = Release-netfx|Any CPU
{E02DA82D-3FEE-4C60-BD80-9EC3C3448DFC}.Release-netfx|Any CPU.Build.0 = Release-netfx|Any CPU
{F308DC6B-7E59-40D7-A581-834E8CD99CFE}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{F308DC6B-7E59-40D7-A581-834E8CD99CFE}.Debug|Any CPU.Build.0 = Debug|Any CPU
{F308DC6B-7E59-40D7-A581-834E8CD99CFE}.Debug-Intrinsics|Any CPU.ActiveCfg = Debug-Intrinsics|Any CPU
{F308DC6B-7E59-40D7-A581-834E8CD99CFE}.Debug-Intrinsics|Any CPU.Build.0 = Debug-Intrinsics|Any CPU
{F308DC6B-7E59-40D7-A581-834E8CD99CFE}.Debug-netfx|Any CPU.ActiveCfg = Debug-netfx|Any CPU
{F308DC6B-7E59-40D7-A581-834E8CD99CFE}.Debug-netfx|Any CPU.Build.0 = Debug-netfx|Any CPU
{F308DC6B-7E59-40D7-A581-834E8CD99CFE}.Release|Any CPU.ActiveCfg = Release|Any CPU
{F308DC6B-7E59-40D7-A581-834E8CD99CFE}.Release|Any CPU.Build.0 = Release|Any CPU
{F308DC6B-7E59-40D7-A581-834E8CD99CFE}.Release-Intrinsics|Any CPU.ActiveCfg = Release-Intrinsics|Any CPU
{F308DC6B-7E59-40D7-A581-834E8CD99CFE}.Release-Intrinsics|Any CPU.Build.0 = Release-Intrinsics|Any CPU
{F308DC6B-7E59-40D7-A581-834E8CD99CFE}.Release-netfx|Any CPU.ActiveCfg = Release-netfx|Any CPU
{F308DC6B-7E59-40D7-A581-834E8CD99CFE}.Release-netfx|Any CPU.Build.0 = Release-netfx|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down Expand Up @@ -1055,6 +1069,7 @@ Global
{AD7058C9-5608-49A8-BE23-58C33A74EE91} = {D3D38B03-B557-484D-8348-8BADEE4DF592}
{E02DA82D-3FEE-4C60-BD80-9EC3C3448DFC} = {09EADF06-BE25-4228-AB53-95AE3E15B530}
{B1B3F284-FA3D-4D76-A712-FF04495D244B} = {D3D38B03-B557-484D-8348-8BADEE4DF592}
{F308DC6B-7E59-40D7-A581-834E8CD99CFE} = {7F13E156-3EBA-4021-84A5-CD56BA72F99E}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {41165AF1-35BB-4832-A189-73060F82B01D}
Expand Down
1 change: 1 addition & 0 deletions build/Dependencies.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
<CodecovVersion>1.1.1</CodecovVersion>
<CoverletVersion>2.6.0</CoverletVersion>
<ReportGeneratorVersion>4.0.9</ReportGeneratorVersion>
<MicrosoftDotNetApiCompatPackageVersion>1.0.0-beta.19225.5</MicrosoftDotNetApiCompatPackageVersion>
</PropertyGroup>

<!-- Test-only Dependencies -->
Expand Down
14 changes: 13 additions & 1 deletion src/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,19 @@
<PackageRid>$(PackageRid)-$(TargetArchitecture)</PackageRid>
</PropertyGroup>

<!-- API Compat -->
<PropertyGroup>
<RunApiCompat Condition="'$(RunApiCompat)' == ''">$(IsStableProject)</RunApiCompat>
<ToolHostCmd Condition="'$(OS)' != 'Windows_NT'">$(ToolsDir)dotnetcli/dotnet</ToolHostCmd>


<!-- only validate that the current implementation is compatible
with the old one, not vice-versa (since latest may have new
API missing from old) -->
<RunApiCompatForSrc>true</RunApiCompatForSrc>
<RunMatchingRefApiCompat>false</RunMatchingRefApiCompat>
</PropertyGroup>

<ItemGroup>
<ProjectReference
Condition="'$(UseMLCodeAnalyzer)' != 'false' and '$(MSBuildProjectExtension)' == '.csproj'"
Expand All @@ -37,5 +50,4 @@
<Link>stylecop.json</Link>
</AdditionalFiles>
</ItemGroup>

</Project>
28 changes: 28 additions & 0 deletions src/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,32 @@

</Target>

<!-- API Compat -->
Copy link
Member

@ericstj ericstj Apr 30, 2019 •
edited by artidoro
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@artidoro artidoro Apr 30, 2019

Choose a reason for hiding this comment

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

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)

<PropertyGroup Condition="'$(RunApiCompat)' == 'true'">
<!-- resolve contract for APICompat as part of resolve references -->
<ResolveReferencesDependsOn>
$(ResolveReferencesDependsOn);
ResolveMatchingContract
</ResolveReferencesDependsOn>

<StableApiProject>$(RepoRoot)tools-local\Microsoft.ML.StableApi\Microsoft.ML.StableApi.csproj</StableApiProject>
</PropertyGroup>

<ItemGroup Condition="'$(RunApiCompat)' == 'true'">
<PackageReference Include="Microsoft.DotNet.ApiCompat"
Version="$(MicrosoftDotNetApiCompatPackageVersion)"
PrivateAssets="All" />
</ItemGroup>

<Target Name="ResolveMatchingContract">
<MSBuild Projects="$(StableApiProject)"
Targets="GetContract"
Properties="ContractName=$(AssemblyName);TargetFramework=$(TargetFramework)">
<Output TaskParameter="TargetOutputs" ItemName="ResolvedMatchingContract"/>
</MSBuild>
<PropertyGroup>
<ContractOutputPath>%(ResolvedMatchingContract.DependencyPaths)</ContractOutputPath>
</PropertyGroup>
</Target>

</Project>
45 changes: 45 additions & 0 deletions tools-local/Microsoft.ML.StableApi/Microsoft.ML.StableApi.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Member

@eerhardt eerhardt May 1, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@artidoro artidoro May 3, 2019 •
edited by eerhardt
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member

@eerhardt eerhardt May 3, 2019

Choose a reason for hiding this comment

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

Yes, I think that is fine.


<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>
Copy link
Member

@ericstj ericstj May 3, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

@eerhardt eerhardt May 3, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ericstj ericstj May 3, 2019

Choose a reason for hiding this comment

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

I see, makes sense.

</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.ML" Version="1.0.0" />
<PackageReference Include="Microsoft.ML.DataView" Version="1.0.0" />
<PackageReference Include="Microsoft.ML.CpuMath" Version="1.0.0" />
<PackageReference Include="Microsoft.ML.FastTree" Version="1.0.0" />
<PackageReference Include="Microsoft.ML.LightGbm" Version="1.0.0" />
<PackageReference Include="Microsoft.ML.ImageAnalytics" Version="1.0.0" />
<PackageReference Include="Microsoft.ML.Mkl.Components" Version="1.0.0" />
</ItemGroup>

<!-- The purpose of this target is to return a path from a referenced
package / project -->
<Target Name="GetContract" DependsOnTargets="ResolveReferences" Returns="@(_contractReferencePath)">
<Error Condition="'$(ContractName)' == ''" Text="ContractName must be specified" />
<ItemGroup>
<_contractReferencePath Include="@(ReferencePath)" Condition="'%(FileName)' == '$(ContractName)'" />
<_allReferenceDirectories Include="%(ReferencePath.RootDir)%(ReferencePath.Directory)" />
<_contractReferencePath Include="@(ReferencePath)" Condition="'%(FileName)' == '$(ContractName)'" />
Copy link
Contributor

@RussKie RussKie Oct 28, 2019

Choose a reason for hiding this comment

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

Lines 24 and 26 are identical... 😕

Copy link
Member

@eerhardt eerhardt Oct 28, 2019

Choose a reason for hiding this comment

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

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

<_contractReferencePath DependencyPaths="@(_allReferenceDirectories)" />
</ItemGroup>
<Error Condition="'@(_contractReferencePath)' == ''" Text="Could not locate $(ContractName)" />
</Target>

<Target Name="Build">
Copy link
Member

@ericstj ericstj May 3, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@artidoro artidoro May 3, 2019

Choose a reason for hiding this comment

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

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.

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

</Project>