Added numeric ranking Performance Tests - #888

Merged
justinormont merged 10 commits into
dotnet:masterfrom
Anipik:NumericRanking
Sep 19, 2018
Merged

Added numeric ranking Performance Tests#888
justinormont merged 10 commits into
dotnet:masterfrom
Anipik:NumericRanking

Conversation

@Anipik

@AnipikAnipik commented Sep 11, 2018

Copy link
Copy Markdown
Contributor

Added benchmarking performance tests for Numeric ranking.

cc @justinormont@sfilipi @danmosemsft @eerhardt@shauheen

@Anipik

Anipik commented Sep 11, 2018

Copy link
Copy Markdown
ContributorAuthor
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 72.7866 s, StdErr = 0.5849 s (0.80%); N = 20, StdDev = 2.6156 sMin = 68.6174 s, Q1 = 70.0802 s, Median = 73.3494 s, Q3 = 74.9717 s, Max = 77.5199 s
IQR = 4.8915 s, LowerFence = 62.7429 s, UpperFence = 82.3089 s
ConfidenceInterval = [70.5153 s; 75.0579 s] (CI 99.9%), Margin = 2.2713 s (3.12% of Mean)Skewness = -0.09, Kurtosis = 1.66, MValue = 3.14
-------------------- Histogram --------------------
[68.616 s ; 70.533 s) | @@@@@@
[70.533 s ; 72.219 s) | @@
[72.219 s ; 74.846 s) | @@@@@@@
[74.846 s ; 76.379 s) | @@@@
[76.379 s ; 78.363 s) | @
---------------------------------------------------
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 65.4654 s, StdErr = 0.9955 s (1.52%); N = 20, StdDev = 4.4522 sMin = 58.3025 s, Q1 = 62.0859 s, Median = 65.7175 s, Q3 = 68.5847 s, Max = 75.8618 s
IQR = 6.4988 s, LowerFence = 52.3377 s, UpperFence = 78.3329 s
ConfidenceInterval = [61.5993 s; 69.3315 s] (CI 99.9%), Margin = 3.8661 s (5.91% of Mean)Skewness = 0.1, Kurtosis = 2.64, MValue = 3.25
-------------------- Histogram --------------------
[58.224 s ; 61.721 s) | @@@@@
[61.721 s ; 64.643 s) | @
[64.643 s ; 67.513 s) | @@@@@@@@
[67.513 s ; 70.412 s) | @@@@@
[70.412 s ; 74.427 s) |
[74.427 s ; 77.297 s) | @
---------------------------------------------------
Ranking.Test_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 4.5715 s, StdErr = 0.0079 s (0.17%); N = 14, StdDev = 0.0295 sMin = 4.5317 s, Q1 = 4.5526 s, Median = 4.5653 s, Q3 = 4.5778 s, Max = 4.6321 s
IQR = 0.0252 s, LowerFence = 4.5147 s, UpperFence = 4.6157 s
ConfidenceInterval = [4.5382 s; 4.6047 s] (CI 99.9%), Margin = 0.0333 s (0.73% of Mean)Skewness = 0.67, Kurtosis = 2.39, MValue = 2
-------------------- Histogram --------------------
[4.524 s ; 4.643 s) | @@@@@@@@@@@@@@
---------------------------------------------------
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree72.787 s2.2713 s2.6156 s-6247000.00001058000.0000286000.000026171.31 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM65.465 s3.8661 s4.4522 s-3595000.00001684000.0000267000.0000304.59 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree4.571 s0.0333 s0.0295 s-558000.0000279000.00001000.000011.93 MB

eerhardt
eerhardt reviewed Sep 12, 2018
View reviewed changes
Comment threadbuild.proj Outdated
eerhardt
eerhardt reviewed Sep 12, 2018
View reviewed changes
Comment threadbuild.proj Outdated
</ItemGroup>

<ItemGroup Condition="'$(IncludeBenchmarkData)' == 'true'" >
<TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/WikiDetoxAnnotated160kRows.tsv"

@eerhardteerhardtSep 12, 2018•
edited
Loading

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.

The duplication here could be simplified using MSBuild. Something along the lines of:

<ItemGroup>
<TlcResourceFileInclude="WikiDetoxAnnotated160kRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTrain3.6MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KValidate1.2MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTest1.2MRows.tsv" />
<TlcResourceFileUpdate="@(TlcResourceFile)">
<Url>http://aka.ms/tlc-resources/benchmarks/%(Identity)</Url>
<DestinationFile>$(MSBuildThisFileDirectory)test/data/external/%(Identity)</DestinationFile>
</TlcResourceFile>
<TestFileInclude="@(TlcResourceFile->'$(MSBuildThisFileDirectory)/test/data/external/%(Identity)')" />
</ItemGroup>

I'm not 100% sure it is better, but it reduces the number of times these URLs need to be copied.

@danmoseleydanmoseleySep 12, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's batching, I think it would only work within a <Target> ?

@AnipikAnipikSep 12, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yep dan is right. its not working in this case. any other suggestion here ?

@eerhardteerhardtSep 12, 2018

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.

It works, you just need to have the right syntax. I've updated the above with actual MSBuild code that works.

@AnipikAnipikSep 12, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Okay thanks :)

eerhardt
eerhardt reviewed Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs Outdated
eerhardt
eerhardt reviewed Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs Outdated
eerhardt
eerhardt reviewed Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs Outdated
@Anipik

Anipik commented Sep 12, 2018

Copy link
Copy Markdown
ContributorAuthor
BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.228 (1803/April2018Update/Redstone4)
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=2.1.400
[Host] : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Job-QFXMOR : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree32.993 s0.5025 s0.4455 s-2762000.0000192000.000056000.000015435.34 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM31.045 s2.1895 s2.5215 s-1198000.0000560000.000084000.0000246.64 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree1.153 s0.0943 s0.1086 s-122000.000055000.000012000.00002.93 MB

@justinormont
justinormont self-requested a review September 12, 2018 19:35
@justinormontjustinormont mentioned this pull request Sep 12, 2018
Closed
justinormont
justinormont reviewed Sep 12, 2018
View reviewed changes
Comment threadbuild.proj Outdated
justinormont
justinormont reviewed Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs Outdated
sfilipi
sfilipi reviewed Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj Outdated
Include="..\data\external\WikiDetoxAnnotated160kRows.tsv"
Link="external\WikiDetoxAnnotated160kRows.tsv">

<TlcResourceFile Update="@(TlcResourceFile)">

@sfilipisfilipiSep 13, 2018

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.

TlcResourceFile [](start = 31, length = 15)

call it something non TLC

@eerhardteerhardtSep 13, 2018

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.

Agreed, but the reason I chose the name originally is because of the URL:

http://aka.ms/tlc-resources

Can we change this URL? Or make a new aka.ms URL pointing to the same location with a different name?

sfilipi
sfilipi approved these changes Sep 13, 2018
View reviewed changes

@sfilipisfilipi left a comment

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.

:shipit:

justinormont
justinormont approved these changes Sep 13, 2018
View reviewed changes

@justinormontjustinormont left a comment

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.

LGTM.

Let's wait to merge until the MSLR-WEB10K dataset is available in the CDN.

eerhardt
eerhardt reviewed Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj Outdated
eerhardt
eerhardt reviewed Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj Outdated
eerhardt
eerhardt approved these changes Sep 13, 2018
View reviewed changes

@eerhardteerhardt left a comment

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.

:shipit:

@justinormont

justinormont commented Sep 13, 2018

Copy link
Copy Markdown
Contributor

Please also add the citation to the MSLR-WEB10K dataset.

justinormont
justinormont reviewed Sep 14, 2018
View reviewed changes
Comment threadbuild.proj Outdated
justinormont
justinormont reviewed Sep 14, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs Outdated

