Uh oh!
There was an error while loading. Please reload this page.
Port SymSGD - #556
Conversation
| #include "../Stdafx.h" | ||
| #include "mkl.h" | ||
| #ifndef COMPILER_GCC | ||
| #pragma comment(lib, "../../../Libraries/MKL/Win/Microsoft.ML.MklImports.lib") |
There was a problem hiding this comment.
Libraries/MKL/Win/Microsoft.ML.MklImports.lib [](start = 31, length = 45)
Hi @codemzs thanks for looking at this. We need to either shift this thing to not use MKL, or port the libraries. However, considering that this involves the static linking of the library, and all other usages are using it as a dynamic library, this becomes especially awkward here. #Resolved
There was a problem hiding this comment.
I wonder if temporarily (or not so temporarily) we can shift to using regular ops rather than calling MKL, based on some compile time flag (as we do elsewhere). If on, then it will compile with MKL support. If off, it does it manually. Since these are ops like SAXPYI and whatnot, it's not like they would be incredibly hard to support. #Resolved
There was a problem hiding this comment.
Hi @TomFinley , There is a PR out that creates the MKL nuget. I have tested the native symSGD code by linking against the MKL binaries assuming they are present in the packages directory. #Resolved
There was a problem hiding this comment.
OK. Once that PR is in, assuming it works, you would add the nuget dependency in which project?
In reply to: 205300173 [](ancestors = 205300173)
There was a problem hiding this comment.
Microsoft.ML.Tests and Microsoft.ML.Predictor.Test. Anything that uses sym sgd would need the reference to the nuget so that binaries are copied over to the project folder in bin directory.
In reply to: 205467167 [](ancestors = 205467167,205300173)
There was a problem hiding this comment.
| link_directories(${CMAKE_SOURCE_DIR}/../../packages/MlNetMklDeps/runtimes/win-x64/native) | ||
| else() | ||
| list(APPEND SOURCES ${VERSION_FILE_PATH}) | ||
| if(APPLE) |
There was a problem hiding this comment.
[](start = 0, length = 1)
Consistent indentation please. #Resolved
| class SymSGD { | ||
| private: | ||
| int _numFreqFeat; | ||
| // Local models that is learned |
There was a problem hiding this comment.
[](start = 0, length = 1)
Please shift these to use spaces for indentation. #Resolved
| <NativeAssemblyReference Include="FastTreeNative" /> | ||
| <NativeAssemblyReference Include="CpuMathNative" /> | ||
| <NativeAssemblyReference Include="FactorizationMachineNative" /> | ||
| <NativeAssemblyReference Include="SymSgdNative" /> |
There was a problem hiding this comment.
[](start = 0, length = 1)
Yet another place where I see tabs... #Resolved
Ivanidzo4ka
commented
Jul 26, 2018
You probably want new version of SymSGD with respectful changes in ITrainer. Which can be found in PR towards internal repo |
@Ivanidzo4ka This PR consumes the latest ITrainer. The code would have not compiled and tests would have not run otherwise, right? Is there anything specific that you are referring to? In reply to: 408173274 [](ancestors = 408173274) |
From what I discover yesterday, you can't just take this code, and update internal repository. SymSgd internally is in separate package, and visibility of LinearTrainerBase don't allow it to be referenced from other projects. In reply to: 408181017 [](ancestors = 408181017,408173274) |
| <NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)FactorizationMachineNative$(NativeLibExtension)" | ||
| RelativePath="Microsoft.ML\runtimes\$(PackageRid)\native" /> | ||
| <NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)SymSgdNative$(NativeLibExtension)" | ||
| RelativePath="Microsoft.ML\runtimes\$(PackageRid)\native" /> |
There was a problem hiding this comment.
[](start = 86, length = 8)
something, something, tabs #Resolved
| // See the LICENSE file in the project root for more information. | ||
| #pragma once | ||
| #define MAX(__X__, __Y__) (((__X__) < (__Y__)) ? (__Y__) : (__X__)) |
There was a problem hiding this comment.
MAX never get used, do we need it? #Resolved
| @@ -0,0 +1,127 @@ | |||
| #pragma once | |||
codemzs
commented
Jul 26, 2018
I don't follow, can you please comment on SymSgdClassificationTrainer.cs and clarify? In reply to: 408182382 [](ancestors = 408182382,408181017,408173274) |
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="MlNetMklDeps" Version="0.0.0.1" /> |
There was a problem hiding this comment.
Correct. We shouldn't be referencing MlNetMklDeps from the Microsoft.ML nuget package.
| { | ||
| return; | ||
| } | ||
| using (var env = new TlcEnvironment()) |
There was a problem hiding this comment.
maybe log an issue about re-enabling.
There was a problem hiding this comment.
Why are we disabling existing tests when we are adding new functionality? That seems wrong.
| RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
same, might be worth logging an issue.
| /// <summary> | ||
| /// Train a symbolic SGD. | ||
| /// </summary> |
There was a problem hiding this comment.
Not sure how much documentation we have about SymSGD. If any, might be nice to include it.
| ) | ||
| if(WIN32) | ||
| find_library(MKL_LIBRARY Microsoft.ML.MklImports HINTS ${CMAKE_SOURCE_DIR}/../../packages/mlnetmkldeps/0.0.0.1/runtimes/win-x64/native) |
There was a problem hiding this comment.
Like we discussed offline, we shouldn't be hard-coding these paths and version numbers into our CMake scripts.
| find_library(MKL_LIBRARY Microsoft.ML.MklImports.dylib HINTS "${CMAKE_SOURCE_DIR}/../../packages/mlnetmkldeps/0.0.0.1/runtimes/osx-x64/native") | ||
| else() | ||
| message("Linking SymSgdNative with MKL on linux.") | ||
| link_directories(${CMAKE_SOURCE_DIR}/../../packages/mlnetmkldeps/0.0.0.1/runtimes/linux-x64/native) |
There was a problem hiding this comment.
Why do you need both this and the HINTS below? Isn't one of them enough?
| goto :Failure | ||
| :BuildNativeProj | ||
| echo Copying MKL library in bin folder. This is a temporary fix. |
| cd "$__IntermediatesDir" | ||
| #codemzs: temporary fix until mkl nuget binaries are properly renamed so that they can be consumed by CMAKE. |
There was a problem hiding this comment.
We should also remove this. And the one below.
| <NativeAssemblyReference Include="CpuMathNative" /> | ||
| <NativeAssemblyReference Include="FastTreeNative" /> | ||
| <NativeAssemblyReference Include="LdaNative" /> | ||
| <NativeAssemblyReference Include="SymSgdNative" /> |
| </ItemGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="MlNetMklDeps" Version="0.0.0.1" /> |
There was a problem hiding this comment.
0.0.0.1 - this version number should go in 1 place - in the https://github.com/dotnet/machinelearning/blob/master/build/Dependencies.props file.
* Allow CpuMath to reference C# Hardware Intrinsics APIs. Need to multi-target CpuMath for netstandard and netcoreapp3.0. Also, since we are going to move CpuMath into its own NuGet package, remove the dependency from CpuMath to the ML.Core project. Add a build parameter to enable building against .NET Core 3.0's Runtime Intrinsics APIs. Fixdotnet#534 * Respond to PR feedback.
* failing test case for multiclass * Refactored PipelineSweeperSupportedMetrics Class; added unit test for MultiClassClassification; refactored out unit tests for the PipelineSweeper * take care of review comments; display transforms/learners + metrics in pipeline * taking care of PR comments + refactor PipelineSweeperRunSummary * taking care of review comments
* remove domain from onnx operators for non-ML types. * Make ONNX compatible with Windows RS5 and add more tests. * PR feedback. * PR feedback. * fix build.
* Remove Windows and Linux configurations from netci.groovy * Add end of line to yml files * Add badges and change leg name to Linux * Not merge test results * Add searchFolder to publish test results task
add proper fields to trainer info for FT, FM, and OnlineLearner regarding incremental training and validation datasets
* Fix warning for L2 in SDCA * String interpolation * Word-smithing the warning message
Introduce word embedding transform
Add images support based on System.Drawing
…#611) * moving Ols to a separate project * Revert "moving Ols to a separate project" This reverts commit 9b7eab3. * separating OLS in its own project * adding nupkgproj files to create a package for AdditionalLearners adding AdditionalLearners to the core.tests project fixing the core_ep and core_manifest * CSharpApi should not get generated every time. * Addressing Ivan's comments * referencing package version 0.0.0.4 of MlNetMklDeps that contains new names for the mkl library. * Correcting the error message. * renaming AdditionalLearners to HalLearners < - Hardware Accelerated Learners removign unsafe from the Hal csproj. removing the orphaned member section from the doc.xml * referencing package 0.0.0.5 and updating the name. * regenerating the CsharpApi and the eplist post merge * regenerate the CSharpApi and the eplists post merge. Fix the namespace post merge. * typo * one shall not space * spacing
…to symsgd # Conflicts: # src/Microsoft.ML.HalLearners/Microsoft.ML.HalLearners.csproj # src/Microsoft.ML.HalLearners/doc.xml # src/Microsoft.ML/CSharpApi.cs # test/BaselineOutput/Common/EntryPoints/core_manifest.json
eerhardt
commented
Aug 1, 2018
This can be closed. We merged the other PR |
This change adds parallel SGD trainer.
fixes#623