Skip to content

Adding the MlNetMklDeps package, adding the entry point, tests and docs for OLS. #578

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

Closed
wants to merge 9 commits into from

Conversation

sfilipi
Copy link
Member

@sfilipi sfilipi commented Jul 24, 2018

MKL package has been published

@sfilipi sfilipi requested review from Ivanidzo4ka and eerhardt July 24, 2018 17:24
@@ -0,0 +1,5 @@
<Project DefaultTargets="Pack">

<Import Project="Microsoft.ML.Parquet.nupkgproj" />
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Jul 24, 2018

Choose a reason for hiding this comment

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

Parquet [](start = 32, length = 7)

nope #Resolved

@codemzs
Copy link
Member

codemzs commented Jul 24, 2018

Have you tested MKL works as expected? #Resolved

</PropertyGroup>

<ItemGroup>
<ProjectReference Include="../Microsoft.ML/Microsoft.ML.nupkgproj" />
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Jul 24, 2018

Choose a reason for hiding this comment

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

Can we write down how exactly it's gonna work?
How it looks in my head based on conversations (can be far away from reality)

  1. Have a internal repo with pre build custom dlls for each OS.
  2. Publish them to nuget.org
  3. Reference them in Microsoft.ML nuget as dependency

this steps have nothing to do with this package file, so I obviously don't understand something.

#Resolved

@@ -8,5 +8,6 @@
<SystemReflectionEmitLightweightPackageVersion>4.3.0</SystemReflectionEmitLightweightPackageVersion>
<PublishSymbolsPackageVersion>1.0.0-beta-62824-02</PublishSymbolsPackageVersion>
<LightGBMPackageVersion>2.1.2.2</LightGBMPackageVersion>
<MlNetMklDepsPackageVersion>0.0.0.1</MlNetMklDepsPackageVersion>
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Jul 24, 2018

Choose a reason for hiding this comment

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

[](start = 0, length = 1)

ze evil tabs! #Resolved

@TomFinley
Copy link
Contributor

TomFinley commented Jul 24, 2018

Hi @sfilipi thanks for getting this started.

I see we have a package definition added, I think. But I might have expected the package definition to live somewhere else in a different repository somehow (since it cannot be part of the build here), and for components in ML.Net that need it to simply reference that externally defined nuget. Is that unfair?

I also feel like I might understand this better if we saw how existing component (like OLS) or upcoming ones (like #556) will actually consume this. #Resolved

@codemzs
Copy link
Member

codemzs commented Jul 24, 2018

Hi @[email protected] ,I have tested MKL binaries by linking them with native SymSGD code in PR556 (that iteration is not posted yet) using CMAKE. The way we would do is by restoring MKL nuget before CMAKE is called by adding an external project to the sln file that package references MKL nuget, this step fetches the binaries to a known location on the drive and then we can link against the binaries in our CMAKE file.

<Project Include="Microsoft.ML.sln" />
(this line is executed before BuildNative is called)


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

@codemzs codemzs mentioned this pull request Jul 26, 2018
sfilipi added 2 commits July 26, 2018 09:31
Adding Mkl dependency to the StandartLearners project
Enabling the Ols tests, adding its EntryPoint, and documentation.
</remarks>
<example>
<code language="csharp">
new StochasticDualCoordinateAscentRegressor
Copy link
Member Author

@sfilipi sfilipi Jul 26, 2018

Choose a reason for hiding this comment

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

new StochasticDualCoordinateAscentRegressor [](start = 9, length = 44)

I'll fix this. #Resolved

sfilipi added 3 commits July 26, 2018 14:08
…ureEntryPointModule so that the OLS EntryPoint is visible to the Module Catalog.

Updating the CSharpApi and the ep-list and manifest.
Fixing the test in the documentation
@sfilipi sfilipi changed the title WIP: adding the MlNetMklDeps package Adding the MlNetMklDeps package, adding the entry point, tests and docs for OLS. Jul 27, 2018
sfilipi added 3 commits July 27, 2018 14:13
…ual prediction is outside of the calculated range.

since we have seen quite a bit of those, set a minimum threeshold, and ignore fluctuations after the 5th decimal point.
@sfilipi
Copy link
Member Author

sfilipi commented Jul 30, 2018

The changes went in through the other PR.

@sfilipi sfilipi closed this Jul 30, 2018
@sfilipi sfilipi deleted the mkldeps branch July 30, 2018 17:26
@ghost ghost locked as resolved and limited conversation to collaborators Mar 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants