Port SymSGD trainer #624

codemzs · 2018-08-01T12:07:40Z

This change adds parallel SGD trainer but disables its multi-threading capabilities because of the lack of OpenMP support by the Clang compiler on linux and macOS build systems and MKL library. It also Supersedes PR #556 by squashing all the commits in one and rebasing with master.

fixes #623

eerhardt · 2018-08-01T13:54:42Z

src/Native/build.sh

@@ -98,3 +98,7 @@ set -x # turn on trace
 cmake "$DIR" -G "Unix Makefiles" $__cmake_defines
 set +x # turn off trace
 make install
+if [[ "$OSTYPE" == "darwin"* ]]; then
+    echo "Changing libMklImports.dylib's executable path within libSymSgdNative.dylib so that loader can find it."
+    install_name_tool "-change" @loader_path/libMklImports.dylib @rpath/libMklImports.dylib "$RootRepo"/bin/x64."$__configuration"/Native/libSymSgdNative.dylib


Will this work on an end-user's machine, when they use our NuGet packages? #Resolved

Note: you can do this stuff inside of CMake too. It may make more sense there, since you won't have to do things like "$RootRepo"/bin/x64."$__configuration"/Native. Plus then it can also be localized in the SymSgdNative\CmakeLists.txt, and this high-level build script doesn't need to know about individual assemblies we are building.

Here's an example: https://github.com/dotnet/corefx/blob/20bcc468b12119e12428cfd9cce50ef5d5b9cc13/src/Native/System.Security.Cryptography.Native/CMakeLists.txt#L56-L70

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

Just double-checking, would the NuGet work?

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

We spoke about it offline and the insall_name_tool step can moved out of the script and be done when nuget is created. I'll test the nuget as end user during bug-bash.

In reply to: 206964367 [](ancestors = 206964367,206888376)

eerhardt · 2018-08-01T14:01:42Z

test/Microsoft.ML.Predictor.Tests/TestPredictors.cs

+        [TestCategory("Binary")]
+        public void BinaryClassifierSymSgdTest()
+        {
+            //Results sometimes go out of error tolerance on OS X.


Can we open an issue to enable this test on macOS? #Resolved

We can but linear learners tend to differ on results. #Resolved

At the end of the day, we still need test coverage even if the results differ. Just because running on a different OS produces different results/behavior doesn't mean we should test on that OS. #Resolved

The other test of SymSGD is enabled on macOS too. I can enable this as well just that roughly half the time it will fail because of error tolerance. #Resolved

Just open an issue saying that we should enable this test on macOS. Then when we work on that issue, we can make it stable as well. #Resolved

Will do.

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

eerhardt · 2018-08-01T14:06:12Z

test/Microsoft.ML.TestFramework/Microsoft.ML.TestFramework.csproj

@@ -11,6 +11,7 @@
    <ProjectReference Include="..\..\src\Microsoft.ML.FastTree\Microsoft.ML.FastTree.csproj" />
    <ProjectReference Include="..\..\src\Microsoft.ML.Maml\Microsoft.ML.Maml.csproj" />
    <ProjectReference Include="..\..\src\Microsoft.ML.ResultProcessor\Microsoft.ML.ResultProcessor.csproj" />
+    <ProjectReference Include="..\..\src\Microsoft.ML.StandardLearners\Microsoft.ML.StandardLearners.csproj" />


Is this change necessary now that SymSGD isn't in the StandardLearners project? #Resolved

eerhardt · 2018-08-01T14:08:20Z

src/Microsoft.ML/CSharpApi.cs

+            /// <summary>
+            /// Tolerance for difference in average loss in consecutive passes.
+            /// </summary>
+            public float Tol { get; set; } = 0.0001f;


This should probably be a complete word: Tolerance. #Resolved

eerhardt · 2018-08-01T14:10:13Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

+            public int NumberOfIterations = 50;
+
+            [Argument(ArgumentType.AtMostOnce, HelpText = "Tolerance for difference in average loss in consecutive passes.", ShortName = "tol")]
+            public float Tol = 1e-4f;


Tol => Tolerance? #Resolved

eerhardt · 2018-08-01T14:33:19Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

+
+            static Native()
+            {
+                if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))