namespace Microsoft.ML.Benchmarks
{
[WarmupCount(8)] // It helps to reduce the standard deviation of these tests.

@justinormontjustinormontSep 14, 2018•
edited
Loading

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.

The normal user is unlikely to pre-train a model 8 times before training their model. This will be representative of the steady state reached when a model is retrained many times, but not very representative of the normal user's interaction w/ ML.net.

Do we know what's causing the time difference between the first run and the later runs? The first run is most representative of what a normal user will experience.

@AnipikAnipikSep 14, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@adamsitnik can you give us a better view here ? Increasing the warmup iterations leads to reducing the standard deviation here

@AnipikAnipikSep 14, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@justinormont do u want me to reduce it ?

cc @danmosemsft

@adamsitnikadamsitnikSep 17, 2018•
edited
Loading

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 you give us a better view here ?

@Anipik Unfortunately, it's not that simple. To find out why given benchmark behaves differently for different warmup counts we would have to profile it. It could be that OS gets warmed up and reading the input files becomes faster or anything like that.

The normal user is unlikely to pre-train a model 8 times before training their model.

@justinormont I agree. In that case we should set the WarmupCount to 0, IterationCount to 1 and LaunchCount to 20. Which means that BenchmarkDotNet is going to start a new process 20 times and each time execute the benchmark only once, without any warmup (the real use case) and just exit the process.

Edit: we should most probably have two configs: one for training benchmarks (no warmups) and one for prediction benchmarks (the one we have today)

@AnipikAnipikSep 17, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I will revert warmupCount to 1 for this PR to get merged, we can later follow up with 2 config files as adam suggested

justinormont
justinormont reviewed Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md Outdated
justinormont
justinormont reviewed Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md Outdated
justinormont
justinormont suggested changes Sep 14, 2018
View reviewed changes

@justinormontjustinormont left a comment •
edited
Loading

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.

Looks good, though there's a couple minor things:

eerhardt
eerhardt reviewed Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md Outdated
justinormont
justinormont reviewed Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md Outdated
@Anipik

Anipik commented Sep 17, 2018

Copy link
Copy Markdown
ContributorAuthor

@justinormont cam you take a look here ?

justinormont
justinormont reviewed Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md Outdated
justinormont
justinormont reviewed Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md Outdated
@Anipik

Anipik commented Sep 18, 2018

Copy link
Copy Markdown
ContributorAuthor

@justinormont can you merge this ? I don't have the write access to the repo ?

@justinormont

justinormont commented Sep 18, 2018

Copy link
Copy Markdown
Contributor

I'm going to close & re-open this pull request to notify the CI to re-check this PR.

@justinormont

justinormont commented Sep 18, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

The CI test are failing. Though GitHub says 'in progress', it will say failed soon.

The winequality-white.csv file is the cause.

Error:

2018-09-18T17:27:13.4451915Z System.IO.IOException : Could not find file 'D:\a\1\s\test\data\external\winequality-white.csv'
2018-09-18T17:27:13.4452129Z Stack Trace:
2018-09-18T17:27:13.4452368Z at Microsoft.ML.Runtime.Data.MultiFileSource..ctor(String path) in D:\a\1\s\src\Microsoft.ML.Data\DataLoadSave\MultiFileSource.cs:line 31
2018-09-18T17:27:13.4452691Z at Microsoft.ML.StaticPipelineTesting.Training.SdcaRegression() in D:\a\1\s\test\Microsoft.ML.StaticPipelineTesting\Training.cs:line 29
2018-09-18T17:27:13.4452938Z 2018-09-18T17:27:13.4453436Z Results File: D:\a\1\s\bin/AnyCPU.Release\Microsoft.ML.StaticPipelineTesting\VssAdministrator_factoryvm-az385_2018-09-18_17_27_12.trx
2018-09-18T17:27:13.4454016Z 2018-09-18T17:27:13.4454366Z Total tests: 13. Passed: 9. Failed: 4. Skipped: 0.

@Anipik

Anipik commented Sep 18, 2018

Copy link
Copy Markdown
ContributorAuthor

Looking into it

@eerhardt

eerhardt commented Sep 18, 2018

Copy link
Copy Markdown
Member

It's a well-known issue that the wine dataset isn't working right now. @artidoro is working on it.

@justinormont

justinormont commented Sep 18, 2018

Copy link
Copy Markdown
Contributor

Related issue for the Wine dataset: #889Hot linking to a UCI dataset

Currently, the UCI web server is non-responsive, causing the dataset to not download, and the test to fail.

@Anipik

Anipik commented Sep 19, 2018

Copy link
Copy Markdown
ContributorAuthor

@justinormont the ci is green, can we go ahead and merge this one ?

@justinormont

justinormont commented Sep 19, 2018

Copy link
Copy Markdown
Contributor

@Anipik, the merge is waiting on a merge conflict, can you look in to it?

@Anipik

Anipik commented Sep 19, 2018

Copy link
Copy Markdown
ContributorAuthor

@justinormont i resolved the conflict

@justinormont
Add missing semicolon
eb738d2
Missing semicolon is causing the build the fail:
```
2018-09-19T04:41:39.2181028Z Datasets.cs(171,10): error CS1002: ; expected [/__w/3/s/test/Microsoft.ML.TestFramework/Microsoft.ML.TestFramework.csproj]
2018-09-19T04:41:39.7812509Z Microsoft.ML.StandardLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.StandardLearners/netstandard2.0/Microsoft.ML.StandardLearners.dll
2018-09-19T04:41:40.7120753Z Microsoft.ML.HalLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.HalLearners/netstandard2.0/Microsoft.ML.HalLearners.dll
2018-09-19T04:41:40.8804119Z Microsoft.ML.Ensemble -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Ensemble/netstandard2.0/Microsoft.ML.Ensemble.dll
2018-09-19T04:41:40.9555420Z Microsoft.ML.LightGBM -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.LightGBM/netstandard2.0/Microsoft.ML.LightGBM.dll
2018-09-19T04:41:41.5610322Z Microsoft.ML.PipelineInference -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.PipelineInference/netstandard2.0/Microsoft.ML.PipelineInference.dll
2018-09-19T04:41:42.4887819Z Microsoft.ML.Console -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Console/netcoreapp2.0/MML.dll
2018-09-19T04:41:45.7637388Z Microsoft.ML.FSharp.Tests -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.FSharp.Tests/netcoreapp2.1/Microsoft.ML.FSharp.Tests.dll
2018-09-19T04:41:45.7926386Z /__w/3/s/dir.traversal.targets(25,5): error : Build failed. See earlier errors. [/__w/3/s/build.proj]
2018-09-19T04:41:45.8133725Z 2018-09-19T04:41:45.8152732Z Build FAILED.
```
@justinormont
justinormont merged commit 86f4d93 into dotnet:masterSep 19, 2018
@justinormont

justinormont commented Sep 19, 2018

Copy link
Copy Markdown
Contributor

Thanks @Anipik for all the unexpected work needed in this pull request.

@Anipik
Anipik deleted the NumericRanking branch October 10, 2018 18:22
@ghostghost locked as resolved and limited conversation to collaborators Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

5 more reviewers
@justinormontjustinormontjustinormont approved these changes
@adamsitnikadamsitnikadamsitnik left review comments
@danmoseleydanmoseleydanmoseley left review comments
@eerhardteerhardteerhardt approved these changes
@sfilipisfilipisfilipi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Anipik@justinormont@eerhardt@adamsitnik@danmoseley@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Added numeric ranking Performance Tests - #888
Merged
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Sep 19, 2018
Merged
Added numeric ranking Performance Tests#888
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Conversation
@Anipik
@AnipikAnipik
commented
Sep 11, 2018

Copy link


Copy Markdown

Contributor
Added benchmarking performance tests for Numeric ranking.
cc @justinormont@sfilipi @danmosemsft @eerhardt@shauheen
@Anipik
Anipik
commented
Sep 11, 2018

Copy link


Copy Markdown

ContributorAuthor
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 72.7866 s, StdErr = 0.5849 s (0.80%); N = 20, StdDev = 2.6156 sMin = 68.6174 s, Q1 = 70.0802 s, Median = 73.3494 s, Q3 = 74.9717 s, Max = 77.5199 s
IQR = 4.8915 s, LowerFence = 62.7429 s, UpperFence = 82.3089 s
ConfidenceInterval = [70.5153 s; 75.0579 s] (CI 99.9%), Margin = 2.2713 s (3.12% of Mean)Skewness = -0.09, Kurtosis = 1.66, MValue = 3.14
-------------------- Histogram --------------------
[68.616 s ; 70.533 s) | @@@@@@
[70.533 s ; 72.219 s) | @@
[72.219 s ; 74.846 s) | @@@@@@@
[74.846 s ; 76.379 s) | @@@@
[76.379 s ; 78.363 s) | @
---------------------------------------------------
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 65.4654 s, StdErr = 0.9955 s (1.52%); N = 20, StdDev = 4.4522 sMin = 58.3025 s, Q1 = 62.0859 s, Median = 65.7175 s, Q3 = 68.5847 s, Max = 75.8618 s
IQR = 6.4988 s, LowerFence = 52.3377 s, UpperFence = 78.3329 s
ConfidenceInterval = [61.5993 s; 69.3315 s] (CI 99.9%), Margin = 3.8661 s (5.91% of Mean)Skewness = 0.1, Kurtosis = 2.64, MValue = 3.25
-------------------- Histogram --------------------
[58.224 s ; 61.721 s) | @@@@@
[61.721 s ; 64.643 s) | @
[64.643 s ; 67.513 s) | @@@@@@@@
[67.513 s ; 70.412 s) | @@@@@
[70.412 s ; 74.427 s) |
[74.427 s ; 77.297 s) | @
---------------------------------------------------
Ranking.Test_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 4.5715 s, StdErr = 0.0079 s (0.17%); N = 14, StdDev = 0.0295 sMin = 4.5317 s, Q1 = 4.5526 s, Median = 4.5653 s, Q3 = 4.5778 s, Max = 4.6321 s
IQR = 0.0252 s, LowerFence = 4.5147 s, UpperFence = 4.6157 s
ConfidenceInterval = [4.5382 s; 4.6047 s] (CI 99.9%), Margin = 0.0333 s (0.73% of Mean)Skewness = 0.67, Kurtosis = 2.39, MValue = 2
-------------------- Histogram --------------------
[4.524 s ; 4.643 s) | @@@@@@@@@@@@@@
---------------------------------------------------
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree72.787 s2.2713 s2.6156 s-6247000.00001058000.0000286000.000026171.31 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM65.465 s3.8661 s4.4522 s-3595000.00001684000.0000267000.0000304.59 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree4.571 s0.0333 s0.0295 s-558000.0000279000.00001000.000011.93 MB
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(IncludeBenchmarkData)' == 'true'" > 
 <TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/WikiDetoxAnnotated160kRows.tsv"
@eerhardteerhardtSep 12, 2018•

edited

Loading

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.

The duplication here could be simplified using MSBuild. Something along the lines of:
<ItemGroup>
<TlcResourceFileInclude="WikiDetoxAnnotated160kRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTrain3.6MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KValidate1.2MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTest1.2MRows.tsv" />
<TlcResourceFileUpdate="@(TlcResourceFile)">
<Url>http://aka.ms/tlc-resources/benchmarks/%(Identity)</Url>
<DestinationFile>$(MSBuildThisFileDirectory)test/data/external/%(Identity)</DestinationFile>
</TlcResourceFile>
<TestFileInclude="@(TlcResourceFile->'$(MSBuildThisFileDirectory)/test/data/external/%(Identity)')" />
</ItemGroup>
I'm not 100% sure it is better, but it reduces the number of times these URLs need to be copied.
@danmoseleydanmoseleySep 12, 2018

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

That's batching, I think it would only work within a <Target> ?
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep dan is right. its not working in this case. any other suggestion here ?
@eerhardteerhardtSep 12, 2018

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.

It works, you just need to have the right syntax. I've updated the above with actual MSBuild code that works.
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Okay thanks :)
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

@Anipik
Anipik
commented
Sep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.228 (1803/April2018Update/Redstone4)
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=2.1.400
[Host] : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Job-QFXMOR : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree32.993 s0.5025 s0.4455 s-2762000.0000192000.000056000.000015435.34 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM31.045 s2.1895 s2.5215 s-1198000.0000560000.000084000.0000246.64 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree1.153 s0.0943 s0.1086 s-122000.000055000.000012000.00002.93 MB
@justinormont
justinormont
self-requested a review
September 12, 2018 19:35
@justinormontjustinormont
mentioned this pull request
Sep 12, 2018

Closed

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

sfilipi
sfilipi
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

 Include="..\data\external\WikiDetoxAnnotated160kRows.tsv"
 Link="external\WikiDetoxAnnotated160kRows.tsv">

 <TlcResourceFile Update="@(TlcResourceFile)">
@sfilipisfilipiSep 13, 2018

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.

TlcResourceFile [](start = 31, length = 15)
call it something non TLC
@eerhardteerhardtSep 13, 2018

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.

Agreed, but the reason I chose the name originally is because of the URL:
http://aka.ms/tlc-resources
Can we change this URL? Or make a new aka.ms URL pointing to the same location with a different name?
sfilipi
sfilipi
approved these changes
Sep 13, 2018
View reviewed changes
@sfilipisfilipi
left a comment


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.

:shipit:
justinormont
justinormont
approved these changes
Sep 13, 2018
View reviewed changes
@justinormontjustinormont
left a comment


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.

LGTM.
Let's wait to merge until the MSLR-WEB10K dataset is available in the CDN.
eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
approved these changes
Sep 13, 2018
View reviewed changes
@eerhardteerhardt
left a comment


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.

:shipit:
@justinormont
justinormont
commented
Sep 13, 2018

Copy link


Copy Markdown

Contributor
Please also add the citation to the MSLR-WEB10K dataset.
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated


