Skip to content

Updating MKL #2867

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
singlis merged 19 commits into from
Mar 18, 2019
Merged

Updating MKL #2867

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
022512d
Updating ml.net to use the mlnetdep 0.0.0.9 nuget package.
singlis Mar 5, 2019
39c8915
- Enabled NumberOfThreads parameter
singlis Mar 6, 2019
63ca59a
- Update for mac build
singlis Mar 6, 2019
88c986f
- Forcing libomp to create a link
singlis Mar 6, 2019
719835b
Merge remote-tracking branch 'origin/master' into singlis/mkl-update
singlis Mar 6, 2019
6413c7a
Apple test for building...
singlis Mar 6, 2019
8337e77
- Moving the install of libomp to happen before build for Apple
singlis Mar 6, 2019
b2bde4f
- Updating from feedback
singlis Mar 6, 2019
24e65d3
Merge remote-tracking branch 'origin/master' into singlis/mkl-update
singlis Mar 13, 2019
c1b2565
- Updating machine images
singlis Mar 13, 2019
dd99735
- Updating docker images
singlis Mar 13, 2019
61822db
Merge remote-tracking branch 'origin/master' into singlis/mkl-update
singlis Mar 14, 2019
6cdad9d
- Updating centos container, addressing feedback
singlis Mar 14, 2019
3a9e990
-fixing yml file.
singlis Mar 14, 2019
6bcf592
-Minor cmakefile cleanup
singlis Mar 15, 2019
b9ba70d
Merge remote-tracking branch 'origin/master' into singlis/mkl-update
singlis Mar 15, 2019
f4e528d
- NumberOfThreads defaults to Environment.ProcessorCount, comment upd…
singlis Mar 15, 2019
1b7f951
- Setting SymSSGD test to use 1 thread for consistency
singlis Mar 16, 2019
3247bde
Merge remote-tracking branch 'origin/master' into singlis/mkl-update
singlis Mar 18, 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
2 changes: 1 addition & 1 deletion build/Dependencies.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<GoogleProtobufPackageVersion>3.5.1</GoogleProtobufPackageVersion>
<LightGBMPackageVersion>2.2.3</LightGBMPackageVersion>
<MicrosoftMLOnnxRuntimePackageVersion>0.2.1</MicrosoftMLOnnxRuntimePackageVersion>
<MlNetMklDepsPackageVersion>0.0.0.7</MlNetMklDepsPackageVersion>
<MlNetMklDepsPackageVersion>0.0.0.9</MlNetMklDepsPackageVersion>
<ParquetDotNetPackageVersion>2.1.3</ParquetDotNetPackageVersion>
<SystemDrawingCommonPackageVersion>4.5.0</SystemDrawingCommonPackageVersion>
<SystemIOFileSystemAccessControl>4.5.0</SystemIOFileSystemAccessControl>
Expand Down
5 changes: 4 additions & 1 deletion src/Microsoft.ML.Console/Microsoft.ML.Console.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,11 @@
<NativeAssemblyReference Include="FastTreeNative" />
<NativeAssemblyReference Include="CpuMathNative" />
<NativeAssemblyReference Include="FactorizationMachineNative" />
<NativeAssemblyReference Include="MatrixFactorizationNative" />
<NativeAssemblyReference Include="LdaNative" />
<NativeAssemblyReference Include="SymSgdNative" />
<NativeAssemblyReference Include="SymSgdNative"/>
<NativeAssemblyReference Include="MklImports"/>
<NativeAssemblyReference Condition="'$(OS)' == 'Windows_NT'" Include="libiomp5md"/>
</ItemGroup>

