Skip to content
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

Jump to bottom

Updating MKL #2867

Merged
merged 19 commits into from
Mar 18, 2019
Merged

Updating MKL #2867

Show file tree
Hide file tree
Changes from 14 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
4 changes: 2 additions & 2 deletions .vsts-dotnet-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
resources:
containers:
- container: CentosContainer
image: microsoft/dotnet-buildtools-prereqs:centos-7-b46d863-20180719033416
image: mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7-mlnet-8bba86b-20190314145033

- container: UbuntuContainer
image: microsoft/dotnet-buildtools-prereqs:ubuntu-16.04-10fcdcf-20190208200917
image: mcr.microsoft.com/dotnet-buildtools/prereqs:ubuntu-16.04-mlnet-207e097-20190312152303

phases:
- template: /build/ci/phase-template.yml
Expand Down
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
6 changes: 3 additions & 3 deletions build/ci/phase-template.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,13 @@ phases:
${{ insert }}: ${{ parameters.customMatrixes }}
${{ insert }}: ${{ parameters.queue }}
steps:
- ${{ if eq(parameters.name, 'MacOS') }}:
Copy link
Member

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

Choose a reason for hiding this comment

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

Note - you are going to need to update the "official" build as well:

machinelearning/build/vsts-ci.yml

Lines 37 to 60 in 6f4e055

################################################################################
- phase: MacOS
################################################################################
variables:
BuildConfig: Release
OfficialBuildId: $(BUILD.BUILDNUMBER)
DOTNET_CLI_TELEMETRY_OPTOUT: 1
DOTNET_SKIP_FIRST_TIME_EXPERIENCE: 1
DOTNET_MULTILEVEL_LOOKUP: 0
queue:
name: DotNetCore-Build
demands:
- agent.os -equals Darwin
steps:
# Only build native assets to avoid conflicts.
- script: ./build.sh -buildNative -$(BuildConfig) -skipRIDAgnosticAssets
displayName: Build
- task: PublishBuildArtifacts@1
displayName: Publish macOS package assets
inputs:
pathToPublish: $(Build.SourcesDirectory)/bin/obj/packages
artifactName: PackageAssets
artifactType: container
#Resolved

Copy link
Member Author

@singlis singlis Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

thanks - ill update that as well. #Resolved

- script: brew update && brew install libomp && brew install mono-libgdiplus gettext && brew link gettext --force && brew link libomp --force
Copy link
Contributor

Choose a reason for hiding this comment

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

libomp [](start = 44, length = 6)

do we still need to install libomp if we install it in vsts-ci.yml?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe it does as Eric's comment earlier was to make sure to also add this to vsts-ci.yml. My impression is that the vsts-ci.yml is for the gated build (post merge) and vsts-ci.yml is separate from phase-template.yml, but its a good question. @eerhardt can you clarify? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

they are 2 completely separate builds:

  1. For PR validation and post merge CI.
  2. For official build - which signs assemblies, publishes nuget packages and symbols, etc.

displayName: Install build dependencies
- script: $(_buildScript) -$(_configuration) -buildArch=$(_arch)
displayName: Build
- script: $(_buildScript) -- /t:DownloadExternalTestFiles /p:IncludeBenchmarkData=$(_includeBenchmarkData)
displayName: Download Benchmark Data
- ${{ if eq(parameters.name, 'MacOS') }}:
- script: brew update && brew install libomp mono-libgdiplus gettext && brew link gettext --force
displayName: Install runtime dependencies
- script: $(_buildScript) -$(_configuration) -runtests -coverage=$(_codeCoverage)
displayName: Run Tests.
- script: $(Build.SourcesDirectory)/Tools/dotnetcli/dotnet msbuild build/Codecoverage.proj /p:CodeCovToken=$(CODECOV_TOKEN)
Expand Down
4 changes: 3 additions & 1 deletion build/vsts-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
resources:
containers:
- container: CentosContainer
image: microsoft/dotnet-buildtools-prereqs:centos-7-b46d863-20180719033416
image: mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7-mlnet-8bba86b-20190314145033

phases:
################################################################################
Expand Down Expand Up @@ -48,6 +48,8 @@ phases:
demands:
- agent.os -equals Darwin
steps:
- script: brew update && brew install libomp && brew link libomp --force
displayName: Install build dependencies
# Only build native assets to avoid conflicts.
- script: ./build.sh -buildNative -$(BuildConfig) -skipRIDAgnosticAssets
displayName: Build
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 @@ -664,6 +664,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;
Copy link
Contributor

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

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

Copy link
Member Author

@singlis singlis Mar 15, 2019 •
edited
Loading

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
25 changes: 23 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)
Copy link
Member

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

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

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

Copy link
Member

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

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

Copy link
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 @@ -15,11 +35,12 @@ if(NOT WIN32)
endif()
endif()

add_definitions(-DUSE_OMP)
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)
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)"
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 6, 2019 •
edited
Loading

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

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