Arm build changes by michaelgsharp · Pull Request #5789 · dotnet/machinelearning

michaelgsharp · 2021-05-10T20:34:48Z

Changes that allow building for arm/arm64/apple silicon, as well as cross targeting to those same architectures.

Tests don't pass on those architectures yet, this is just enabling building. Anything that doesn't depend on x86/64 SIMD or IntelMKL works correctly.

codecov · 2021-05-10T22:23:51Z

Codecov Report

Merging #5789 (b207b56) into main (43c49f6) will decrease coverage by 0.03%.
The diff coverage is 25.40%.

@@            Coverage Diff             @@
##             main    #5789      +/-   ##
==========================================
- Coverage   68.35%   68.32%   -0.04%     
==========================================
  Files        1131     1131              
  Lines      241210   241292      +82     
  Branches    25039    25053      +14     
==========================================
- Hits       164887   164860      -27     
- Misses      69819    69928     +109     
  Partials     6504     6504

Flag	Coverage Δ
Debug	`68.32% <25.40%> (-0.04%)`	⬇️
production	`62.93% <24.57%> (-0.05%)`	⬇️
test	`89.24% <50.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...c/Microsoft.ML.FastTree/Dataset/SegmentIntArray.cs	`0.00% <0.00%> (ø)`
...t/Microsoft.ML.PerformanceTests/Harness/Configs.cs	`0.00% <0.00%> (ø)`
src/Microsoft.ML.FastTree/FastTreeRanking.cs	`50.79% <15.94%> (-4.28%)`	⬇️
src/Microsoft.ML.FastTree/Dataset/IntArray.cs	`12.10% <25.00%> (-0.11%)`	⬇️
src/Microsoft.ML.FastTree/Dataset/DenseIntArray.cs	`26.73% <33.33%> (+0.11%)`	⬆️
...rc/Microsoft.ML.FastTree/Dataset/SparseIntArray.cs	`54.40% <91.66%> (+0.23%)`	⬆️
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs	`70.11% <100.00%> (ø)`
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs	`79.48% <0.00%> (-20.52%)`	⬇️
src/Microsoft.ML.Core/Data/ProgressReporter.cs	`70.95% <0.00%> (-6.99%)`	⬇️
...soft.ML.Transforms/Text/WordEmbeddingsExtractor.cs	`85.74% <0.00%> (-1.14%)`	⬇️
... and 10 more

eerhardt · 2021-05-12T14:29:45Z

docs/samples/Microsoft.ML.AutoML.Samples/Microsoft.ML.AutoML.Samples.csproj


    <NativeAssemblyReference Include="MatrixFactorizationNative" />
-    <NativeAssemblyReference Include="FastTreeNative" />
+    <NativeAssemblyReference Condition="'$(TargetArchitecture)' != 'arm64' And '$(TargetArchitecture)' != 'arm'" Include="FastTreeNative" />


I wonder if we can create a helper property in Directory.Build.props $(TargetsArm) or similar. Then we don't need this duplicated everywhere.

Yep, good idea. I'll go ahead and do that.

Ok I have added this. Take a look and see if it was what you were thinking. @safern can you take a look as well since you know msbuild really well? Directory.Build.props lines 30 - 37.

eng/common/cross/build-rootfs.sh

src/Microsoft.ML.Console/Microsoft.ML.Console.csproj

eerhardt · 2021-05-12T14:33:39Z

src/Microsoft.ML.FastTree/Microsoft.ML.FastTree.csproj

    <PackageDescription>ML.NET component for FastTree</PackageDescription>
-    <DefineConstants>$(DefineConstants);USE_FASTTREENATIVE;NO_STORE;CORECLR</DefineConstants>
+    <DefineConstants>$(DefineConstants);NO_STORE;CORECLR</DefineConstants>
+    <DefineConstants Condition="'$(TargetArchitecture)' != 'arm64' And '$(TargetArchitecture)' != 'arm'">$(DefineConstants);USE_FASTTREENATIVE</DefineConstants>


I don't think this is going to work. We only put 1 managed Microsoft.ML.FastTree.dll in our NuGet package.

We will either need to build the C# code twice, and include each in a RID-specific section of the NuGet package.

Or we will need to continue building the C# code once, and then have a switch at runtime whether we are running on ARM or not.

Good point. I think that having the runtime switch is better. Instead of checking if we are running on arm or not I'll just check if the native dll is present or not. That way even blazer would work with this.

Alright, I have added a runtime switch for this. It uses a delegate to pick between the native/managed version so it shouldn't have any performance impact, but if you could take a look I would appreciate it. I am gonig to run the benchmark tests before/after as well.

So the benchmarks are basically identical speedwise before and after these changes. I had our benchmark run 30 times and the mean of the current code and the new code are within .15 seconds of each other on my local machine. So there shouldn't be noticeable performance impact from doing it this way.

src/Native/MatrixFactorizationNative/CMakeLists.txt

src/Native/build.sh

Directory.Build.props

src/Microsoft.ML.Console/Microsoft.ML.Console.csproj

src/Microsoft.ML.FastTree/Dataset/IntArray.cs

test/Microsoft.ML.Tests/Microsoft.ML.Tests.csproj

test/Microsoft.Extensions.ML.Tests/Microsoft.Extensions.ML.Tests.csproj

test/Microsoft.ML.AutoML.Tests/Microsoft.ML.AutoML.Tests.csproj

eerhardt · 2021-05-17T19:30:02Z

Directory.Build.targets


-		<Copy SourceFiles = "@(NativeAssemblyReference->'%(FullAssemblyPath)')"
+    <PropertyGroup>
+      <ShouldCopyx64>false</ShouldCopyx64>


I'm not sure I understand the value of these properties. What other target architectures do we have beside x86, x64, arm64, or arm?

These properties appear to only be used in the below Copy, and as far as I can tell, that condition is always true.

Oh, I see now, this is to support copying LdaNative and MatrixFactorizationNative assemblies on arm, but not the other native files.

Correct. I figured we could also use this same pattern when we get to blazer WASM. Though I guess we can't really copy any native files there... so maybe this was a bit overkill.

Directory.Build.props

src/Microsoft.ML.FastTree/Dataset/IntArray.cs

…tecture

eerhardt · 2021-05-17T22:41:12Z

src/Native/gen-buildsys-win.bat

-"%CMakePath%" "-DCMAKE_BUILD_TYPE=%CMAKE_BUILD_TYPE%" "-DCMAKE_INSTALL_PREFIX=%__CMakeBinDir%" "-DMKL_LIB_PATH=%MKL_LIB_PATH%" -G "Visual Studio %__VSString%" %__ExtraCmakeParams% -B. -H%1
+if /i "%3" == "arm64"     (set __ExtraCmakeParams=%__ExtraCmakeParams% -A arm64)
+if /i "%3" == "arm"     (set __ExtraCmakeParams=%__ExtraCmakeParams% -A arm)
+"%CMakePath%" "-DCMAKE_BUILD_TYPE=%CMAKE_BUILD_TYPE%" "-DCMAKE_INSTALL_PREFIX=%__CMakeBinDir%" "-DMKL_LIB_PATH=%MKL_LIB_PATH%" "-DARCHITECTURE=%3" -G "Visual Studio %__VSString%" %__ExtraCmakeParams% -B. -H%1


Why do we need both -DARCHITECTURE and -A above? Can we just use 1 command line arg to set the archiecture?

So the -A doesn't work on the Unix Makefile Generator, but that is how you set the architecture correctly for visual studio. I needed a way in the cmakelists file to be able to exclude native projects for both generators, and thats how I ended up with the -DARCHITECTURE. I'm sure there is a way I could use only the -A in visual studio and not need the -DARCHITECTURE, I just haven't been able to figure it out yet. We will always need -DARCHITECTURE for the Unix generator, its just if we can remove it from the visual studio one.

Have you looked how it is done in other repos? For example dotnet/runtime?

I'm checking that out right now. They pass the -arch flag. Thats a custom flag. I'm still investigating how they plumb it internally.

That eventually turns into this /p:TargetArchitecture=$arch. Still looking into how thats passed to the native side of things.

So it actually looks like they do the same thing.
https://github.com/dotnet/runtime/blob/e4b4807e2fae2164d9116fbcdd49ba9044461e7e/eng/native/gen-buildsys.cmd#L34 they set the -A same way I do.

https://github.com/dotnet/runtime/blob/e4b4807e2fae2164d9116fbcdd49ba9044461e7e/src/coreclr/build-runtime.cmd#L450 they a flag -DCLR_CMAKE_TARGET_ARCH which is used the same why I am using the ARCHITECTURE flag. They do more complex stuff with it than we do since they target so many things, but its the same idea.

Sounds good. Thanks for verifying.

eerhardt

Just one minor question remaining. After answering that, this LGTM.

Nice work here!

* arm testing * initial commit with build working on arm64 * windows changes * build fixes for arm/arm64 with cross compilation * cross build instructions added * renamed arm to Arm. Changed TargetArchitecture to default to OS architecture * fixed some formatting * fixed capitilization * fixed Arm Capitilization * Fix cross-compilation if statement * building on apple silicon * removed non build related files * Changes from PR comments. Removal of FastTreeNative flag. * Changes from pr comments. * Fixes from PR comments. * Changed how we are excluding files.

…#5796) * Raised the limit of recursions in the creation of the CodedInputStream in the OnnxTransformer (as the default value in the Google.Protobuf). Otherwise some models cannot be loaded (ex. TF2 Efficentdet). * Updated arcade to the latest version (#5783) * updated arcade to the latest version * updated eng/common correctly * Fixed benchmark test. * Use dotnet certificate (#5794) * Use dotnet certificate * Update 3.1 SDK Co-authored-by: Prashanth Govindarajan <prgovi@microsoft.com> Co-authored-by: Michael Sharp <51342856+michaelgsharp@users.noreply.github.com> * Arm build changes (#5789) * arm testing * initial commit with build working on arm64 * windows changes * build fixes for arm/arm64 with cross compilation * cross build instructions added * renamed arm to Arm. Changed TargetArchitecture to default to OS architecture * fixed some formatting * fixed capitilization * fixed Arm Capitilization * Fix cross-compilation if statement * building on apple silicon * removed non build related files * Changes from PR comments. Removal of FastTreeNative flag. * Changes from pr comments. * Fixes from PR comments. * Changed how we are excluding files. * Onnx load model (#5782) * fixed onnx temp model deleting * random file path fixed * updates from pr * Changes from PR comments. * Changed how auto ml caches. * PR fixes. * Update src/Microsoft.ML.AutoML/API/ExperimentSettings.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> * Tensorflow fixes from PR comments * fixed filepath issues Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> Co-authored-by: Michael Sharp <51342856+michaelgsharp@users.noreply.github.com> Co-authored-by: Matt Mitchell <mmitche@microsoft.com> Co-authored-by: Prashanth Govindarajan <prgovi@microsoft.com> Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

michaelgsharp marked this pull request as ready for review May 11, 2021 17:15

michaelgsharp requested review from a team, eerhardt, ericstj, safern and tarekgh May 11, 2021 17:16

eerhardt reviewed May 12, 2021

View reviewed changes

eng/common/cross/build-rootfs.sh Show resolved Hide resolved

eerhardt reviewed May 12, 2021

View reviewed changes

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

eerhardt reviewed May 12, 2021

View reviewed changes

src/Native/MatrixFactorizationNative/CMakeLists.txt Outdated Show resolved Hide resolved

eerhardt reviewed May 12, 2021

View reviewed changes

src/Native/build.sh Outdated Show resolved Hide resolved

eerhardt reviewed May 14, 2021

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

eerhardt reviewed May 14, 2021

View reviewed changes

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

eerhardt reviewed May 14, 2021

View reviewed changes

src/Microsoft.ML.FastTree/Dataset/IntArray.cs Outdated Show resolved Hide resolved

eerhardt reviewed May 14, 2021

View reviewed changes

test/Microsoft.ML.Tests/Microsoft.ML.Tests.csproj Outdated Show resolved Hide resolved

eerhardt reviewed May 17, 2021

View reviewed changes

test/Microsoft.Extensions.ML.Tests/Microsoft.Extensions.ML.Tests.csproj Show resolved Hide resolved

eerhardt reviewed May 17, 2021

View reviewed changes

test/Microsoft.ML.AutoML.Tests/Microsoft.ML.AutoML.Tests.csproj Outdated Show resolved Hide resolved

eerhardt reviewed May 17, 2021

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

eerhardt reviewed May 17, 2021

View reviewed changes

src/Microsoft.ML.FastTree/Dataset/IntArray.cs Outdated Show resolved Hide resolved

eerhardt reviewed May 17, 2021

View reviewed changes

src/Microsoft.ML.FastTree/Dataset/IntArray.cs Outdated Show resolved Hide resolved

michaelgsharp added 7 commits May 17, 2021 13:56

arm testing

07acc68

initial commit with build working on arm64

b813fb9

windows changes

c945a05

build fixes for arm/arm64 with cross compilation

37ca2d7

cross build instructions added

745088b

renamed arm to Arm. Changed TargetArchitecture to default to OS archi…

a50ba08

…tecture

fixed some formatting

f277fbf

michaelgsharp added 8 commits May 17, 2021 13:56

fixed capitilization

266208a

fixed Arm Capitilization

3e27e67

Fix cross-compilation if statement

0b461b2

building on apple silicon

955c40b

removed non build related files

630e024

Changes from PR comments. Removal of FastTreeNative flag.

07e1fe0

Changes from pr comments.

73c3f0f

Fixes from PR comments.

629b4e6

michaelgsharp force-pushed the arm-build branch from abe8af4 to 629b4e6 Compare May 17, 2021 22:23

eerhardt reviewed May 17, 2021

View reviewed changes

eerhardt approved these changes May 17, 2021

View reviewed changes

Changed how we are excluding files.

b207b56

michaelgsharp merged commit bf31c94 into dotnet:main May 18, 2021

michaelgsharp deleted the arm-build branch May 18, 2021 16:19

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

Conversation

michaelgsharp commented May 10, 2021

Uh oh!

codecov bot commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented May 10, 2021 •

edited

Loading