namespace Microsoft.ML.Benchmarks
{
 [WarmupCount(8)] // It helps to reduce the standard deviation of these tests.
@justinormontjustinormontSep 14, 2018•

edited

Loading

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.

The normal user is unlikely to pre-train a model 8 times before training their model. This will be representative of the steady state reached when a model is retrained many times, but not very representative of the normal user's interaction w/ ML.net.
Do we know what's causing the time difference between the first run and the later runs? The first run is most representative of what a normal user will experience.
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@adamsitnik can you give us a better view here ? Increasing the warmup iterations leads to reducing the standard deviation here
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@justinormont do u want me to reduce it ?
cc @danmosemsft
@adamsitnikadamsitnikSep 17, 2018•

edited

Loading

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 you give us a better view here ?
@Anipik Unfortunately, it's not that simple. To find out why given benchmark behaves differently for different warmup counts we would have to profile it. It could be that OS gets warmed up and reading the input files becomes faster or anything like that.
The normal user is unlikely to pre-train a model 8 times before training their model.
@justinormont I agree. In that case we should set the WarmupCount to 0, IterationCount to 1 and LaunchCount to 20. Which means that BenchmarkDotNet is going to start a new process 20 times and each time execute the benchmark only once, without any warmup (the real use case) and just exit the process.
Edit: we should most probably have two configs: one for training benchmarks (no warmups) and one for prediction benchmarks (the one we have today)
@AnipikAnipikSep 17, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will revert warmupCount to 1 for this PR to get merged, we can later follow up with 2 config files as adam suggested
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
suggested changes
Sep 14, 2018
View reviewed changes
@justinormontjustinormont
left a comment
•

edited

Loading

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.

Looks good, though there's a couple minor things:
eerhardt
eerhardt
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 17, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont cam you take a look here ?
justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont can you merge this ? I don't have the write access to the repo ?
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
I'm going to close & re-open this pull request to notify the CI to re-check this PR.
@justinormont
justinormont
commented
Sep 18, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI test are failing. Though GitHub says 'in progress', it will say failed soon.
The winequality-white.csv file is the cause.
Error:
2018-09-18T17:27:13.4451915Z System.IO.IOException : Could not find file 'D:\a\1\s\test\data\external\winequality-white.csv'
2018-09-18T17:27:13.4452129Z Stack Trace:
2018-09-18T17:27:13.4452368Z at Microsoft.ML.Runtime.Data.MultiFileSource..ctor(String path) in D:\a\1\s\src\Microsoft.ML.Data\DataLoadSave\MultiFileSource.cs:line 31
2018-09-18T17:27:13.4452691Z at Microsoft.ML.StaticPipelineTesting.Training.SdcaRegression() in D:\a\1\s\test\Microsoft.ML.StaticPipelineTesting\Training.cs:line 29
2018-09-18T17:27:13.4452938Z 2018-09-18T17:27:13.4453436Z Results File: D:\a\1\s\bin/AnyCPU.Release\Microsoft.ML.StaticPipelineTesting\VssAdministrator_factoryvm-az385_2018-09-18_17_27_12.trx
2018-09-18T17:27:13.4454016Z 2018-09-18T17:27:13.4454366Z Total tests: 13. Passed: 9. Failed: 4. Skipped: 0.

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
Looking into it
@eerhardt
eerhardt
commented
Sep 18, 2018

Copy link


Copy Markdown

Member
It's a well-known issue that the wine dataset isn't working right now. @artidoro is working on it.
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
Related issue for the Wine dataset: #889Hot linking to a UCI dataset
Currently, the UCI web server is non-responsive, causing the dataset to not download, and the test to fail.
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont the ci is green, can we go ahead and merge this one ?
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
@Anipik, the merge is waiting on a merge conflict, can you look in to it?
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont i resolved the conflict
@justinormont
Add missing semicolon
eb738d2
Missing semicolon is causing the build the fail:
```
2018-09-19T04:41:39.2181028Z Datasets.cs(171,10): error CS1002: ; expected [/__w/3/s/test/Microsoft.ML.TestFramework/Microsoft.ML.TestFramework.csproj]
2018-09-19T04:41:39.7812509Z Microsoft.ML.StandardLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.StandardLearners/netstandard2.0/Microsoft.ML.StandardLearners.dll
2018-09-19T04:41:40.7120753Z Microsoft.ML.HalLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.HalLearners/netstandard2.0/Microsoft.ML.HalLearners.dll
2018-09-19T04:41:40.8804119Z Microsoft.ML.Ensemble -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Ensemble/netstandard2.0/Microsoft.ML.Ensemble.dll
2018-09-19T04:41:40.9555420Z Microsoft.ML.LightGBM -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.LightGBM/netstandard2.0/Microsoft.ML.LightGBM.dll
2018-09-19T04:41:41.5610322Z Microsoft.ML.PipelineInference -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.PipelineInference/netstandard2.0/Microsoft.ML.PipelineInference.dll
2018-09-19T04:41:42.4887819Z Microsoft.ML.Console -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Console/netcoreapp2.0/MML.dll
2018-09-19T04:41:45.7637388Z Microsoft.ML.FSharp.Tests -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.FSharp.Tests/netcoreapp2.1/Microsoft.ML.FSharp.Tests.dll
2018-09-19T04:41:45.7926386Z /__w/3/s/dir.traversal.targets(25,5): error : Build failed. See earlier errors. [/__w/3/s/build.proj]
2018-09-19T04:41:45.8133725Z 2018-09-19T04:41:45.8152732Z Build FAILED.
```
@justinormont
justinormont
merged commit 86f4d93
into
dotnet:masterSep 19, 2018
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
Thanks @Anipik for all the unexpected work needed in this pull request.
@Anipik
Anipik
deleted the

NumericRanking

branch
October 10, 2018 18:22
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


5 more reviewers

@justinormontjustinormontjustinormont approved these changes
@adamsitnikadamsitnikadamsitnik left review comments
@danmoseleydanmoseleydanmoseley left review comments
@eerhardteerhardteerhardt approved these changes
@sfilipisfilipisfilipi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Anipik@justinormont@eerhardt@adamsitnik@danmoseley@sfilipi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Added numeric ranking Performance Tests - #888
Merged
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Sep 19, 2018
Merged
Added numeric ranking Performance Tests#888
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Conversation
@Anipik
@AnipikAnipik
commented
Sep 11, 2018

Copy link


Copy Markdown

Contributor
Added benchmarking performance tests for Numeric ranking.
cc @justinormont@sfilipi @danmosemsft @eerhardt@shauheen
@Anipik
Anipik
commented
Sep 11, 2018

Copy link


Copy Markdown

ContributorAuthor
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 72.7866 s, StdErr = 0.5849 s (0.80%); N = 20, StdDev = 2.6156 sMin = 68.6174 s, Q1 = 70.0802 s, Median = 73.3494 s, Q3 = 74.9717 s, Max = 77.5199 s
IQR = 4.8915 s, LowerFence = 62.7429 s, UpperFence = 82.3089 s
ConfidenceInterval = [70.5153 s; 75.0579 s] (CI 99.9%), Margin = 2.2713 s (3.12% of Mean)Skewness = -0.09, Kurtosis = 1.66, MValue = 3.14
-------------------- Histogram --------------------
[68.616 s ; 70.533 s) | @@@@@@
[70.533 s ; 72.219 s) | @@
[72.219 s ; 74.846 s) | @@@@@@@
[74.846 s ; 76.379 s) | @@@@
[76.379 s ; 78.363 s) | @
---------------------------------------------------
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 65.4654 s, StdErr = 0.9955 s (1.52%); N = 20, StdDev = 4.4522 sMin = 58.3025 s, Q1 = 62.0859 s, Median = 65.7175 s, Q3 = 68.5847 s, Max = 75.8618 s
IQR = 6.4988 s, LowerFence = 52.3377 s, UpperFence = 78.3329 s
ConfidenceInterval = [61.5993 s; 69.3315 s] (CI 99.9%), Margin = 3.8661 s (5.91% of Mean)Skewness = 0.1, Kurtosis = 2.64, MValue = 3.25
-------------------- Histogram --------------------
[58.224 s ; 61.721 s) | @@@@@
[61.721 s ; 64.643 s) | @
[64.643 s ; 67.513 s) | @@@@@@@@
[67.513 s ; 70.412 s) | @@@@@
[70.412 s ; 74.427 s) |
[74.427 s ; 77.297 s) | @
---------------------------------------------------
Ranking.Test_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 4.5715 s, StdErr = 0.0079 s (0.17%); N = 14, StdDev = 0.0295 sMin = 4.5317 s, Q1 = 4.5526 s, Median = 4.5653 s, Q3 = 4.5778 s, Max = 4.6321 s
IQR = 0.0252 s, LowerFence = 4.5147 s, UpperFence = 4.6157 s
ConfidenceInterval = [4.5382 s; 4.6047 s] (CI 99.9%), Margin = 0.0333 s (0.73% of Mean)Skewness = 0.67, Kurtosis = 2.39, MValue = 2
-------------------- Histogram --------------------
[4.524 s ; 4.643 s) | @@@@@@@@@@@@@@
---------------------------------------------------
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree72.787 s2.2713 s2.6156 s-6247000.00001058000.0000286000.000026171.31 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM65.465 s3.8661 s4.4522 s-3595000.00001684000.0000267000.0000304.59 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree4.571 s0.0333 s0.0295 s-558000.0000279000.00001000.000011.93 MB
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(IncludeBenchmarkData)' == 'true'" > 
 <TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/WikiDetoxAnnotated160kRows.tsv"
@eerhardteerhardtSep 12, 2018•

edited

Loading

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.

The duplication here could be simplified using MSBuild. Something along the lines of:
<ItemGroup>
<TlcResourceFileInclude="WikiDetoxAnnotated160kRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTrain3.6MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KValidate1.2MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTest1.2MRows.tsv" />
<TlcResourceFileUpdate="@(TlcResourceFile)">
<Url>http://aka.ms/tlc-resources/benchmarks/%(Identity)</Url>
<DestinationFile>$(MSBuildThisFileDirectory)test/data/external/%(Identity)</DestinationFile>
</TlcResourceFile>
<TestFileInclude="@(TlcResourceFile->'$(MSBuildThisFileDirectory)/test/data/external/%(Identity)')" />
</ItemGroup>
I'm not 100% sure it is better, but it reduces the number of times these URLs need to be copied.
@danmoseleydanmoseleySep 12, 2018

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

That's batching, I think it would only work within a <Target> ?
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep dan is right. its not working in this case. any other suggestion here ?
@eerhardteerhardtSep 12, 2018

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.

It works, you just need to have the right syntax. I've updated the above with actual MSBuild code that works.
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Okay thanks :)
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

@Anipik
Anipik
commented
Sep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.228 (1803/April2018Update/Redstone4)
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=2.1.400
[Host] : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Job-QFXMOR : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree32.993 s0.5025 s0.4455 s-2762000.0000192000.000056000.000015435.34 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM31.045 s2.1895 s2.5215 s-1198000.0000560000.000084000.0000246.64 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree1.153 s0.0943 s0.1086 s-122000.000055000.000012000.00002.93 MB
@justinormont
justinormont
self-requested a review
September 12, 2018 19:35
@justinormontjustinormont
mentioned this pull request
Sep 12, 2018

Closed

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

sfilipi
sfilipi
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

 Include="..\data\external\WikiDetoxAnnotated160kRows.tsv"
 Link="external\WikiDetoxAnnotated160kRows.tsv">

 <TlcResourceFile Update="@(TlcResourceFile)">
@sfilipisfilipiSep 13, 2018

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.

TlcResourceFile [](start = 31, length = 15)
call it something non TLC
@eerhardteerhardtSep 13, 2018

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.

Agreed, but the reason I chose the name originally is because of the URL:
http://aka.ms/tlc-resources
Can we change this URL? Or make a new aka.ms URL pointing to the same location with a different name?
sfilipi
sfilipi
approved these changes
Sep 13, 2018
View reviewed changes
@sfilipisfilipi
left a comment


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.

:shipit:
justinormont
justinormont
approved these changes
Sep 13, 2018
View reviewed changes
@justinormontjustinormont
left a comment


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.

LGTM.
Let's wait to merge until the MSLR-WEB10K dataset is available in the CDN.
eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
approved these changes
Sep 13, 2018
View reviewed changes
@eerhardteerhardt
left a comment


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.

:shipit:
@justinormont
justinormont
commented
Sep 13, 2018

Copy link


Copy Markdown

Contributor
Please also add the citation to the MSLR-WEB10K dataset.
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated


