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.

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

Updating MKL #2867

Merged
merged 19 commits into from
Mar 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
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
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
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

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:

################################################################################
- 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

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
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
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>
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public sealed class Options : TrainerInputBaseWithLabel
{
/// <summary>
/// Degree of lock-free parallelism. Determinism not guaranteed if this is set to higher than 1.
/// Multi-threading is not supported currently.
/// The default value is the number of logical cores that are available on the system.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Degree of lock-free parallelism. Determinism not guaranteed. " +
"Multi-threading is not supported currently.", ShortName = "nt")]
Expand Down Expand Up @@ -662,7 +662,8 @@ 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;
int numThreads = _options.NumberOfThreads ?? Environment.ProcessorCount;

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

Expand Down
31 changes: 26 additions & 5 deletions src/Native/SymSgdNative/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,25 +1,46 @@
project (SymSgdNative)


set(SOURCES
SymSgdNative.cpp
)

find_library(MKL_LIBRARY MklImports HINTS ${MKL_LIB_PATH})
if(NOT WIN32)
if(APPLE)
Copy link
Member

@eerhardt eerhardt Mar 15, 2019

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

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")

list(APPEND SOURCES ${VERSION_FILE_PATH})
if(NOT APPLE)
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()

if (NOT WIN32)
eerhardt marked this conversation as resolved.
Show resolved Hide resolved
list(APPEND SOURCES ${VERSION_FILE_PATH})
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()

find_library(MKL_LIBRARY MklImports HINTS ${MKL_LIB_PATH})

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

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
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
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
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
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>
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>
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>
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>
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>
12 changes: 10 additions & 2 deletions test/Microsoft.ML.Tests/FeatureContributionTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,22 @@ public void TestSDCABinary()
public void TestSGDBinary()
{
TestFeatureContribution(ML.BinaryClassification.Trainers.SgdCalibrated(
new SgdCalibratedTrainer.Options { NumberOfThreads = 1 }),
new SgdCalibratedTrainer.Options()
{
NumberOfThreads = 1
}),
GetSparseDataset(TaskType.BinaryClassification, 100), "SGDBinary");
}

[Fact]
public void TestSSGDBinary()
{
TestFeatureContribution(ML.BinaryClassification.Trainers.SymbolicSgd(), GetSparseDataset(TaskType.BinaryClassification, 100), "SSGDBinary", 4);
TestFeatureContribution(ML.BinaryClassification.Trainers.SymbolicSgd(
new SymbolicSgdTrainer.Options()
{
NumberOfThreads = 1
}),
GetSparseDataset(TaskType.BinaryClassification, 100), "SSGDBinary", 4);
}

[Fact]
Expand Down
1 change: 1 addition & 0 deletions test/Microsoft.ML.Tests/Microsoft.ML.Tests.csproj
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
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>