Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 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-df8e152-20190312232803

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

@eerhardt eerhardt Mar 6, 2019

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.

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

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

thanks - ill update that as well. #Resolved

- script: brew update && brew install libomp && brew link libomp --force
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
- script: brew update && brew install mono-libgdiplus gettext && brew link gettext --force

@eerhardt eerhardt Mar 13, 2019

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.

Why split this into 2 steps? Why not just brew install all the stuff in one step? #Resolved

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 I was differentiating between the build dependency and runtime dependencies. Its minor - so I will move to a single step.


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

displayName: Install runtime dependencies
- script: $(_buildScript) -- /t:DownloadExternalTestFiles /p:IncludeBenchmarkData=$(_includeBenchmarkData)
displayName: Download Benchmark Data
- 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-df8e152-20190312232803

phases:
################################################################################
Expand Down Expand Up @@ -49,6 +49,8 @@ phases:
- agent.os -equals Darwin
steps:
# Only build native assets to avoid conflicts.

@eerhardt eerhardt Mar 13, 2019

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 comment should go below your new script - it applies to the ./build.sh line. #Closed

- script: brew update && brew install libomp && brew link libomp --force
displayName: Install build dependencies
- 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 @@ -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;

@Ivanidzo4ka Ivanidzo4ka Mar 15, 2019

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

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
25 changes: 23 additions & 2 deletions src/Native/SymSgdNative/CMakeLists.txt
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

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

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

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

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 @@ -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)
2 changes: 1 addition & 1 deletion src/Native/SymSgdNative/SymSgdNative.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -486,4 +486,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

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

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

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