namespace Microsoft.ML.Benchmarks
{
 [WarmupCount(8)] // It helps to reduce the standard deviation of these tests.
@justinormontjustinormontSep 14, 2018•

edited

Loading

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.

The normal user is unlikely to pre-train a model 8 times before training their model. This will be representative of the steady state reached when a model is retrained many times, but not very representative of the normal user's interaction w/ ML.net.
Do we know what's causing the time difference between the first run and the later runs? The first run is most representative of what a normal user will experience.
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@adamsitnik can you give us a better view here ? Increasing the warmup iterations leads to reducing the standard deviation here
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@justinormont do u want me to reduce it ?
cc @danmosemsft
@adamsitnikadamsitnikSep 17, 2018•

edited

Loading

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 you give us a better view here ?
@Anipik Unfortunately, it's not that simple. To find out why given benchmark behaves differently for different warmup counts we would have to profile it. It could be that OS gets warmed up and reading the input files becomes faster or anything like that.
The normal user is unlikely to pre-train a model 8 times before training their model.
@justinormont I agree. In that case we should set the WarmupCount to 0, IterationCount to 1 and LaunchCount to 20. Which means that BenchmarkDotNet is going to start a new process 20 times and each time execute the benchmark only once, without any warmup (the real use case) and just exit the process.
Edit: we should most probably have two configs: one for training benchmarks (no warmups) and one for prediction benchmarks (the one we have today)
@AnipikAnipikSep 17, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will revert warmupCount to 1 for this PR to get merged, we can later follow up with 2 config files as adam suggested
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
suggested changes
Sep 14, 2018
View reviewed changes
@justinormontjustinormont
left a comment
•

edited

Loading

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.

Looks good, though there's a couple minor things:
eerhardt
eerhardt
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 17, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont cam you take a look here ?
justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont can you merge this ? I don't have the write access to the repo ?
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
I'm going to close & re-open this pull request to notify the CI to re-check this PR.
@justinormont
justinormont
commented
Sep 18, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI test are failing. Though GitHub says 'in progress', it will say failed soon.
The winequality-white.csv file is the cause.
Error:
2018-09-18T17:27:13.4451915Z System.IO.IOException : Could not find file 'D:\a\1\s\test\data\external\winequality-white.csv'
2018-09-18T17:27:13.4452129Z Stack Trace:
2018-09-18T17:27:13.4452368Z at Microsoft.ML.Runtime.Data.MultiFileSource..ctor(String path) in D:\a\1\s\src\Microsoft.ML.Data\DataLoadSave\MultiFileSource.cs:line 31
2018-09-18T17:27:13.4452691Z at Microsoft.ML.StaticPipelineTesting.Training.SdcaRegression() in D:\a\1\s\test\Microsoft.ML.StaticPipelineTesting\Training.cs:line 29
2018-09-18T17:27:13.4452938Z 2018-09-18T17:27:13.4453436Z Results File: D:\a\1\s\bin/AnyCPU.Release\Microsoft.ML.StaticPipelineTesting\VssAdministrator_factoryvm-az385_2018-09-18_17_27_12.trx
2018-09-18T17:27:13.4454016Z 2018-09-18T17:27:13.4454366Z Total tests: 13. Passed: 9. Failed: 4. Skipped: 0.

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
Looking into it
@eerhardt
eerhardt
commented
Sep 18, 2018

Copy link


Copy Markdown

Member
It's a well-known issue that the wine dataset isn't working right now. @artidoro is working on it.
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
Related issue for the Wine dataset: #889Hot linking to a UCI dataset
Currently, the UCI web server is non-responsive, causing the dataset to not download, and the test to fail.
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont the ci is green, can we go ahead and merge this one ?
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
@Anipik, the merge is waiting on a merge conflict, can you look in to it?
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont i resolved the conflict
@justinormont
Add missing semicolon
eb738d2
Missing semicolon is causing the build the fail:
```
2018-09-19T04:41:39.2181028Z Datasets.cs(171,10): error CS1002: ; expected [/__w/3/s/test/Microsoft.ML.TestFramework/Microsoft.ML.TestFramework.csproj]
2018-09-19T04:41:39.7812509Z Microsoft.ML.StandardLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.StandardLearners/netstandard2.0/Microsoft.ML.StandardLearners.dll
2018-09-19T04:41:40.7120753Z Microsoft.ML.HalLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.HalLearners/netstandard2.0/Microsoft.ML.HalLearners.dll
2018-09-19T04:41:40.8804119Z Microsoft.ML.Ensemble -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Ensemble/netstandard2.0/Microsoft.ML.Ensemble.dll
2018-09-19T04:41:40.9555420Z Microsoft.ML.LightGBM -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.LightGBM/netstandard2.0/Microsoft.ML.LightGBM.dll
2018-09-19T04:41:41.5610322Z Microsoft.ML.PipelineInference -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.PipelineInference/netstandard2.0/Microsoft.ML.PipelineInference.dll
2018-09-19T04:41:42.4887819Z Microsoft.ML.Console -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Console/netcoreapp2.0/MML.dll
2018-09-19T04:41:45.7637388Z Microsoft.ML.FSharp.Tests -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.FSharp.Tests/netcoreapp2.1/Microsoft.ML.FSharp.Tests.dll
2018-09-19T04:41:45.7926386Z /__w/3/s/dir.traversal.targets(25,5): error : Build failed. See earlier errors. [/__w/3/s/build.proj]
2018-09-19T04:41:45.8133725Z 2018-09-19T04:41:45.8152732Z Build FAILED.
```
@justinormont
justinormont
merged commit 86f4d93
into
dotnet:masterSep 19, 2018
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
Thanks @Anipik for all the unexpected work needed in this pull request.
@Anipik
Anipik
deleted the

NumericRanking

branch
October 10, 2018 18:22
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


5 more reviewers

@justinormontjustinormontjustinormont approved these changes
@adamsitnikadamsitnikadamsitnik left review comments
@danmoseleydanmoseleydanmoseley left review comments
@eerhardteerhardteerhardt approved these changes
@sfilipisfilipisfilipi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Anipik@justinormont@eerhardt@adamsitnik@danmoseley@sfilipi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Added numeric ranking Performance Tests - #888
Merged
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Sep 19, 2018
Merged
Added numeric ranking Performance Tests#888
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Conversation
@Anipik
@AnipikAnipik
commented
Sep 11, 2018

Copy link


Copy Markdown

Contributor
Added benchmarking performance tests for Numeric ranking.
cc @justinormont@sfilipi @danmosemsft @eerhardt@shauheen
@Anipik
Anipik
commented
Sep 11, 2018

Copy link


Copy Markdown

ContributorAuthor
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 72.7866 s, StdErr = 0.5849 s (0.80%); N = 20, StdDev = 2.6156 sMin = 68.6174 s, Q1 = 70.0802 s, Median = 73.3494 s, Q3 = 74.9717 s, Max = 77.5199 s
IQR = 4.8915 s, LowerFence = 62.7429 s, UpperFence = 82.3089 s
ConfidenceInterval = [70.5153 s; 75.0579 s] (CI 99.9%), Margin = 2.2713 s (3.12% of Mean)Skewness = -0.09, Kurtosis = 1.66, MValue = 3.14
-------------------- Histogram --------------------
[68.616 s ; 70.533 s) | @@@@@@
[70.533 s ; 72.219 s) | @@
[72.219 s ; 74.846 s) | @@@@@@@
[74.846 s ; 76.379 s) | @@@@
[76.379 s ; 78.363 s) | @
---------------------------------------------------
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 65.4654 s, StdErr = 0.9955 s (1.52%); N = 20, StdDev = 4.4522 sMin = 58.3025 s, Q1 = 62.0859 s, Median = 65.7175 s, Q3 = 68.5847 s, Max = 75.8618 s
IQR = 6.4988 s, LowerFence = 52.3377 s, UpperFence = 78.3329 s
ConfidenceInterval = [61.5993 s; 69.3315 s] (CI 99.9%), Margin = 3.8661 s (5.91% of Mean)Skewness = 0.1, Kurtosis = 2.64, MValue = 3.25
-------------------- Histogram --------------------
[58.224 s ; 61.721 s) | @@@@@
[61.721 s ; 64.643 s) | @
[64.643 s ; 67.513 s) | @@@@@@@@
[67.513 s ; 70.412 s) | @@@@@
[70.412 s ; 74.427 s) |
[74.427 s ; 77.297 s) | @
---------------------------------------------------
Ranking.Test_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 4.5715 s, StdErr = 0.0079 s (0.17%); N = 14, StdDev = 0.0295 sMin = 4.5317 s, Q1 = 4.5526 s, Median = 4.5653 s, Q3 = 4.5778 s, Max = 4.6321 s
IQR = 0.0252 s, LowerFence = 4.5147 s, UpperFence = 4.6157 s
ConfidenceInterval = [4.5382 s; 4.6047 s] (CI 99.9%), Margin = 0.0333 s (0.73% of Mean)Skewness = 0.67, Kurtosis = 2.39, MValue = 2
-------------------- Histogram --------------------
[4.524 s ; 4.643 s) | @@@@@@@@@@@@@@
---------------------------------------------------
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree72.787 s2.2713 s2.6156 s-6247000.00001058000.0000286000.000026171.31 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM65.465 s3.8661 s4.4522 s-3595000.00001684000.0000267000.0000304.59 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree4.571 s0.0333 s0.0295 s-558000.0000279000.00001000.000011.93 MB
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(IncludeBenchmarkData)' == 'true'" > 
 <TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/WikiDetoxAnnotated160kRows.tsv"
@eerhardteerhardtSep 12, 2018•

edited

Loading

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.

The duplication here could be simplified using MSBuild. Something along the lines of:
<ItemGroup>
<TlcResourceFileInclude="WikiDetoxAnnotated160kRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTrain3.6MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KValidate1.2MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTest1.2MRows.tsv" />
<TlcResourceFileUpdate="@(TlcResourceFile)">
<Url>http://aka.ms/tlc-resources/benchmarks/%(Identity)</Url>
<DestinationFile>$(MSBuildThisFileDirectory)test/data/external/%(Identity)</DestinationFile>
</TlcResourceFile>
<TestFileInclude="@(TlcResourceFile->'$(MSBuildThisFileDirectory)/test/data/external/%(Identity)')" />
</ItemGroup>
I'm not 100% sure it is better, but it reduces the number of times these URLs need to be copied.
@danmoseleydanmoseleySep 12, 2018

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

That's batching, I think it would only work within a <Target> ?
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep dan is right. its not working in this case. any other suggestion here ?
@eerhardteerhardtSep 12, 2018

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.

It works, you just need to have the right syntax. I've updated the above with actual MSBuild code that works.
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Okay thanks :)
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

@Anipik
Anipik
commented
Sep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.228 (1803/April2018Update/Redstone4)
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=2.1.400
[Host] : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Job-QFXMOR : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree32.993 s0.5025 s0.4455 s-2762000.0000192000.000056000.000015435.34 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM31.045 s2.1895 s2.5215 s-1198000.0000560000.000084000.0000246.64 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree1.153 s0.0943 s0.1086 s-122000.000055000.000012000.00002.93 MB
@justinormont
justinormont
self-requested a review
September 12, 2018 19:35
@justinormontjustinormont
mentioned this pull request
Sep 12, 2018

Closed

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

sfilipi
sfilipi
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

 Include="..\data\external\WikiDetoxAnnotated160kRows.tsv"
 Link="external\WikiDetoxAnnotated160kRows.tsv">

 <TlcResourceFile Update="@(TlcResourceFile)">
@sfilipisfilipiSep 13, 2018

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.

TlcResourceFile [](start = 31, length = 15)
call it something non TLC
@eerhardteerhardtSep 13, 2018

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.

Agreed, but the reason I chose the name originally is because of the URL:
http://aka.ms/tlc-resources
Can we change this URL? Or make a new aka.ms URL pointing to the same location with a different name?
sfilipi
sfilipi
approved these changes
Sep 13, 2018
View reviewed changes
@sfilipisfilipi
left a comment


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.

:shipit:
justinormont
justinormont
approved these changes
Sep 13, 2018
View reviewed changes
@justinormontjustinormont
left a comment


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.

LGTM.
Let's wait to merge until the MSLR-WEB10K dataset is available in the CDN.
eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
approved these changes
Sep 13, 2018
View reviewed changes
@eerhardteerhardt
left a comment


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.

:shipit:
@justinormont
justinormont
commented
Sep 13, 2018

Copy link


Copy Markdown

Contributor
Please also add the citation to the MSLR-WEB10K dataset.
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated


namespace Microsoft.ML.Benchmarks
{
 [WarmupCount(8)] // It helps to reduce the standard deviation of these tests.
@justinormontjustinormontSep 14, 2018•

edited

Loading

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.

The normal user is unlikely to pre-train a model 8 times before training their model. This will be representative of the steady state reached when a model is retrained many times, but not very representative of the normal user's interaction w/ ML.net.
Do we know what's causing the time difference between the first run and the later runs? The first run is most representative of what a normal user will experience.
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@adamsitnik can you give us a better view here ? Increasing the warmup iterations leads to reducing the standard deviation here
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@justinormont do u want me to reduce it ?
cc @danmosemsft
@adamsitnikadamsitnikSep 17, 2018•

edited

Loading

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 you give us a better view here ?
@Anipik Unfortunately, it's not that simple. To find out why given benchmark behaves differently for different warmup counts we would have to profile it. It could be that OS gets warmed up and reading the input files becomes faster or anything like that.
The normal user is unlikely to pre-train a model 8 times before training their model.
@justinormont I agree. In that case we should set the WarmupCount to 0, IterationCount to 1 and LaunchCount to 20. Which means that BenchmarkDotNet is going to start a new process 20 times and each time execute the benchmark only once, without any warmup (the real use case) and just exit the process.
Edit: we should most probably have two configs: one for training benchmarks (no warmups) and one for prediction benchmarks (the one we have today)
@AnipikAnipikSep 17, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will revert warmupCount to 1 for this PR to get merged, we can later follow up with 2 config files as adam suggested
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
suggested changes
Sep 14, 2018
View reviewed changes
@justinormontjustinormont
left a comment
•

edited

Loading

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.

Looks good, though there's a couple minor things:
eerhardt
eerhardt
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 17, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont cam you take a look here ?
justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont can you merge this ? I don't have the write access to the repo ?
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
I'm going to close & re-open this pull request to notify the CI to re-check this PR.
@justinormont
justinormont
commented
Sep 18, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI test are failing. Though GitHub says 'in progress', it will say failed soon.
The winequality-white.csv file is the cause.
Error:
2018-09-18T17:27:13.4451915Z System.IO.IOException : Could not find file 'D:\a\1\s\test\data\external\winequality-white.csv'
2018-09-18T17:27:13.4452129Z Stack Trace:
2018-09-18T17:27:13.4452368Z at Microsoft.ML.Runtime.Data.MultiFileSource..ctor(String path) in D:\a\1\s\src\Microsoft.ML.Data\DataLoadSave\MultiFileSource.cs:line 31
2018-09-18T17:27:13.4452691Z at Microsoft.ML.StaticPipelineTesting.Training.SdcaRegression() in D:\a\1\s\test\Microsoft.ML.StaticPipelineTesting\Training.cs:line 29
2018-09-18T17:27:13.4452938Z 2018-09-18T17:27:13.4453436Z Results File: D:\a\1\s\bin/AnyCPU.Release\Microsoft.ML.StaticPipelineTesting\VssAdministrator_factoryvm-az385_2018-09-18_17_27_12.trx
2018-09-18T17:27:13.4454016Z 2018-09-18T17:27:13.4454366Z Total tests: 13. Passed: 9. Failed: 4. Skipped: 0.

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
Looking into it
@eerhardt
eerhardt
commented
Sep 18, 2018

Copy link


Copy Markdown

Member
It's a well-known issue that the wine dataset isn't working right now. @artidoro is working on it.
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
Related issue for the Wine dataset: #889Hot linking to a UCI dataset
Currently, the UCI web server is non-responsive, causing the dataset to not download, and the test to fail.
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont the ci is green, can we go ahead and merge this one ?
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
@Anipik, the merge is waiting on a merge conflict, can you look in to it?
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont i resolved the conflict
@justinormont
Add missing semicolon
eb738d2
Missing semicolon is causing the build the fail:
```
2018-09-19T04:41:39.2181028Z Datasets.cs(171,10): error CS1002: ; expected [/__w/3/s/test/Microsoft.ML.TestFramework/Microsoft.ML.TestFramework.csproj]
2018-09-19T04:41:39.7812509Z Microsoft.ML.StandardLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.StandardLearners/netstandard2.0/Microsoft.ML.StandardLearners.dll
2018-09-19T04:41:40.7120753Z Microsoft.ML.HalLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.HalLearners/netstandard2.0/Microsoft.ML.HalLearners.dll
2018-09-19T04:41:40.8804119Z Microsoft.ML.Ensemble -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Ensemble/netstandard2.0/Microsoft.ML.Ensemble.dll
2018-09-19T04:41:40.9555420Z Microsoft.ML.LightGBM -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.LightGBM/netstandard2.0/Microsoft.ML.LightGBM.dll
2018-09-19T04:41:41.5610322Z Microsoft.ML.PipelineInference -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.PipelineInference/netstandard2.0/Microsoft.ML.PipelineInference.dll
2018-09-19T04:41:42.4887819Z Microsoft.ML.Console -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Console/netcoreapp2.0/MML.dll
2018-09-19T04:41:45.7637388Z Microsoft.ML.FSharp.Tests -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.FSharp.Tests/netcoreapp2.1/Microsoft.ML.FSharp.Tests.dll
2018-09-19T04:41:45.7926386Z /__w/3/s/dir.traversal.targets(25,5): error : Build failed. See earlier errors. [/__w/3/s/build.proj]
2018-09-19T04:41:45.8133725Z 2018-09-19T04:41:45.8152732Z Build FAILED.
```
@justinormont
justinormont
merged commit 86f4d93
into
dotnet:masterSep 19, 2018
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
Thanks @Anipik for all the unexpected work needed in this pull request.
@Anipik
Anipik
deleted the

NumericRanking

branch
October 10, 2018 18:22
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


5 more reviewers

@justinormontjustinormontjustinormont approved these changes
@adamsitnikadamsitnikadamsitnik left review comments
@danmoseleydanmoseleydanmoseley left review comments
@eerhardteerhardteerhardt approved these changes
@sfilipisfilipisfilipi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Anipik@justinormont@eerhardt@adamsitnik@danmoseley@sfilipi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Added numeric ranking Performance Tests - #888
Merged
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Sep 19, 2018
Merged
Added numeric ranking Performance Tests#888
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Conversation
@Anipik
@AnipikAnipik
commented
Sep 11, 2018

Copy link


Copy Markdown

Contributor
Added benchmarking performance tests for Numeric ranking.
cc @justinormont@sfilipi @danmosemsft @eerhardt@shauheen
@Anipik
Anipik
commented
Sep 11, 2018

Copy link


Copy Markdown

ContributorAuthor
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 72.7866 s, StdErr = 0.5849 s (0.80%); N = 20, StdDev = 2.6156 sMin = 68.6174 s, Q1 = 70.0802 s, Median = 73.3494 s, Q3 = 74.9717 s, Max = 77.5199 s
IQR = 4.8915 s, LowerFence = 62.7429 s, UpperFence = 82.3089 s
ConfidenceInterval = [70.5153 s; 75.0579 s] (CI 99.9%), Margin = 2.2713 s (3.12% of Mean)Skewness = -0.09, Kurtosis = 1.66, MValue = 3.14
-------------------- Histogram --------------------
[68.616 s ; 70.533 s) | @@@@@@
[70.533 s ; 72.219 s) | @@
[72.219 s ; 74.846 s) | @@@@@@@
[74.846 s ; 76.379 s) | @@@@
[76.379 s ; 78.363 s) | @
---------------------------------------------------
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 65.4654 s, StdErr = 0.9955 s (1.52%); N = 20, StdDev = 4.4522 sMin = 58.3025 s, Q1 = 62.0859 s, Median = 65.7175 s, Q3 = 68.5847 s, Max = 75.8618 s
IQR = 6.4988 s, LowerFence = 52.3377 s, UpperFence = 78.3329 s
ConfidenceInterval = [61.5993 s; 69.3315 s] (CI 99.9%), Margin = 3.8661 s (5.91% of Mean)Skewness = 0.1, Kurtosis = 2.64, MValue = 3.25
-------------------- Histogram --------------------
[58.224 s ; 61.721 s) | @@@@@
[61.721 s ; 64.643 s) | @
[64.643 s ; 67.513 s) | @@@@@@@@
[67.513 s ; 70.412 s) | @@@@@
[70.412 s ; 74.427 s) |
[74.427 s ; 77.297 s) | @
---------------------------------------------------
Ranking.Test_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 4.5715 s, StdErr = 0.0079 s (0.17%); N = 14, StdDev = 0.0295 sMin = 4.5317 s, Q1 = 4.5526 s, Median = 4.5653 s, Q3 = 4.5778 s, Max = 4.6321 s
IQR = 0.0252 s, LowerFence = 4.5147 s, UpperFence = 4.6157 s
ConfidenceInterval = [4.5382 s; 4.6047 s] (CI 99.9%), Margin = 0.0333 s (0.73% of Mean)Skewness = 0.67, Kurtosis = 2.39, MValue = 2
-------------------- Histogram --------------------
[4.524 s ; 4.643 s) | @@@@@@@@@@@@@@
---------------------------------------------------
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree72.787 s2.2713 s2.6156 s-6247000.00001058000.0000286000.000026171.31 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM65.465 s3.8661 s4.4522 s-3595000.00001684000.0000267000.0000304.59 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree4.571 s0.0333 s0.0295 s-558000.0000279000.00001000.000011.93 MB
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(IncludeBenchmarkData)' == 'true'" > 
 <TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/WikiDetoxAnnotated160kRows.tsv"
@eerhardteerhardtSep 12, 2018•

edited

Loading

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.

The duplication here could be simplified using MSBuild. Something along the lines of:
<ItemGroup>
<TlcResourceFileInclude="WikiDetoxAnnotated160kRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTrain3.6MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KValidate1.2MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTest1.2MRows.tsv" />
<TlcResourceFileUpdate="@(TlcResourceFile)">
<Url>http://aka.ms/tlc-resources/benchmarks/%(Identity)</Url>
<DestinationFile>$(MSBuildThisFileDirectory)test/data/external/%(Identity)</DestinationFile>
</TlcResourceFile>
<TestFileInclude="@(TlcResourceFile->'$(MSBuildThisFileDirectory)/test/data/external/%(Identity)')" />
</ItemGroup>
I'm not 100% sure it is better, but it reduces the number of times these URLs need to be copied.
@danmoseleydanmoseleySep 12, 2018

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

That's batching, I think it would only work within a <Target> ?
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep dan is right. its not working in this case. any other suggestion here ?
@eerhardteerhardtSep 12, 2018

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.

It works, you just need to have the right syntax. I've updated the above with actual MSBuild code that works.
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Okay thanks :)
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

@Anipik
Anipik
commented
Sep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.228 (1803/April2018Update/Redstone4)
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=2.1.400
[Host] : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Job-QFXMOR : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree32.993 s0.5025 s0.4455 s-2762000.0000192000.000056000.000015435.34 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM31.045 s2.1895 s2.5215 s-1198000.0000560000.000084000.0000246.64 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree1.153 s0.0943 s0.1086 s-122000.000055000.000012000.00002.93 MB
@justinormont
justinormont
self-requested a review
September 12, 2018 19:35
@justinormontjustinormont
mentioned this pull request
Sep 12, 2018

Closed

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

sfilipi
sfilipi
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

 Include="..\data\external\WikiDetoxAnnotated160kRows.tsv"
 Link="external\WikiDetoxAnnotated160kRows.tsv">

 <TlcResourceFile Update="@(TlcResourceFile)">
@sfilipisfilipiSep 13, 2018

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.

TlcResourceFile [](start = 31, length = 15)
call it something non TLC
@eerhardteerhardtSep 13, 2018

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.

Agreed, but the reason I chose the name originally is because of the URL:
http://aka.ms/tlc-resources
Can we change this URL? Or make a new aka.ms URL pointing to the same location with a different name?
sfilipi
sfilipi
approved these changes
Sep 13, 2018
View reviewed changes
@sfilipisfilipi
left a comment


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.

:shipit:
justinormont
justinormont
approved these changes
Sep 13, 2018
View reviewed changes
@justinormontjustinormont
left a comment


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.

LGTM.
Let's wait to merge until the MSLR-WEB10K dataset is available in the CDN.
eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
approved these changes
Sep 13, 2018
View reviewed changes
@eerhardteerhardt
left a comment


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.

:shipit:
@justinormont
justinormont
commented
Sep 13, 2018

Copy link


Copy Markdown

Contributor
Please also add the citation to the MSLR-WEB10K dataset.
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated


namespace Microsoft.ML.Benchmarks
{
 [WarmupCount(8)] // It helps to reduce the standard deviation of these tests.
@justinormontjustinormontSep 14, 2018•

edited

Loading

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.

The normal user is unlikely to pre-train a model 8 times before training their model. This will be representative of the steady state reached when a model is retrained many times, but not very representative of the normal user's interaction w/ ML.net.
Do we know what's causing the time difference between the first run and the later runs? The first run is most representative of what a normal user will experience.
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@adamsitnik can you give us a better view here ? Increasing the warmup iterations leads to reducing the standard deviation here
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@justinormont do u want me to reduce it ?
cc @danmosemsft
@adamsitnikadamsitnikSep 17, 2018•

edited

Loading

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 you give us a better view here ?
@Anipik Unfortunately, it's not that simple. To find out why given benchmark behaves differently for different warmup counts we would have to profile it. It could be that OS gets warmed up and reading the input files becomes faster or anything like that.
The normal user is unlikely to pre-train a model 8 times before training their model.
@justinormont I agree. In that case we should set the WarmupCount to 0, IterationCount to 1 and LaunchCount to 20. Which means that BenchmarkDotNet is going to start a new process 20 times and each time execute the benchmark only once, without any warmup (the real use case) and just exit the process.
Edit: we should most probably have two configs: one for training benchmarks (no warmups) and one for prediction benchmarks (the one we have today)
@AnipikAnipikSep 17, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will revert warmupCount to 1 for this PR to get merged, we can later follow up with 2 config files as adam suggested
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
suggested changes
Sep 14, 2018
View reviewed changes
@justinormontjustinormont
left a comment
•

edited

Loading

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.

Looks good, though there's a couple minor things:
eerhardt
eerhardt
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 17, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont cam you take a look here ?
justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont can you merge this ? I don't have the write access to the repo ?
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
I'm going to close & re-open this pull request to notify the CI to re-check this PR.
@justinormont
justinormont
commented
Sep 18, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI test are failing. Though GitHub says 'in progress', it will say failed soon.
The winequality-white.csv file is the cause.
Error:
2018-09-18T17:27:13.4451915Z System.IO.IOException : Could not find file 'D:\a\1\s\test\data\external\winequality-white.csv'
2018-09-18T17:27:13.4452129Z Stack Trace:
2018-09-18T17:27:13.4452368Z at Microsoft.ML.Runtime.Data.MultiFileSource..ctor(String path) in D:\a\1\s\src\Microsoft.ML.Data\DataLoadSave\MultiFileSource.cs:line 31
2018-09-18T17:27:13.4452691Z at Microsoft.ML.StaticPipelineTesting.Training.SdcaRegression() in D:\a\1\s\test\Microsoft.ML.StaticPipelineTesting\Training.cs:line 29
2018-09-18T17:27:13.4452938Z 2018-09-18T17:27:13.4453436Z Results File: D:\a\1\s\bin/AnyCPU.Release\Microsoft.ML.StaticPipelineTesting\VssAdministrator_factoryvm-az385_2018-09-18_17_27_12.trx
2018-09-18T17:27:13.4454016Z 2018-09-18T17:27:13.4454366Z Total tests: 13. Passed: 9. Failed: 4. Skipped: 0.

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
Looking into it
@eerhardt
eerhardt
commented
Sep 18, 2018

Copy link


Copy Markdown

Member
It's a well-known issue that the wine dataset isn't working right now. @artidoro is working on it.
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
Related issue for the Wine dataset: #889Hot linking to a UCI dataset
Currently, the UCI web server is non-responsive, causing the dataset to not download, and the test to fail.
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont the ci is green, can we go ahead and merge this one ?
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
@Anipik, the merge is waiting on a merge conflict, can you look in to it?
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont i resolved the conflict
@justinormont
Add missing semicolon
eb738d2
Missing semicolon is causing the build the fail:
```
2018-09-19T04:41:39.2181028Z Datasets.cs(171,10): error CS1002: ; expected [/__w/3/s/test/Microsoft.ML.TestFramework/Microsoft.ML.TestFramework.csproj]
2018-09-19T04:41:39.7812509Z Microsoft.ML.StandardLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.StandardLearners/netstandard2.0/Microsoft.ML.StandardLearners.dll
2018-09-19T04:41:40.7120753Z Microsoft.ML.HalLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.HalLearners/netstandard2.0/Microsoft.ML.HalLearners.dll
2018-09-19T04:41:40.8804119Z Microsoft.ML.Ensemble -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Ensemble/netstandard2.0/Microsoft.ML.Ensemble.dll
2018-09-19T04:41:40.9555420Z Microsoft.ML.LightGBM -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.LightGBM/netstandard2.0/Microsoft.ML.LightGBM.dll
2018-09-19T04:41:41.5610322Z Microsoft.ML.PipelineInference -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.PipelineInference/netstandard2.0/Microsoft.ML.PipelineInference.dll
2018-09-19T04:41:42.4887819Z Microsoft.ML.Console -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Console/netcoreapp2.0/MML.dll
2018-09-19T04:41:45.7637388Z Microsoft.ML.FSharp.Tests -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.FSharp.Tests/netcoreapp2.1/Microsoft.ML.FSharp.Tests.dll
2018-09-19T04:41:45.7926386Z /__w/3/s/dir.traversal.targets(25,5): error : Build failed. See earlier errors. [/__w/3/s/build.proj]
2018-09-19T04:41:45.8133725Z 2018-09-19T04:41:45.8152732Z Build FAILED.
```
@justinormont
justinormont
merged commit 86f4d93
into
dotnet:masterSep 19, 2018
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
Thanks @Anipik for all the unexpected work needed in this pull request.
@Anipik
Anipik
deleted the

NumericRanking

branch
October 10, 2018 18:22
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


5 more reviewers

@justinormontjustinormontjustinormont approved these changes
@adamsitnikadamsitnikadamsitnik left review comments
@danmoseleydanmoseleydanmoseley left review comments
@eerhardteerhardteerhardt approved these changes
@sfilipisfilipisfilipi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Anipik@justinormont@eerhardt@adamsitnik@danmoseley@sfilipi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Added numeric ranking Performance Tests - #888
Merged
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Sep 19, 2018
Merged
Added numeric ranking Performance Tests#888
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Conversation
@Anipik
@AnipikAnipik
commented
Sep 11, 2018

Copy link


Copy Markdown

Contributor
Added benchmarking performance tests for Numeric ranking.
cc @justinormont@sfilipi @danmosemsft @eerhardt@shauheen
@Anipik
Anipik
commented
Sep 11, 2018

Copy link


Copy Markdown

ContributorAuthor
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 72.7866 s, StdErr = 0.5849 s (0.80%); N = 20, StdDev = 2.6156 sMin = 68.6174 s, Q1 = 70.0802 s, Median = 73.3494 s, Q3 = 74.9717 s, Max = 77.5199 s
IQR = 4.8915 s, LowerFence = 62.7429 s, UpperFence = 82.3089 s
ConfidenceInterval = [70.5153 s; 75.0579 s] (CI 99.9%), Margin = 2.2713 s (3.12% of Mean)Skewness = -0.09, Kurtosis = 1.66, MValue = 3.14
-------------------- Histogram --------------------
[68.616 s ; 70.533 s) | @@@@@@
[70.533 s ; 72.219 s) | @@
[72.219 s ; 74.846 s) | @@@@@@@
[74.846 s ; 76.379 s) | @@@@
[76.379 s ; 78.363 s) | @
---------------------------------------------------
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 65.4654 s, StdErr = 0.9955 s (1.52%); N = 20, StdDev = 4.4522 sMin = 58.3025 s, Q1 = 62.0859 s, Median = 65.7175 s, Q3 = 68.5847 s, Max = 75.8618 s
IQR = 6.4988 s, LowerFence = 52.3377 s, UpperFence = 78.3329 s
ConfidenceInterval = [61.5993 s; 69.3315 s] (CI 99.9%), Margin = 3.8661 s (5.91% of Mean)Skewness = 0.1, Kurtosis = 2.64, MValue = 3.25
-------------------- Histogram --------------------
[58.224 s ; 61.721 s) | @@@@@
[61.721 s ; 64.643 s) | @
[64.643 s ; 67.513 s) | @@@@@@@@
[67.513 s ; 70.412 s) | @@@@@
[70.412 s ; 74.427 s) |
[74.427 s ; 77.297 s) | @
---------------------------------------------------
Ranking.Test_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 4.5715 s, StdErr = 0.0079 s (0.17%); N = 14, StdDev = 0.0295 sMin = 4.5317 s, Q1 = 4.5526 s, Median = 4.5653 s, Q3 = 4.5778 s, Max = 4.6321 s
IQR = 0.0252 s, LowerFence = 4.5147 s, UpperFence = 4.6157 s
ConfidenceInterval = [4.5382 s; 4.6047 s] (CI 99.9%), Margin = 0.0333 s (0.73% of Mean)Skewness = 0.67, Kurtosis = 2.39, MValue = 2
-------------------- Histogram --------------------
[4.524 s ; 4.643 s) | @@@@@@@@@@@@@@
---------------------------------------------------
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree72.787 s2.2713 s2.6156 s-6247000.00001058000.0000286000.000026171.31 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM65.465 s3.8661 s4.4522 s-3595000.00001684000.0000267000.0000304.59 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree4.571 s0.0333 s0.0295 s-558000.0000279000.00001000.000011.93 MB
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(IncludeBenchmarkData)' == 'true'" > 
 <TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/WikiDetoxAnnotated160kRows.tsv"
@eerhardteerhardtSep 12, 2018•

edited

Loading

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.

The duplication here could be simplified using MSBuild. Something along the lines of:
<ItemGroup>
<TlcResourceFileInclude="WikiDetoxAnnotated160kRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTrain3.6MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KValidate1.2MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTest1.2MRows.tsv" />
<TlcResourceFileUpdate="@(TlcResourceFile)">
<Url>http://aka.ms/tlc-resources/benchmarks/%(Identity)</Url>
<DestinationFile>$(MSBuildThisFileDirectory)test/data/external/%(Identity)</DestinationFile>
</TlcResourceFile>
<TestFileInclude="@(TlcResourceFile->'$(MSBuildThisFileDirectory)/test/data/external/%(Identity)')" />
</ItemGroup>
I'm not 100% sure it is better, but it reduces the number of times these URLs need to be copied.
@danmoseleydanmoseleySep 12, 2018

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

That's batching, I think it would only work within a <Target> ?
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep dan is right. its not working in this case. any other suggestion here ?
@eerhardteerhardtSep 12, 2018

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.

It works, you just need to have the right syntax. I've updated the above with actual MSBuild code that works.
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Okay thanks :)
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

