Update documentation for LightGBM and add missing binary references to console app. #452

codemzs · 2018-06-29T00:47:12Z

fixes #450
fixes #451

…d transform including native code to console command line binary.

eerhardt · 2018-06-29T01:20:55Z

src/Microsoft.ML.Console/Directory.Build.targets

+<Project>
+
+
+  <Target Name="CopyNativeAssemblies"


You can move this up to a common Directory.Build.targets instead of duplicating it with the test\Directory.Build.targets. #Resolved

eerhardt · 2018-06-29T01:22:40Z

test/Microsoft.ML.Tests/Scenarios/SentimentPredictionTests.cs

+
+            if (useLightGBM)
+            {
+                Assert.True(predictions.ElementAt(0).Sentiment.IsTrue);


This seems wrong. Why would we get different predictions using different algorithms? Wouldn't we want to get the same prediction? #Resolved

Not necessarily, they are different algorithms and results can vary. Even evaluation metrics should be different. I would be very surprised if the evaluation metrics were the same.

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

I would myself be utterly shocked if the two algorithms yielded identical predictions.

In reply to: 199031723 [](ancestors = 199031723,199031606)

eerhardt · 2018-06-29T01:23:49Z

src/Microsoft.ML.LightGBM/LightGbmRankingTrainer.cs

-        [TlcModule.EntryPoint(Name = "Trainers.LightGbmRanker", Desc = "Train an LightGBM ranking model", UserName = LightGbmRankingTrainer.Summary, ShortName = LightGbmRankingTrainer.ShortName)]
+        [TlcModule.EntryPoint(
+            Name = "Trainers.LightGbmRanker", 
+            Desc = "Train an LightGBM ranking model. This API requires Microsoft.ML.LightGBM nuget.", 


Train an LightGBM => Train a LightGBM #Resolved

TomFinley · 2018-06-29T01:55:46Z

src/Microsoft.ML.LightGBM/LightGbmRankingTrainer.cs

-        [TlcModule.EntryPoint(Name = "Trainers.LightGbmRanker", Desc = "Train an LightGBM ranking model", UserName = LightGbmRankingTrainer.Summary, ShortName = LightGbmRankingTrainer.ShortName)]
+        [TlcModule.EntryPoint(
+            Name = "Trainers.LightGbmRanker", 
+            Desc = "Train a LightGBM ranking model. This API requires Microsoft.ML.LightGBM nuget.", 


requires Microsoft.ML.LightGBM nuget [](start = 61, length = 36)

Entry-points are not just for the C# API. (Indeed, in a few months they won't be for the C# API at all.) This description will also show up in the other language bindings. This note is specific to .NET, so it does not belong here.

Rather, you must make the change in the C# API itself. Exploit the fact that these are partial classes. If you must put, the specific logic in the C# API generator. This is not the right place for it. #Resolved

TomFinley · 2018-06-29T01:57:13Z

src/Microsoft.ML.LightGBM/LightGbmRegressionTrainer.cs

-        [TlcModule.EntryPoint(Name = "Trainers.LightGbmRegressor", Desc = LightGbmRegressorTrainer.Summary, UserName = LightGbmRegressorTrainer.UserNameValue, ShortName = LightGbmRegressorTrainer.ShortName)]
+        [TlcModule.EntryPoint(
+            Name = "Trainers.LightGbmRegressor", 
+            Desc = LightGbmRegressorTrainer.Summary + ". This API requires Microsoft.ML.LightGBM nuget.", 


This API requires Microsoft.ML.LightGBM nuget.", [](start = 57, length = 48)

Same comment. #Resolved

TomFinley

🕐

Ivanidzo4ka · 2018-06-29T17:51:04Z

test/Microsoft.ML.Tests/Scenarios/SentimentPredictionTests.cs

@@ -163,38 +175,71 @@ public void CrossValidateSentimentModelTest()
            Assert.True(predictions.ElementAt(1).Sentiment.IsTrue);
        }

-        private void ValidateBinaryMetrics(BinaryClassificationMetrics metrics)
+        private void ValidateBinaryMetrics(BinaryClassificationMetrics metrics, bool useLightGBM = false)


, bool useLightGBM = false [](start = 78, length = 26)

This is really weird design.
Why can't you just have two different methods? #Resolved

This test will go away after a month when the old API is removed.

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

Will it go away, or be replaced with something calling the new API?

Are we saying we don't care about quality and comprehensibility of the tests over the existing API, because the plan is to do away with the existing API? Would that be a sensible plan given that we are still on the hook to support the old API?

I agree that this test looks "odd." It doesn't quite resemble any other test we have for learners, which will make the time when we want to migrate it to the new API harder to do. #Resolved

The test will have to be rewritten. There is no concept of Pipeline in the new API. I have just added an option to switch between LightGBM and FastTree trainer in a function that will need to be deleted. #Resolved

eerhardt · 2018-06-29T18:21:01Z

src/Microsoft.ML.Console/Microsoft.ML.Console.csproj


+    <NativeAssemblyReference Include="FastTreeNative" />
+    <NativeAssemblyReference Include="CpuMathNative" />
+    <NativeAssemblyReference Include="FactorizationMachineNative" />


What about LdaNative? https://github.com/dotnet/machinelearning/tree/master/src/Native/LdaNative. Is that necessary? #Resolved

Good catch. We will need that if someone uses that via command line.

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

…to console # Conflicts: # test/BaselineOutput/Common/EntryPoints/core_manifest.json

Ivanidzo4ka · 2018-06-29T20:06:15Z

src/Microsoft.ML/Trainers/LightGBM.cs

+using System.Text;
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.


it's usually header first, using second. #Resolved

Ivanidzo4ka · 2018-06-29T20:08:13Z

Microsoft.ML.Runtime.Tools.Console.Console

if you have desire to publish one more iteration, you can replace this tabs to spaces :) #Resolved

Refers to: src/Microsoft.ML.Console/Microsoft.ML.Console.csproj:9 in 2eccd02. [](commit_id = 2eccd02, deletion_comment = False)

revoking review

TomFinley · 2018-06-30T14:45:04Z

On the subject of fixes for LightGBM, I noticed in #392 there was a comment from @Ivanidzo4ka about the catalog not being updated. See here. Since you're fixing bugs or oversights in the initial checkin, now is also a good time to add the reference to LightGBM in that test project.

codemzs · 2018-06-30T16:47:54Z

Which test project are you referring to? This is already added to Microsoft.ML.Core.Tests.

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

TomFinley

Ivanidzo4ka

mjmckp · 2018-07-03T00:48:11Z

The LightGBM support looks very promising, are there any plans to leverage the GPU training features it has?

codemzs · 2018-07-03T18:10:53Z

@mjmckp Thanks but currently there is no GPU support because it requires binary to compiled with GPU support and then packaged into the nuget. @guolinke please correct me if I'm wrong.

mjmckp · 2018-07-03T22:13:18Z

The Microsoft CNTK nuget package is a fully self-contained package that includes the necessary GPU libraries, is there any reason the same couldn't be done here?

Failing that, can we add a configuration parameter to allow for (GPU enabled) locally built versions of lib_lightgbm.dll? I think that just amounts to adding adding the input argument device_type=gpu.

codemzs · 2018-07-03T22:31:35Z

@mjmckp check this and lets wait for @guolinke to reply because the nuget is produced from his end. In ML.NET passing device_type=gpu parameter is easy but native lightgbm libraries need to be GPU compliant.

…o console app. (dotnet#452) * Add LightGBM to the entrypoint manifest and references to trainers and transform including native code to console command line binary. * Add documentation to use LightGBM nuget for LightGBM APIs.

codemzs added 2 commits June 28, 2018 14:31

Add LightGBM to the entrypoint manifest and references to trainers an…

3f7f02a

…d transform including native code to console command line binary.

Add documentation to use LightGBM nuget for LightGBM APIs.

fb67580

codemzs requested review from GalOshri, TomFinley and eerhardt June 29, 2018 00:47

update test baselines.

2604bf3

eerhardt reviewed Jun 29, 2018

View reviewed changes

PR feedback.

194e171

TomFinley reviewed Jun 29, 2018

View reviewed changes

TomFinley previously requested changes Jun 29, 2018

View reviewed changes

Ivanidzo4ka reviewed Jun 29, 2018

View reviewed changes

eerhardt reviewed Jun 29, 2018

View reviewed changes

codemzs added 4 commits June 29, 2018 12:41

PR feedback.

d44fb65

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

d0eb457

…to console # Conflicts: # test/BaselineOutput/Common/EntryPoints/core_manifest.json

update baselines and sync to master/resolve conflicts.

599dc92

PR feedback.

2eccd02

Ivanidzo4ka reviewed Jun 29, 2018

View reviewed changes

PR feedback.

fb5026a

PR feedback.

958e22d

TomFinley approved these changes Jul 2, 2018

View reviewed changes

Ivanidzo4ka approved these changes Jul 2, 2018

View reviewed changes

codemzs merged commit 2d427bd into dotnet:master Jul 2, 2018

codemzs deleted the console branch July 2, 2018 20:51

shauheen mentioned this pull request Jul 5, 2018

Add options to enable use of GPU with LightGBM #492

Closed

sfilipi mentioned this pull request Oct 2, 2018

Create EntryPoint for LightGBM #465

Closed

torronen mentioned this pull request Jan 31, 2022

Q: Roadmap for LightGBM interface in .NET #6065

Closed

ghost locked as resolved and limited conversation to collaborators Mar 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update documentation for LightGBM and add missing binary references to console app. #452

Update documentation for LightGBM and add missing binary references to console app. #452

codemzs commented Jun 29, 2018

eerhardt Jun 29, 2018 •

edited by codemzs

Loading

eerhardt Jun 29, 2018 •

edited by codemzs

Loading

codemzs Jun 29, 2018

TomFinley Jun 29, 2018

eerhardt Jun 29, 2018 •

edited by codemzs

Loading

TomFinley Jun 29, 2018 •

edited by codemzs

Loading

TomFinley Jun 29, 2018 •

edited by codemzs

Loading

TomFinley left a comment

Ivanidzo4ka Jun 29, 2018 •

edited by codemzs

Loading

codemzs Jun 29, 2018

TomFinley Jun 30, 2018 •

edited by codemzs

Loading

codemzs Jun 30, 2018 •

edited

Loading

eerhardt Jun 29, 2018 •

edited by codemzs

Loading

codemzs Jun 29, 2018 •

edited

Loading

Ivanidzo4ka Jun 29, 2018 •

edited by codemzs

Loading

Ivanidzo4ka commented Jun 29, 2018 •

edited by codemzs

Loading

TomFinley commented Jun 30, 2018 •

edited

Loading

codemzs commented Jun 30, 2018

TomFinley left a comment

Ivanidzo4ka left a comment

mjmckp commented Jul 3, 2018

codemzs commented Jul 3, 2018 •

edited

Loading

mjmckp commented Jul 3, 2018

codemzs commented Jul 3, 2018

Update documentation for LightGBM and add missing binary references to console app. #452

Update documentation for LightGBM and add missing binary references to console app. #452

Conversation

codemzs commented Jun 29, 2018

eerhardt Jun 29, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

eerhardt Jun 29, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

codemzs Jun 29, 2018

Choose a reason for hiding this comment

TomFinley Jun 29, 2018

Choose a reason for hiding this comment

eerhardt Jun 29, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

TomFinley Jun 29, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

TomFinley Jun 29, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

TomFinley left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Jun 29, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

codemzs Jun 29, 2018

Choose a reason for hiding this comment

TomFinley Jun 30, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

codemzs Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

eerhardt Jun 29, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

codemzs Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Jun 29, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Jun 29, 2018 • edited by codemzs Loading

TomFinley commented Jun 30, 2018 • edited Loading

codemzs commented Jun 30, 2018

TomFinley left a comment

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

mjmckp commented Jul 3, 2018

codemzs commented Jul 3, 2018 • edited Loading

mjmckp commented Jul 3, 2018

codemzs commented Jul 3, 2018

eerhardt Jun 29, 2018 •

edited by codemzs

Loading

eerhardt Jun 29, 2018 •

edited by codemzs

Loading

eerhardt Jun 29, 2018 •

edited by codemzs

Loading

TomFinley Jun 29, 2018 •

edited by codemzs

Loading

TomFinley Jun 29, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Jun 29, 2018 •

edited by codemzs

Loading

TomFinley Jun 30, 2018 •

edited by codemzs

Loading

codemzs Jun 30, 2018 •

edited

Loading

eerhardt Jun 29, 2018 •

edited by codemzs

Loading

codemzs Jun 29, 2018 •

edited

Loading

Ivanidzo4ka Jun 29, 2018 •

edited by codemzs

Loading

Ivanidzo4ka commented Jun 29, 2018 •

edited by codemzs

Loading

TomFinley commented Jun 30, 2018 •

edited

Loading

codemzs commented Jul 3, 2018 •

edited

Loading