Uh oh!
There was an error while loading. Please reload this page.
Support for custom metrics reported in the Benchmarks - #735
Conversation
…r selected benchmarks, not all
… rely on hardcoded folder hierarchy
…ed in a dedicated process
…ot as decimal separator (and it fails for cultures with ",")
…osoft.ML.Runtime.IHost and BenchmarkDotNet.Engines.IHost..
shauheen
commented
Aug 28, 2018
Thanks @adamsitnik , can you please associate this with the relevant issue? |
| public int PriorityInCategory => 1; | ||
| public UnitType UnitType => UnitType.Dimensionless; | ||
| // enforce Neutral Language as "en-us" because the input data files use dot as decimal separator (and it fails for cultures with ",") | ||
| Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture; |
There was a problem hiding this comment.
This line of code is a bit surprising in a method that is supposed to return a data path. Maybe it would be better to do this in the Main method, or a GlobalSetup method?
There was a problem hiding this comment.
@eerhardt I agree that I am breaking CQRS here. My only excuse is that I have named the method GetInvariantCultureDataPath so people can expect that.
I was thinking about moving it to a [GlobalSetup] method but I am afraid that people will don't follow this pattern in new benchmarks. By having it here I guarantee that whoever is going to use files will be using CultureInfo.InvariantCulture for reading these files.
I also wonder how ML.NET samples deal with the culture info problem. Does anybody know?
| [GlobalCleanup] | ||
| public void ReportMetrics() | ||
| { | ||
| foreach (var metric in GetMetrics()) |
There was a problem hiding this comment.
nit: Would it improve perf to set var metrics = GetMetrics(); right before the foreach loop and then write the condition as var metric in metrics? Not sure...
There was a problem hiding this comment.
@briancylui no, it would not.
Whenever you are not sure about something you can benchmark it with BenchmarkDotNet ;)
| var foldeWithAutogeneratedExe = Path.GetDirectoryName(artifactsPaths.ExecutablePath); | ||
| var folderWithNativeDependencies = Path.GetDirectoryName(typeof(ProjectGenerator).Assembly.Location); | ||
| foreach(var nativeDependency in Directory |
There was a problem hiding this comment.
nit: missing space between foreach and the succeeding (
| for (int bi = 0; bi < batch.Length; bi++) | ||
| { | ||
| batch[bi] = _example; | ||
| } |
There was a problem hiding this comment.
Does this for loop change elements of _batches[i] or only elements of the local variable batch? Not an expert so not sure whether batch is a ref-type.
There was a problem hiding this comment.
this is done on purpose, it's a Setup method
briancylui
left a comment
There was a problem hiding this comment.
Feel free to merge after responding to PR comments - I don't have write access so can't hit merge unfortunately. Not an expert in Benchmark.NET, but this PR looks good to me! Thanks @adamsitnik
briancylui
commented
Aug 29, 2018
More reviewers are needed for this PR to be merged - my review doesn't count towards mergeability since I don't have write access. |
adamsitnik
commented
Aug 29, 2018
@briancylui@eerhardt thank you for your reviews! I don't have write access myself, so who could merge it? @shauheen there is no issue, but there was an email thread. Do you want me to create an issue for that? |
eerhardt
commented
Aug 29, 2018
test OSX10.13 Debug |
briancylui
commented
Aug 29, 2018
test OSX10.13 Debug please |
# Conflicts: # build/Dependencies.props # test/Microsoft.ML.Benchmarks/KMeansAndLogisticRegressionBench.cs
This PR enables two things:
Why should we run every benchmark in a separate process?
Results when running all the benchmarks in the same process:
When running every benchmark in a dedicated process:
To run every benchmark in a standalone, dedicated process BenchmarkDotNet needs to be able to create, build and run new executable.
So far it was not possible out of the box due to MSBuild limitation. When
Microsoft.ML.Benchmarksreferences native assembly and the auto-generated BenchmarkDotNet project referencesMicrosoft.ML.Benchmarksthe native dependencies are not copied to the output folder of the auto-generated project with benchmarks. This is why I had to implementProjectGeneratorwhich does that for us.@eerhardt we had a conversation about making it possible for BenchmarkDotNet to compile ML.NET stuff a long time ago and the blocker was the native dependency.
The other thing are custom metrics. BenchmarkDotNet does not support it out of the box, I had to implement it. How it works:
WithExtraMetricsand implementIEnumerable<Metric> GetMetrics()methodWithExtraMetricsafter running the benchmarks prints the custom metrics to console in child processExtraMetricColumnparses the output in parent process.Sample results:
Other changes: so far the benchmarks were using
currentAssemblyLocation.Directory.Parent.Parent.Parent.Parent.FullNameto get the path to folder with input files. I believe it's better to reference them as links incsprojand "copy to output directory if newer". This solution is cleaner and more futureproof./cc @eerhardt @danmosemsft @briancylui@KrzysztofCwalina