@Anipik
Anipik
commented
Sep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.228 (1803/April2018Update/Redstone4)
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=2.1.400
[Host] : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Job-QFXMOR : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree32.993 s0.5025 s0.4455 s-2762000.0000192000.000056000.000015435.34 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM31.045 s2.1895 s2.5215 s-1198000.0000560000.000084000.0000246.64 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree1.153 s0.0943 s0.1086 s-122000.000055000.000012000.00002.93 MB
@justinormont
justinormont
self-requested a review
September 12, 2018 19:35
@justinormontjustinormont
mentioned this pull request
Sep 12, 2018

Closed

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

sfilipi
sfilipi
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

 Include="..\data\external\WikiDetoxAnnotated160kRows.tsv"
 Link="external\WikiDetoxAnnotated160kRows.tsv">

 <TlcResourceFile Update="@(TlcResourceFile)">
@sfilipisfilipiSep 13, 2018

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.

TlcResourceFile [](start = 31, length = 15)
call it something non TLC
@eerhardteerhardtSep 13, 2018

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.

Agreed, but the reason I chose the name originally is because of the URL:
http://aka.ms/tlc-resources
Can we change this URL? Or make a new aka.ms URL pointing to the same location with a different name?
sfilipi
sfilipi
approved these changes
Sep 13, 2018
View reviewed changes
@sfilipisfilipi
left a comment


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.

:shipit:
justinormont
justinormont
approved these changes
Sep 13, 2018
View reviewed changes
@justinormontjustinormont
left a comment


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.

LGTM.
Let's wait to merge until the MSLR-WEB10K dataset is available in the CDN.
eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
approved these changes
Sep 13, 2018
View reviewed changes
@eerhardteerhardt
left a comment


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.

:shipit:
@justinormont
justinormont
commented
Sep 13, 2018

Copy link


Copy Markdown

Contributor
Please also add the citation to the MSLR-WEB10K dataset.
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated


namespace Microsoft.ML.Benchmarks
{
 [WarmupCount(8)] // It helps to reduce the standard deviation of these tests.
@justinormontjustinormontSep 14, 2018•

edited

Loading

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.

The normal user is unlikely to pre-train a model 8 times before training their model. This will be representative of the steady state reached when a model is retrained many times, but not very representative of the normal user's interaction w/ ML.net.
Do we know what's causing the time difference between the first run and the later runs? The first run is most representative of what a normal user will experience.
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@adamsitnik can you give us a better view here ? Increasing the warmup iterations leads to reducing the standard deviation here
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@justinormont do u want me to reduce it ?
cc @danmosemsft
@adamsitnikadamsitnikSep 17, 2018•

edited

Loading

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 you give us a better view here ?
@Anipik Unfortunately, it's not that simple. To find out why given benchmark behaves differently for different warmup counts we would have to profile it. It could be that OS gets warmed up and reading the input files becomes faster or anything like that.
The normal user is unlikely to pre-train a model 8 times before training their model.
@justinormont I agree. In that case we should set the WarmupCount to 0, IterationCount to 1 and LaunchCount to 20. Which means that BenchmarkDotNet is going to start a new process 20 times and each time execute the benchmark only once, without any warmup (the real use case) and just exit the process.
Edit: we should most probably have two configs: one for training benchmarks (no warmups) and one for prediction benchmarks (the one we have today)
@AnipikAnipikSep 17, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will revert warmupCount to 1 for this PR to get merged, we can later follow up with 2 config files as adam suggested
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
suggested changes
Sep 14, 2018
View reviewed changes
@justinormontjustinormont
left a comment
•

edited

Loading

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.

Looks good, though there's a couple minor things:
eerhardt
eerhardt
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 17, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont cam you take a look here ?
justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont can you merge this ? I don't have the write access to the repo ?
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
I'm going to close & re-open this pull request to notify the CI to re-check this PR.
@justinormont
justinormont
commented
Sep 18, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI test are failing. Though GitHub says 'in progress', it will say failed soon.
The winequality-white.csv file is the cause.
Error:
2018-09-18T17:27:13.4451915Z System.IO.IOException : Could not find file 'D:\a\1\s\test\data\external\winequality-white.csv'
2018-09-18T17:27:13.4452129Z Stack Trace:
2018-09-18T17:27:13.4452368Z at Microsoft.ML.Runtime.Data.MultiFileSource..ctor(String path) in D:\a\1\s\src\Microsoft.ML.Data\DataLoadSave\MultiFileSource.cs:line 31
2018-09-18T17:27:13.4452691Z at Microsoft.ML.StaticPipelineTesting.Training.SdcaRegression() in D:\a\1\s\test\Microsoft.ML.StaticPipelineTesting\Training.cs:line 29
2018-09-18T17:27:13.4452938Z 2018-09-18T17:27:13.4453436Z Results File: D:\a\1\s\bin/AnyCPU.Release\Microsoft.ML.StaticPipelineTesting\VssAdministrator_factoryvm-az385_2018-09-18_17_27_12.trx
2018-09-18T17:27:13.4454016Z 2018-09-18T17:27:13.4454366Z Total tests: 13. Passed: 9. Failed: 4. Skipped: 0.

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
Looking into it
@eerhardt
eerhardt
commented
Sep 18, 2018

Copy link


Copy Markdown

Member
It's a well-known issue that the wine dataset isn't working right now. @artidoro is working on it.
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
Related issue for the Wine dataset: #889Hot linking to a UCI dataset
Currently, the UCI web server is non-responsive, causing the dataset to not download, and the test to fail.
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont the ci is green, can we go ahead and merge this one ?
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
@Anipik, the merge is waiting on a merge conflict, can you look in to it?
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont i resolved the conflict
@justinormont
Add missing semicolon
eb738d2
Missing semicolon is causing the build the fail:
```
2018-09-19T04:41:39.2181028Z Datasets.cs(171,10): error CS1002: ; expected [/__w/3/s/test/Microsoft.ML.TestFramework/Microsoft.ML.TestFramework.csproj]
2018-09-19T04:41:39.7812509Z Microsoft.ML.StandardLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.StandardLearners/netstandard2.0/Microsoft.ML.StandardLearners.dll
2018-09-19T04:41:40.7120753Z Microsoft.ML.HalLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.HalLearners/netstandard2.0/Microsoft.ML.HalLearners.dll
2018-09-19T04:41:40.8804119Z Microsoft.ML.Ensemble -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Ensemble/netstandard2.0/Microsoft.ML.Ensemble.dll
2018-09-19T04:41:40.9555420Z Microsoft.ML.LightGBM -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.LightGBM/netstandard2.0/Microsoft.ML.LightGBM.dll
2018-09-19T04:41:41.5610322Z Microsoft.ML.PipelineInference -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.PipelineInference/netstandard2.0/Microsoft.ML.PipelineInference.dll
2018-09-19T04:41:42.4887819Z Microsoft.ML.Console -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Console/netcoreapp2.0/MML.dll
2018-09-19T04:41:45.7637388Z Microsoft.ML.FSharp.Tests -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.FSharp.Tests/netcoreapp2.1/Microsoft.ML.FSharp.Tests.dll
2018-09-19T04:41:45.7926386Z /__w/3/s/dir.traversal.targets(25,5): error : Build failed. See earlier errors. [/__w/3/s/build.proj]
2018-09-19T04:41:45.8133725Z 2018-09-19T04:41:45.8152732Z Build FAILED.
```
@justinormont
justinormont
merged commit 86f4d93
into
dotnet:masterSep 19, 2018
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
Thanks @Anipik for all the unexpected work needed in this pull request.
@Anipik
Anipik
deleted the

NumericRanking

branch
October 10, 2018 18:22
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


5 more reviewers

@justinormontjustinormontjustinormont approved these changes
@adamsitnikadamsitnikadamsitnik left review comments
@danmoseleydanmoseleydanmoseley left review comments
@eerhardteerhardteerhardt approved these changes
@sfilipisfilipisfilipi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Anipik@justinormont@eerhardt@adamsitnik@danmoseley@sfilipi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Added numeric ranking Performance Tests - #888
Merged
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Sep 19, 2018
Merged
Added numeric ranking Performance Tests#888
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Conversation
@Anipik
@AnipikAnipik
commented
Sep 11, 2018

Copy link


Copy Markdown

Contributor
Added benchmarking performance tests for Numeric ranking.
cc @justinormont@sfilipi @danmosemsft @eerhardt@shauheen
@Anipik
Anipik
commented
Sep 11, 2018

Copy link


Copy Markdown

ContributorAuthor
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 72.7866 s, StdErr = 0.5849 s (0.80%); N = 20, StdDev = 2.6156 sMin = 68.6174 s, Q1 = 70.0802 s, Median = 73.3494 s, Q3 = 74.9717 s, Max = 77.5199 s
IQR = 4.8915 s, LowerFence = 62.7429 s, UpperFence = 82.3089 s
ConfidenceInterval = [70.5153 s; 75.0579 s] (CI 99.9%), Margin = 2.2713 s (3.12% of Mean)Skewness = -0.09, Kurtosis = 1.66, MValue = 3.14
-------------------- Histogram --------------------
[68.616 s ; 70.533 s) | @@@@@@
[70.533 s ; 72.219 s) | @@
[72.219 s ; 74.846 s) | @@@@@@@
[74.846 s ; 76.379 s) | @@@@
[76.379 s ; 78.363 s) | @
---------------------------------------------------
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 65.4654 s, StdErr = 0.9955 s (1.52%); N = 20, StdDev = 4.4522 sMin = 58.3025 s, Q1 = 62.0859 s, Median = 65.7175 s, Q3 = 68.5847 s, Max = 75.8618 s
IQR = 6.4988 s, LowerFence = 52.3377 s, UpperFence = 78.3329 s
ConfidenceInterval = [61.5993 s; 69.3315 s] (CI 99.9%), Margin = 3.8661 s (5.91% of Mean)Skewness = 0.1, Kurtosis = 2.64, MValue = 3.25
-------------------- Histogram --------------------
[58.224 s ; 61.721 s) | @@@@@
[61.721 s ; 64.643 s) | @
[64.643 s ; 67.513 s) | @@@@@@@@
[67.513 s ; 70.412 s) | @@@@@
[70.412 s ; 74.427 s) |
[74.427 s ; 77.297 s) | @
---------------------------------------------------
Ranking.Test_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 4.5715 s, StdErr = 0.0079 s (0.17%); N = 14, StdDev = 0.0295 sMin = 4.5317 s, Q1 = 4.5526 s, Median = 4.5653 s, Q3 = 4.5778 s, Max = 4.6321 s
IQR = 0.0252 s, LowerFence = 4.5147 s, UpperFence = 4.6157 s
ConfidenceInterval = [4.5382 s; 4.6047 s] (CI 99.9%), Margin = 0.0333 s (0.73% of Mean)Skewness = 0.67, Kurtosis = 2.39, MValue = 2
-------------------- Histogram --------------------
[4.524 s ; 4.643 s) | @@@@@@@@@@@@@@
---------------------------------------------------
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree72.787 s2.2713 s2.6156 s-6247000.00001058000.0000286000.000026171.31 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM65.465 s3.8661 s4.4522 s-3595000.00001684000.0000267000.0000304.59 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree4.571 s0.0333 s0.0295 s-558000.0000279000.00001000.000011.93 MB
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(IncludeBenchmarkData)' == 'true'" > 
 <TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/WikiDetoxAnnotated160kRows.tsv"
@eerhardteerhardtSep 12, 2018•

edited

Loading

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.

The duplication here could be simplified using MSBuild. Something along the lines of:
<ItemGroup>
<TlcResourceFileInclude="WikiDetoxAnnotated160kRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTrain3.6MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KValidate1.2MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTest1.2MRows.tsv" />
<TlcResourceFileUpdate="@(TlcResourceFile)">
<Url>http://aka.ms/tlc-resources/benchmarks/%(Identity)</Url>
<DestinationFile>$(MSBuildThisFileDirectory)test/data/external/%(Identity)</DestinationFile>
</TlcResourceFile>
<TestFileInclude="@(TlcResourceFile->'$(MSBuildThisFileDirectory)/test/data/external/%(Identity)')" />
</ItemGroup>
I'm not 100% sure it is better, but it reduces the number of times these URLs need to be copied.
@danmoseleydanmoseleySep 12, 2018

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

That's batching, I think it would only work within a <Target> ?
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep dan is right. its not working in this case. any other suggestion here ?
@eerhardteerhardtSep 12, 2018

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.

It works, you just need to have the right syntax. I've updated the above with actual MSBuild code that works.
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Okay thanks :)
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