It may be nice to comment why this is only needed on Windows, and what makes it work on the other platforms. #Resolved

eerhardt · 2018-08-01T14:34:34Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

+        /// This is the state of a SymSGD learner that is shared between the managed and native code.
+        /// </summary>
+        [StructLayout(LayoutKind.Explicit)]
+        public unsafe struct State


Does this need to be public? I think it should be able to be made internal. #Resolved

eerhardt · 2018-08-01T14:38:48Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

+        public unsafe struct State
+        {
+#pragma warning disable 649 // never assigned
+            [FieldOffset(0x00)]


Why the need to explicitly list the offsets here? This forces us to never work on non-64-bit platforms.

Instead, we should be able to use [StructLayout(Sequential)] and then just list the fields as necessary.

For example: https://github.com/dotnet/corefx/blob/ba496ae6a9ae055708b630b3f73395a76c2ce6ce/src/System.Drawing.Common/src/System/Drawing/Internal/GpPathData.cs#L9-L15 #Resolved

Given this is a port it is probably out of scope to make this kind of change without also involving the authors of sysmsgd, instead I will open an issue and assign it to them. Does that sound good? #Resolved

That works for me. #Resolved

eerhardt · 2018-08-01T14:51:21Z

src/Native/SymSgdNative/SymSgdNative.cpp

+
+// This method learns for a single instance
+inline void LearnInstance(int instSize, int* instIndices, float* instValues,
+	float label, float alpha, float l2Const, float piw, float& weightScaling, float* weightVector, float& bias) {


(nit) It would be nice to follow the coding style already established in the repo and put the opening { on a new line:

inline void LearnInstance(int instSize, int* instIndices, float* instValues, float label, float alpha, float l2Const, float piw, float& weightScaling, float* weightVector, float& bias) { //code } ``` #Resolved

eerhardt · 2018-08-01T15:00:06Z

src/Native/SymSgdNative/SymSgdNative.cpp

+			state->PassIteration++;
+		}
+	} else {
+#if defined(USE_OMP) 


What happens in this case if someone passes in numThreads > 1? Does it just no op? #Resolved

yep. We have defined as part of arguments that multi-threaded is not supported.

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

eerhardt · 2018-08-01T15:01:35Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

+            ch.CheckUserArg(numThreads > 0, nameof(_args.NumberOfThreads),
+                "The number of threads must be either null or a positive integer.");
+
+            ch.Assert(numThreads > 0);


Is this redundant with the line just before it? #Resolved

sfilipi · 2018-08-01T15:29:59Z

src/Microsoft.ML.HalLearners/Microsoft.ML.HalLearners.csproj


  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
    <IncludeInPackage>Microsoft.ML.HalLearners</IncludeInPackage>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>


true [](start = 3, length = 44)

think i removed this #Resolved

I need it for symsgd.

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

sfilipi · 2018-08-01T15:36:05Z

src/Native/SymSgdNative/SymSgdNative.cpp

+						for (int featIndex = 0; featIndex < numFeat; featIndex++)
+							weightVector[featIndex] *= weightScaling;
+						if (threadId == 0)
+							weightScaling = 1.0f;


[](start = 0, length = 6)

tabs in this file. #Resolved

sfilipi

revoking review

eerhardt · 2018-08-01T17:23:41Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

+        {
+            int numFeatures = data.Schema.Feature.Type.VectorSize;
+            var cursorFactory = new FloatLabelCursor.Factory(data, CursOpt.Label | CursOpt.Features | CursOpt.Weight);
+            int numThreads = 1;


Do we have an issue tracking turning NumberOfThreads back on? #Resolved

No, I will open it. We will need to enable this once openmp support comes in.

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

eerhardt · 2018-08-01T17:32:10Z

src/Native/SymSgdNative/CMakeLists.txt

+        find_library(MKL_LIBRARY libMklImports.so HINTS ${CMAKE_SOURCE_DIR}/../../packages/mlnetmkldeps/0.0.0.5/runtimes/linux-x64/native)
+        SET(CMAKE_SKIP_BUILD_RPATH  FALSE)
+        SET(CMAKE_BUILD_WITH_INSTALL_RPATH FALSE)
+        SET(CMAKE_INSTALL_RPATH "${CMAKE_SOURCE_DIR}/../../packages/mlnetmkldeps/0.0.0.5/runtimes")


I'm concerned that this may work for unit tests, but it may not work on an end-user's machine. We will have to test this as an end user to verify. #Resolved

yep. I will make sure to do this as part of bug bash.

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

eerhardt · 2018-08-01T17:33:21Z

src/Native/build.proj

@@ -90,6 +90,8 @@
                          RelativePath="Microsoft.ML\runtimes\$(PackageRid)\native" />
      <NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)FactorizationMachineNative$(NativeLibExtension)"
                          RelativePath="Microsoft.ML\runtimes\$(PackageRid)\native" />
+      <NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)SymSgdNative$(NativeLibExtension)"
+                          RelativePath="Microsoft.ML\runtimes\$(PackageRid)\native" />


This native binary shouldn't go in the Microsoft.ML nuget package. Instead, it needs to go in the Microsoft.ML.HalLearners nuget package. #Resolved

eerhardt · 2018-08-01T17:42:14Z

src/Native/build.proj

@@ -90,6 +90,8 @@
                          RelativePath="Microsoft.ML\runtimes\$(PackageRid)\native" />
      <NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)FactorizationMachineNative$(NativeLibExtension)"
                          RelativePath="Microsoft.ML\runtimes\$(PackageRid)\native" />
+      <NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)SymSgdNative$(NativeLibExtension)"
+                          RelativePath="Microsoft.Hal\runtimes\$(PackageRid)\native" />


