-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Updating MKL #2867
Changes from all commits
022512d
39c8915
63ca59a
88c986f
719835b
6413c7a
8337e77
b2bde4f
24e65d3
c1b2565
dd99735
61822db
6cdad9d
3a9e990
6bcf592
b9ba70d
f4e528d
1b7f951
3247bde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,13 +33,13 @@ phases: | |
${{ insert }}: ${{ parameters.customMatrixes }} | ||
${{ insert }}: ${{ parameters.queue }} | ||
steps: | ||
- ${{ if eq(parameters.name, 'MacOS') }}: | ||
- script: brew update && brew install libomp && brew install mono-libgdiplus gettext && brew link gettext --force && brew link libomp --force | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
do we still need to install libomp if we install it in vsts-ci.yml? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. they are 2 completely separate builds:
|
||
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) | ||
|
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just put this in the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry - I should have been clearer:
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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No support for Linux and MacOS? #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" /> | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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