@Anipik
Anipik
commented
Sep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.228 (1803/April2018Update/Redstone4)
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=2.1.400
[Host] : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Job-QFXMOR : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree32.993 s0.5025 s0.4455 s-2762000.0000192000.000056000.000015435.34 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM31.045 s2.1895 s2.5215 s-1198000.0000560000.000084000.0000246.64 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree1.153 s0.0943 s0.1086 s-122000.000055000.000012000.00002.93 MB
@justinormont
justinormont
self-requested a review
September 12, 2018 19:35
@justinormontjustinormont
mentioned this pull request
Sep 12, 2018

Closed

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

sfilipi
sfilipi
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

 Include="..\data\external\WikiDetoxAnnotated160kRows.tsv"
 Link="external\WikiDetoxAnnotated160kRows.tsv">

 <TlcResourceFile Update="@(TlcResourceFile)">
@sfilipisfilipiSep 13, 2018

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.

TlcResourceFile [](start = 31, length = 15)
call it something non TLC
@eerhardteerhardtSep 13, 2018

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.

Agreed, but the reason I chose the name originally is because of the URL:
http://aka.ms/tlc-resources
Can we change this URL? Or make a new aka.ms URL pointing to the same location with a different name?
sfilipi
sfilipi
approved these changes
Sep 13, 2018
View reviewed changes
@sfilipisfilipi
left a comment


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.

:shipit:
justinormont
justinormont
approved these changes
Sep 13, 2018
View reviewed changes
@justinormontjustinormont
left a comment


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.

LGTM.
Let's wait to merge until the MSLR-WEB10K dataset is available in the CDN.
eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
approved these changes
Sep 13, 2018
View reviewed changes
@eerhardteerhardt
left a comment


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.

:shipit:
@justinormont
justinormont
commented
Sep 13, 2018

Copy link


Copy Markdown

Contributor
Please also add the citation to the MSLR-WEB10K dataset.
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated


namespace Microsoft.ML.Benchmarks
{
 [WarmupCount(8)] // It helps to reduce the standard deviation of these tests.
@justinormontjustinormontSep 14, 2018•

edited

Loading

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.

The normal user is unlikely to pre-train a model 8 times before training their model. This will be representative of the steady state reached when a model is retrained many times, but not very representative of the normal user's interaction w/ ML.net.
Do we know what's causing the time difference between the first run and the later runs? The first run is most representative of what a normal user will experience.
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@adamsitnik can you give us a better view here ? Increasing the warmup iterations leads to reducing the standard deviation here
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@justinormont do u want me to reduce it ?
cc @danmosemsft
@adamsitnikadamsitnikSep 17, 2018•

edited

Loading

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 you give us a better view here ?
@Anipik Unfortunately, it's not that simple. To find out why given benchmark behaves differently for different warmup counts we would have to profile it. It could be that OS gets warmed up and reading the input files becomes faster or anything like that.
The normal user is unlikely to pre-train a model 8 times before training their model.
@justinormont I agree. In that case we should set the WarmupCount to 0, IterationCount to 1 and LaunchCount to 20. Which means that BenchmarkDotNet is going to start a new process 20 times and each time execute the benchmark only once, without any warmup (the real use case) and just exit the process.
Edit: we should most probably have two configs: one for training benchmarks (no warmups) and one for prediction benchmarks (the one we have today)
@AnipikAnipikSep 17, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will revert warmupCount to 1 for this PR to get merged, we can later follow up with 2 config files as adam suggested
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
suggested changes
Sep 14, 2018
View reviewed changes
@justinormontjustinormont
left a comment
•

edited

Loading

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.

Looks good, though there's a couple minor things:
eerhardt
eerhardt
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 17, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont cam you take a look here ?
justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont can you merge this ? I don't have the write access to the repo ?
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
I'm going to close & re-open this pull request to notify the CI to re-check this PR.
@justinormont
justinormont
commented
Sep 18, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI test are failing. Though GitHub says 'in progress', it will say failed soon.
The winequality-white.csv file is the cause.
Error:
2018-09-18T17:27:13.4451915Z System.IO.IOException : Could not find file 'D:\a\1\s\test\data\external\winequality-white.csv'
2018-09-18T17:27:13.4452129Z Stack Trace:
2018-09-18T17:27:13.4452368Z at Microsoft.ML.Runtime.Data.MultiFileSource..ctor(String path) in D:\a\1\s\src\Microsoft.ML.Data\DataLoadSave\MultiFileSource.cs:line 31
2018-09-18T17:27:13.4452691Z at Microsoft.ML.StaticPipelineTesting.Training.SdcaRegression() in D:\a\1\s\test\Microsoft.ML.StaticPipelineTesting\Training.cs:line 29
2018-09-18T17:27:13.4452938Z 2018-09-18T17:27:13.4453436Z Results File: D:\a\1\s\bin/AnyCPU.Release\Microsoft.ML.StaticPipelineTesting\VssAdministrator_factoryvm-az385_2018-09-18_17_27_12.trx
2018-09-18T17:27:13.4454016Z 2018-09-18T17:27:13.4454366Z Total tests: 13. Passed: 9. Failed: 4. Skipped: 0.

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
Looking into it
@eerhardt
eerhardt
commented
Sep 18, 2018

Copy link


Copy Markdown

Member
It's a well-known issue that the wine dataset isn't working right now. @artidoro is working on it.
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
Related issue for the Wine dataset: #889Hot linking to a UCI dataset
Currently, the UCI web server is non-responsive, causing the dataset to not download, and the test to fail.
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont the ci is green, can we go ahead and merge this one ?
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
@Anipik, the merge is waiting on a merge conflict, can you look in to it?
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont i resolved the conflict
@justinormont
Add missing semicolon
eb738d2
Missing semicolon is causing the build the fail:
```
2018-09-19T04:41:39.2181028Z Datasets.cs(171,10): error CS1002: ; expected [/__w/3/s/test/Microsoft.ML.TestFramework/Microsoft.ML.TestFramework.csproj]
2018-09-19T04:41:39.7812509Z Microsoft.ML.StandardLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.StandardLearners/netstandard2.0/Microsoft.ML.StandardLearners.dll
2018-09-19T04:41:40.7120753Z Microsoft.ML.HalLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.HalLearners/netstandard2.0/Microsoft.ML.HalLearners.dll
2018-09-19T04:41:40.8804119Z Microsoft.ML.Ensemble -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Ensemble/netstandard2.0/Microsoft.ML.Ensemble.dll
2018-09-19T04:41:40.9555420Z Microsoft.ML.LightGBM -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.LightGBM/netstandard2.0/Microsoft.ML.LightGBM.dll
2018-09-19T04:41:41.5610322Z Microsoft.ML.PipelineInference -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.PipelineInference/netstandard2.0/Microsoft.ML.PipelineInference.dll
2018-09-19T04:41:42.4887819Z Microsoft.ML.Console -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Console/netcoreapp2.0/MML.dll
2018-09-19T04:41:45.7637388Z Microsoft.ML.FSharp.Tests -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.FSharp.Tests/netcoreapp2.1/Microsoft.ML.FSharp.Tests.dll
2018-09-19T04:41:45.7926386Z /__w/3/s/dir.traversal.targets(25,5): error : Build failed. See earlier errors. [/__w/3/s/build.proj]
2018-09-19T04:41:45.8133725Z 2018-09-19T04:41:45.8152732Z Build FAILED.
```
@justinormont
justinormont
merged commit 86f4d93
into
dotnet:masterSep 19, 2018
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
Thanks @Anipik for all the unexpected work needed in this pull request.
@Anipik
Anipik
deleted the

NumericRanking

branch
October 10, 2018 18:22
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


5 more reviewers

@justinormontjustinormontjustinormont approved these changes
@adamsitnikadamsitnikadamsitnik left review comments
@danmoseleydanmoseleydanmoseley left review comments
@eerhardteerhardteerhardt approved these changes
@sfilipisfilipisfilipi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Anipik@justinormont@eerhardt@adamsitnik@danmoseley@sfilipi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Added numeric ranking Performance Tests - #888
Merged
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Sep 19, 2018
Merged
Added numeric ranking Performance Tests#888
justinormont merged 10 commits into
dotnet:masterfrom 
Anipik:NumericRanking
Conversation
@Anipik
@AnipikAnipik
commented
Sep 11, 2018

Copy link


Copy Markdown

Contributor
Added benchmarking performance tests for Numeric ranking.
cc @justinormont@sfilipi @danmosemsft @eerhardt@shauheen
@Anipik
Anipik
commented
Sep 11, 2018

Copy link


Copy Markdown

ContributorAuthor
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 72.7866 s, StdErr = 0.5849 s (0.80%); N = 20, StdDev = 2.6156 sMin = 68.6174 s, Q1 = 70.0802 s, Median = 73.3494 s, Q3 = 74.9717 s, Max = 77.5199 s
IQR = 4.8915 s, LowerFence = 62.7429 s, UpperFence = 82.3089 s
ConfidenceInterval = [70.5153 s; 75.0579 s] (CI 99.9%), Margin = 2.2713 s (3.12% of Mean)Skewness = -0.09, Kurtosis = 1.66, MValue = 3.14
-------------------- Histogram --------------------
[68.616 s ; 70.533 s) | @@@@@@
[70.533 s ; 72.219 s) | @@
[72.219 s ; 74.846 s) | @@@@@@@
[74.846 s ; 76.379 s) | @@@@
[76.379 s ; 78.363 s) | @
---------------------------------------------------
Ranking.TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 65.4654 s, StdErr = 0.9955 s (1.52%); N = 20, StdDev = 4.4522 sMin = 58.3025 s, Q1 = 62.0859 s, Median = 65.7175 s, Q3 = 68.5847 s, Max = 75.8618 s
IQR = 6.4988 s, LowerFence = 52.3377 s, UpperFence = 78.3329 s
ConfidenceInterval = [61.5993 s; 69.3315 s] (CI 99.9%), Margin = 3.8661 s (5.91% of Mean)Skewness = 0.1, Kurtosis = 2.64, MValue = 3.25
-------------------- Histogram --------------------
[58.224 s ; 61.721 s) | @@@@@
[61.721 s ; 64.643 s) | @
[64.643 s ; 67.513 s) | @@@@@@@@
[67.513 s ; 70.412 s) | @@@@@
[70.412 s ; 74.427 s) |
[74.427 s ; 77.297 s) | @
---------------------------------------------------
Ranking.Test_Multiclass_MSLRWeb10K_Ranking_FastTree: Job-ZGJLIB(Toolchain=netcoreapp2.1, MaxIterationCount=20, WarmupCount=1)
Mean = 4.5715 s, StdErr = 0.0079 s (0.17%); N = 14, StdDev = 0.0295 sMin = 4.5317 s, Q1 = 4.5526 s, Median = 4.5653 s, Q3 = 4.5778 s, Max = 4.6321 s
IQR = 0.0252 s, LowerFence = 4.5147 s, UpperFence = 4.6157 s
ConfidenceInterval = [4.5382 s; 4.6047 s] (CI 99.9%), Margin = 0.0333 s (0.73% of Mean)Skewness = 0.67, Kurtosis = 2.39, MValue = 2
-------------------- Histogram --------------------
[4.524 s ; 4.643 s) | @@@@@@@@@@@@@@
---------------------------------------------------
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree72.787 s2.2713 s2.6156 s-6247000.00001058000.0000286000.000026171.31 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM65.465 s3.8661 s4.4522 s-3595000.00001684000.0000267000.0000304.59 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree4.571 s0.0333 s0.0295 s-558000.0000279000.00001000.000011.93 MB
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