The real name here is: Microsoft.ML.HalLearners #Resolved

It was on accident.

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

eerhardt

Looks good. After this gets merged, let's make sure this will work for our end users on all 3 platforms.

sfilipi

codemzs added 2 commits August 1, 2018 05:00

Port SymSGD trainer.

edb343b

Port SymSGD trainer.

c5fdb85

codemzs requested review from eerhardt and sfilipi August 1, 2018 12:07

codemzs added 2 commits August 1, 2018 05:15

cleanup.

4e01743

misc.

d3e5527

eerhardt reviewed Aug 1, 2018

View reviewed changes

sfilipi reviewed Aug 1, 2018

View reviewed changes

sfilipi previously approved these changes Aug 1, 2018

View reviewed changes

PR feedback.

5207356

eerhardt reviewed Aug 1, 2018

View reviewed changes

PR feedback.

a6a6a56

eerhardt reviewed Aug 1, 2018

View reviewed changes

PR feedback.

68945ae

eerhardt approved these changes Aug 1, 2018

View reviewed changes

sfilipi approved these changes Aug 1, 2018

View reviewed changes

codemzs merged commit a7a15ff into dotnet:master Aug 1, 2018

eerhardt mentioned this pull request Aug 1, 2018

Fix official build #626

Merged

codemzs deleted the symsgd-clean branch August 3, 2018 06:03

antoniovs1029 mentioned this pull request Feb 1, 2020

Enable Conditional Numerical Reproducibility for tests #4569

Merged

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

Port SymSGD trainer #624

Port SymSGD trainer #624

Uh oh!

Conversation

codemzs commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

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 Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codemzs Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codemzs Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codemzs Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 1, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codemzs commented Aug 1, 2018 •

edited

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

codemzs Aug 1, 2018 •

edited

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

codemzs Aug 1, 2018 •

edited

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

codemzs Aug 1, 2018 •

edited

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

sfilipi Aug 1, 2018 •

edited by codemzs

Loading

sfilipi Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading

eerhardt Aug 1, 2018 •

edited by codemzs

Loading