</Project>
4 changes: 4 additions & 0 deletions src/Microsoft.ML.Mkl.Components/SymSgdClassificationTrainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,10 @@ private TPredictor TrainCore(IChannel ch, RoleMappedData data, LinearModelParame
int numFeatures = data.Schema.Feature.Value.Type.GetVectorSize();
var cursorFactory = new FloatLabelCursor.Factory(data, CursOpt.Label | CursOpt.Features);
int numThreads = 1;

if (_options.NumberOfThreads.HasValue)
numThreads = _options.NumberOfThreads.Value;

@Ivanidzo4ka Ivanidzo4ka Mar 15, 2019 •
edited by singlis
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

numThreads [](start = 16, length = 10)

So we now can have multithreading for SymSGD always?
If yes:

  /// <summary>
            /// Degree of lock-free parallelism. Determinism not guaranteed if this is set to higher than 1.
            /// Multi-threading is not supported currently.
            /// </summary>
            [Argument(ArgumentType.AtMostOnce, HelpText = "Degree of lock-free parallelism. Determinism not guaranteed. " +
                "Multi-threading is not supported currently.", ShortName = "nt")]
            public int? NumberOfThreads;

this need some polishing in description, but that minor.

So if it's null (which it is by default) it's would be shame to utilize only one thread.

numThreads = NumberOfThreads ?? Environment.ProcessorCount maybe? #Resolved

@singlis singlis Mar 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for bringing this up - I had similar thoughts and actually meant to bring this up in the PR. I agree - - null should default to the number of threads that are available to openmp. Im happy to update this... #Resolved


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

Expand Down
24 changes: 22 additions & 2 deletions src/Native/SymSgdNative/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,30 @@
project (SymSgdNative)


set(SOURCES
SymSgdNative.cpp
)

if(APPLE)

@eerhardt eerhardt Mar 15, 2019 •
edited by singlis
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we just put this in the if (NOT WIN32) section below? Instead of having 2 different "If not windows" sections? #Resolved

@singlis singlis Mar 15, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hi Eric, sorry I am not quite following. This has special handling for APPLE - adding this under "NOT WIN32" would include both apple and linux which is not the same result. There is also the ELSE clause which is what I want to execute for windows and linux. If I am misunderstanding let me know.

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

@eerhardt eerhardt Mar 15, 2019 •
edited by singlis
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry - I should have been clearer:

if(APPLE)
    # CMake has support for OpenMP, however, Apple has a version of Clang
    # that does not support openMP out of the box. Therefore
    # these commands are added to sepcifically handle the Apple Clang scenario
    # If the LLVM version of clang is used for Apple builds, this can be removed
    # and the else condition can be used instead.
    SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Xpreprocessor -fopenmp")
    SET(OPENMP_LIBRARY "omp")
    include_directories("/usr/local/opt/libomp/include")
    link_directories("/usr/local/opt/libomp/lib")
else()
    find_package(OpenMP)
    if (OPENMP_FOUND)
        SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}")
        SET(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${OpenMP_EXE_LINKER_FLAGS}")
    endif()
endif()

find_library(MKL_LIBRARY MklImports HINTS ${MKL_LIB_PATH})

if(NOT WIN32)
    list(APPEND SOURCES ${VERSION_FILE_PATH})
    if(NOT APPLE)
        SET(CMAKE_SKIP_BUILD_RPATH  FALSE)
        SET(CMAKE_BUILD_WITH_INSTALL_RPATH FALSE)
        SET(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE)
        SET(CMAKE_INSTALL_RPATH "$ORIGIN/")
    endif()
endif()

You see how there are now 2 separate "if" checks for not Windows and Not Apple?

We can combine this together into a single set of "if" conditions:

if(NOT WIN32)
    // Do common non-Windows
    if(APPLE)
        // Apple stuff
    else()
        // Do non-Apple stuff
    endif()
endif()
``` #Resolved

@singlis singlis Mar 15, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Here is what I came up with -- its merged with the least amount of duplication. The line that was duplicated is this: list(APPEND SOURCES ${VERSION_FILE_PATH})

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

# CMake has support for OpenMP, however, Apple has a version of Clang
# that does not support openMP out of the box. Therefore
# these commands are added to sepcifically handle the Apple Clang scenario
# If the LLVM version of clang is used for Apple builds, this can be removed
# and the else condition can be used instead.
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Xpreprocessor -fopenmp")
SET(OPENMP_LIBRARY "omp")
include_directories("/usr/local/opt/libomp/include")
link_directories("/usr/local/opt/libomp/lib")
else()
find_package(OpenMP)
if (OPENMP_FOUND)
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}")
SET(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${OpenMP_EXE_LINKER_FLAGS}")
endif()
endif()

find_library(MKL_LIBRARY MklImports HINTS ${MKL_LIB_PATH})

if(NOT WIN32)
list(APPEND SOURCES ${VERSION_FILE_PATH})
if(NOT APPLE)
Expand All @@ -16,10 +36,10 @@ if(NOT WIN32)
endif()

add_library(SymSgdNative SHARED ${SOURCES} ${RESOURCES})
target_link_libraries(SymSgdNative PUBLIC ${MKL_LIBRARY})
target_link_libraries(SymSgdNative PUBLIC ${MKL_LIBRARY} PUBLIC ${OPENMP_LIBRARY})

if(APPLE)
set_target_properties(SymSgdNative PROPERTIES INSTALL_RPATH "@loader_path;@loader_path/${MKL_LIB_RPATH}}")
endif()

install_library_and_symbols (SymSgdNative)
install_library_and_symbols (SymSgdNative)
10 changes: 1 addition & 9 deletions src/Native/SymSgdNative/SymSgdNative.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@

#include <vector>
#include <random>
#if defined(USE_OMP)
#include <omp.h>
#endif
#include <unordered_map>
#include <string>
#include "../Stdafx.h"
Expand Down Expand Up @@ -240,7 +238,6 @@ void InitializeState(int totalNumInstances, int* instSizes, int** instIndices, f
TuneNumLocIter(numLocIter, totalNumInstances, instSizes, numThreads);

state->WeightScaling = 1.0f;
#if defined(USE_OMP)

@Ivanidzo4ka Ivanidzo4ka Mar 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#if defined(USE_OMP) [](start = 0, length = 21)

why it's going away? #Closed

@eerhardt eerhardt Mar 6, 2019 •
edited by singlis
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we should just define USE_OMP in the CMake files instead? #Resolved

@singlis singlis Mar 6, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm fine with adding back and adding the USE_OMP to CMakefile file.

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

if (numThreads > 1)
{
state->PassIteration = 0;
Expand Down Expand Up @@ -268,7 +265,6 @@ void InitializeState(int totalNumInstances, int* instSizes, int** instIndices, f

// To make sure that MKL runs sequentially
omp_set_num_threads(1);
#endif
}

float Loss(int instSize, int* instIndices, float* instValues,
Expand Down Expand Up @@ -361,7 +357,6 @@ EXPORT_API(void) LearnAll(int totalNumInstances, int* instSizes, int** instIndic
}
} else
{
#if defined(USE_OMP)
// In parallel case...
bool shouldRemap = !((std::unordered_map<int, int>*)state->FreqFeatUnorderedMap)->empty();
SymSGD** learners = (SymSGD**)(state->Learners);
Expand Down Expand Up @@ -448,7 +443,6 @@ EXPORT_API(void) LearnAll(int totalNumInstances, int* instSizes, int** instIndic
}
}
state->TotalInstancesProcessed += numPasses*totalNumInstances;
#endif
}
}

Expand All @@ -471,10 +465,8 @@ EXPORT_API(void) MapBackWeightVector(float* weightVector, SymSGDState* state)
// Deallocation method
EXPORT_API(void) DeallocateSequentially(SymSGDState* state)
{
#if defined(USE_OMP)
// To make sure that for the rest of MKL calls use parallelism
omp_set_num_threads(omp_get_num_procs());
#endif

SymSGD** learners = (SymSGD**)(state->Learners);
if (learners)
Expand All @@ -486,4 +478,4 @@ EXPORT_API(void) DeallocateSequentially(SymSGDState* state)
delete (std::unordered_map<int, int>*)state->FreqFeatUnorderedMap;
if (state->FreqFeatDirectMap)
delete[] state->FreqFeatDirectMap;
}
}

@eerhardt eerhardt Mar 13, 2019 •
edited by singlis
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change can be reverted. No reason to change this file. #Resolved

5 changes: 4 additions & 1 deletion src/Native/build.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,10 @@

<Copy SourceFiles="$(PackagesDir)mlnetmkldeps\$(MlNetMklDepsPackageVersion)\runtimes\$(PackageRid)\native\$(NativeLibPrefix)MklImports$(NativeLibExtension)"
DestinationFolder="$(NativeAssetsBuiltPath)" />


<Copy Condition="'$(OS)' == 'Windows_NT'" SourceFiles="$(PackagesDir)mlnetmkldeps\$(MlNetMklDepsPackageVersion)\runtimes\$(PackageRid)\native\libiomp5md$(NativeLibExtension)"

@Ivanidzo4ka Ivanidzo4ka Mar 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Windows_NT [](start = 33, length = 10)

No support for Linux and MacOS? #Closed

@singlis singlis Mar 6, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So this works on all platforms. For Linux and MacOS, we rely on openmp to be installed on the system.

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

DestinationFolder="$(NativeAssetsBuiltPath)" />

<ItemGroup Condition="'$(UseIntrinsics)' != 'true'">
<NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)CpuMathNative$(NativeLibExtension)"
RelativePath="Microsoft.ML.CpuMath\runtimes\$(PackageRid)\native" />
Expand Down
3 changes: 2 additions & 1 deletion test/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,6 @@
<NativeAssemblyReference Include="CpuMathNative" />
<NativeAssemblyReference Include="FastTreeNative" />
<NativeAssemblyReference Include="MklImports" />
<NativeAssemblyReference Condition="'$(OS)' == 'Windows_NT'" Include="libiomp5md"/>
</ItemGroup>
</Project>
</Project>
3 changes: 2 additions & 1 deletion test/Microsoft.ML.Core.Tests/Microsoft.ML.Core.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,12 @@
</ItemGroup>

<ItemGroup>
<NativeAssemblyReference Include="MklImports" />
<NativeAssemblyReference Include="MklProxyNative" />
<NativeAssemblyReference Include="CpuMathNative" />
<NativeAssemblyReference Include="FastTreeNative" />
<NativeAssemblyReference Include="LdaNative" />
<NativeAssemblyReference Include="MklImports" />
<NativeAssemblyReference Condition="'$(OS)' == 'Windows_NT'" Include="libiomp5md"/>
</ItemGroup>

<!-- TensorFlow is 64-bit only -->
Expand Down
1 change: 1 addition & 0 deletions test/Microsoft.ML.Functional.Tests/Microsoft.ML.Functional.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
<NativeAssemblyReference Include="SymSgdNative" />
<NativeAssemblyReference Include="MklProxyNative" />
<NativeAssemblyReference Include="MklImports" />
<NativeAssemblyReference Condition="'$(OS)' == 'Windows_NT'" Include="libiomp5md"/>
</ItemGroup>

<!-- TensorFlow is 64-bit only -->
Expand Down
1 change: 1 addition & 0 deletions test/Microsoft.ML.OnnxTransformerTest/Microsoft.ML.OnnxTransformerTest.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

<ItemGroup>
<NativeAssemblyReference Include="MklImports" />
<NativeAssemblyReference Condition="'$(OS)' == 'Windows_NT'" Include="libiomp5md"/>
</ItemGroup>

</Project>
1 change: 1 addition & 0 deletions test/Microsoft.ML.Predictor.Tests/Microsoft.ML.Predictor.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
<NativeAssemblyReference Include="FactorizationMachineNative" />
<NativeAssemblyReference Include="SymSgdNative" />
<NativeAssemblyReference Include="MklImports" />
<NativeAssemblyReference Condition="'$(OS)' == 'Windows_NT'" Include="libiomp5md"/>
</ItemGroup>

</Project>
1 change: 1 addition & 0 deletions test/Microsoft.ML.StaticPipelineTesting/Microsoft.ML.StaticPipelineTesting.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,6 @@
<NativeAssemblyReference Include="FastTreeNative" />
<NativeAssemblyReference Include="LdaNative" />
<NativeAssemblyReference Include="MklImports" />
<NativeAssemblyReference Condition="'$(OS)' == 'Windows_NT'" Include="libiomp5md"/>
</ItemGroup>
</Project>
1 change: 1 addition & 0 deletions test/Microsoft.ML.Sweeper.Tests/Microsoft.ML.Sweeper.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@
<NativeAssemblyReference Include="FastTreeNative" />
<NativeAssemblyReference Include="CpuMathNative" />
<NativeAssemblyReference Include="MklImports" />
<NativeAssemblyReference Condition="'$(OS)' == 'Windows_NT'" Include="libiomp5md"/>
</ItemGroup>
</Project>
3 changes: 2 additions & 1 deletion test/Microsoft.ML.TestFramework/Microsoft.ML.TestFramework.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@
<NativeAssemblyReference Include="LdaNative" />
<NativeAssemblyReference Include="FastTreeNative" />
<NativeAssemblyReference Include="CpuMathNative" />
<NativeAssemblyReference Include="MklImports" />
<NativeAssemblyReference Include="MklProxyNative" />
<NativeAssemblyReference Include="FactorizationMachineNative" />
<NativeAssemblyReference Include="MatrixFactorizationNative" />
<NativeAssemblyReference Include="MklImports" />
<NativeAssemblyReference Condition="'$(OS)' == 'Windows_NT'" Include="libiomp5md"/>
</ItemGroup>
</Project>
1 change: 1 addition & 0 deletions test/Microsoft.ML.Tests/Microsoft.ML.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
<NativeAssemblyReference Include="SymSgdNative" />
<NativeAssemblyReference Include="MklProxyNative" />
<NativeAssemblyReference Include="MklImports" />
<NativeAssemblyReference Condition="'$(OS)' == 'Windows_NT'" Include="libiomp5md"/>
</ItemGroup>

<!-- TensorFlow is 64-bit only -->
Expand Down
3 changes: 2 additions & 1 deletion test/Microsoft.ML.TimeSeries.Tests/Microsoft.ML.TimeSeries.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@
<NativeAssemblyReference Include="CpuMathNative" />
<NativeAssemblyReference Include="MklProxyNative" />
<NativeAssemblyReference Include="MklImports" />
<NativeAssemblyReference Condition="'$(OS)' == 'Windows_NT'" Include="libiomp5md"/>
</ItemGroup>
</Project>
</Project>