 </ItemGroup>

 <ItemGroup Condition="'$(IncludeBenchmarkData)' == 'true'" > 
 <TestFile Include="$(MSBuildThisFileDirectory)/test/data/external/WikiDetoxAnnotated160kRows.tsv"
@eerhardteerhardtSep 12, 2018•

edited

Loading

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.

The duplication here could be simplified using MSBuild. Something along the lines of:
<ItemGroup>
<TlcResourceFileInclude="WikiDetoxAnnotated160kRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTrain3.6MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KValidate1.2MRows.tsv" />
<TlcResourceFileInclude="MSLRWeb10KTest1.2MRows.tsv" />
<TlcResourceFileUpdate="@(TlcResourceFile)">
<Url>http://aka.ms/tlc-resources/benchmarks/%(Identity)</Url>
<DestinationFile>$(MSBuildThisFileDirectory)test/data/external/%(Identity)</DestinationFile>
</TlcResourceFile>
<TestFileInclude="@(TlcResourceFile->'$(MSBuildThisFileDirectory)/test/data/external/%(Identity)')" />
</ItemGroup>
I'm not 100% sure it is better, but it reduces the number of times these URLs need to be copied.
@danmoseleydanmoseleySep 12, 2018

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

That's batching, I think it would only work within a <Target> ?
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep dan is right. its not working in this case. any other suggestion here ?
@eerhardteerhardtSep 12, 2018

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.

It works, you just need to have the right syntax. I've updated the above with actual MSBuild code that works.
@AnipikAnipikSep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Okay thanks :)
eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

eerhardt
eerhardt
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

@Anipik
Anipik
commented
Sep 12, 2018

Copy link


Copy Markdown

ContributorAuthor
BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.228 (1803/April2018Update/Redstone4)
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=2.1.400
[Host] : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Job-QFXMOR : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
Toolchain=netcoreapp2.1 MaxIterationCount=20 WarmupCount=1 
MethodMeanErrorStdDevExtra MetricGen 0Gen 1Gen 2Allocated
TrainTest_Multiclass_MSLRWeb10K_Ranking_FastTree32.993 s0.5025 s0.4455 s-2762000.0000192000.000056000.000015435.34 MB
TrainTest_Multiclass_MSLRWeb10K_Ranking_LightGBM31.045 s2.1895 s2.5215 s-1198000.0000560000.000084000.0000246.64 MB
Test_Multiclass_MSLRWeb10K_Ranking_FastTree1.153 s0.0943 s0.1086 s-122000.000055000.000012000.00002.93 MB
@justinormont
justinormont
self-requested a review
September 12, 2018 19:35
@justinormontjustinormont
mentioned this pull request
Sep 12, 2018

Closed

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 12, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated

sfilipi
sfilipi
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

 Include="..\data\external\WikiDetoxAnnotated160kRows.tsv"
 Link="external\WikiDetoxAnnotated160kRows.tsv">

 <TlcResourceFile Update="@(TlcResourceFile)">
@sfilipisfilipiSep 13, 2018

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.

TlcResourceFile [](start = 31, length = 15)
call it something non TLC
@eerhardteerhardtSep 13, 2018

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.

Agreed, but the reason I chose the name originally is because of the URL:
http://aka.ms/tlc-resources
Can we change this URL? Or make a new aka.ms URL pointing to the same location with a different name?
sfilipi
sfilipi
approved these changes
Sep 13, 2018
View reviewed changes
@sfilipisfilipi
left a comment


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.

:shipit:
justinormont
justinormont
approved these changes
Sep 13, 2018
View reviewed changes
@justinormontjustinormont
left a comment


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.

LGTM.
Let's wait to merge until the MSLR-WEB10K dataset is available in the CDN.
eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
reviewed
Sep 13, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Microsoft.ML.Benchmarks.csproj
Outdated

eerhardt
eerhardt
approved these changes
Sep 13, 2018
View reviewed changes
@eerhardteerhardt
left a comment


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.

:shipit:
@justinormont
justinormont
commented
Sep 13, 2018

Copy link


Copy Markdown

Contributor
Please also add the citation to the MSLR-WEB10K dataset.
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadbuild.proj
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/Microsoft.ML.Benchmarks/Numeric/Ranking.cs
Outdated


namespace Microsoft.ML.Benchmarks
{
 [WarmupCount(8)] // It helps to reduce the standard deviation of these tests.
@justinormontjustinormontSep 14, 2018•

edited

Loading

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.

The normal user is unlikely to pre-train a model 8 times before training their model. This will be representative of the steady state reached when a model is retrained many times, but not very representative of the normal user's interaction w/ ML.net.
Do we know what's causing the time difference between the first run and the later runs? The first run is most representative of what a normal user will experience.
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@adamsitnik can you give us a better view here ? Increasing the warmup iterations leads to reducing the standard deviation here
@AnipikAnipikSep 14, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@justinormont do u want me to reduce it ?
cc @danmosemsft
@adamsitnikadamsitnikSep 17, 2018•

edited

Loading

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 you give us a better view here ?
@Anipik Unfortunately, it's not that simple. To find out why given benchmark behaves differently for different warmup counts we would have to profile it. It could be that OS gets warmed up and reading the input files becomes faster or anything like that.
The normal user is unlikely to pre-train a model 8 times before training their model.
@justinormont I agree. In that case we should set the WarmupCount to 0, IterationCount to 1 and LaunchCount to 20. Which means that BenchmarkDotNet is going to start a new process 20 times and each time execute the benchmark only once, without any warmup (the real use case) and just exit the process.
Edit: we should most probably have two configs: one for training benchmarks (no warmups) and one for prediction benchmarks (the one we have today)
@AnipikAnipikSep 17, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will revert warmupCount to 1 for this PR to get merged, we can later follow up with 2 config files as adam suggested
justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
suggested changes
Sep 14, 2018
View reviewed changes
@justinormontjustinormont
left a comment
•

edited

Loading

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.

Looks good, though there's a couple minor things:
eerhardt
eerhardt
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 14, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 17, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont cam you take a look here ?
justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

justinormont
justinormont
reviewed
Sep 18, 2018
View reviewed changes
Comment threadtest/data/README.md
Outdated

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont can you merge this ? I don't have the write access to the repo ?
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
I'm going to close & re-open this pull request to notify the CI to re-check this PR.
@justinormont
justinormont
commented
Sep 18, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI test are failing. Though GitHub says 'in progress', it will say failed soon.
The winequality-white.csv file is the cause.
Error:
2018-09-18T17:27:13.4451915Z System.IO.IOException : Could not find file 'D:\a\1\s\test\data\external\winequality-white.csv'
2018-09-18T17:27:13.4452129Z Stack Trace:
2018-09-18T17:27:13.4452368Z at Microsoft.ML.Runtime.Data.MultiFileSource..ctor(String path) in D:\a\1\s\src\Microsoft.ML.Data\DataLoadSave\MultiFileSource.cs:line 31
2018-09-18T17:27:13.4452691Z at Microsoft.ML.StaticPipelineTesting.Training.SdcaRegression() in D:\a\1\s\test\Microsoft.ML.StaticPipelineTesting\Training.cs:line 29
2018-09-18T17:27:13.4452938Z 2018-09-18T17:27:13.4453436Z Results File: D:\a\1\s\bin/AnyCPU.Release\Microsoft.ML.StaticPipelineTesting\VssAdministrator_factoryvm-az385_2018-09-18_17_27_12.trx
2018-09-18T17:27:13.4454016Z 2018-09-18T17:27:13.4454366Z Total tests: 13. Passed: 9. Failed: 4. Skipped: 0.

@Anipik
Anipik
commented
Sep 18, 2018

Copy link


Copy Markdown

ContributorAuthor
Looking into it
@eerhardt
eerhardt
commented
Sep 18, 2018

Copy link


Copy Markdown

Member
It's a well-known issue that the wine dataset isn't working right now. @artidoro is working on it.
@justinormont
justinormont
commented
Sep 18, 2018

Copy link


Copy Markdown

Contributor
Related issue for the Wine dataset: #889Hot linking to a UCI dataset
Currently, the UCI web server is non-responsive, causing the dataset to not download, and the test to fail.
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont the ci is green, can we go ahead and merge this one ?
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
@Anipik, the merge is waiting on a merge conflict, can you look in to it?
@Anipik
Anipik
commented
Sep 19, 2018

Copy link


Copy Markdown

ContributorAuthor
@justinormont i resolved the conflict
@justinormont
Add missing semicolon
eb738d2
Missing semicolon is causing the build the fail:
```
2018-09-19T04:41:39.2181028Z Datasets.cs(171,10): error CS1002: ; expected [/__w/3/s/test/Microsoft.ML.TestFramework/Microsoft.ML.TestFramework.csproj]
2018-09-19T04:41:39.7812509Z Microsoft.ML.StandardLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.StandardLearners/netstandard2.0/Microsoft.ML.StandardLearners.dll
2018-09-19T04:41:40.7120753Z Microsoft.ML.HalLearners -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.HalLearners/netstandard2.0/Microsoft.ML.HalLearners.dll
2018-09-19T04:41:40.8804119Z Microsoft.ML.Ensemble -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Ensemble/netstandard2.0/Microsoft.ML.Ensemble.dll
2018-09-19T04:41:40.9555420Z Microsoft.ML.LightGBM -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.LightGBM/netstandard2.0/Microsoft.ML.LightGBM.dll
2018-09-19T04:41:41.5610322Z Microsoft.ML.PipelineInference -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.PipelineInference/netstandard2.0/Microsoft.ML.PipelineInference.dll
2018-09-19T04:41:42.4887819Z Microsoft.ML.Console -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.Console/netcoreapp2.0/MML.dll
2018-09-19T04:41:45.7637388Z Microsoft.ML.FSharp.Tests -> /__w/3/s/bin/AnyCPU.Debug/Microsoft.ML.FSharp.Tests/netcoreapp2.1/Microsoft.ML.FSharp.Tests.dll
2018-09-19T04:41:45.7926386Z /__w/3/s/dir.traversal.targets(25,5): error : Build failed. See earlier errors. [/__w/3/s/build.proj]
2018-09-19T04:41:45.8133725Z 2018-09-19T04:41:45.8152732Z Build FAILED.
```
@justinormont
justinormont
merged commit 86f4d93
into
dotnet:masterSep 19, 2018
@justinormont
justinormont
commented
Sep 19, 2018

Copy link


Copy Markdown

Contributor
Thanks @Anipik for all the unexpected work needed in this pull request.
@Anipik
Anipik
deleted the

NumericRanking

branch
October 10, 2018 18:22
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 29, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


5 more reviewers

@justinormontjustinormontjustinormont approved these changes
@adamsitnikadamsitnikadamsitnik left review comments
@danmoseleydanmoseleydanmoseley left review comments
@eerhardteerhardteerhardt approved these changes
@sfilipisfilipisfilipi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Anipik@justinormont@eerhardt@adamsitnik@danmoseley@sfilipi