-
Notifications
You must be signed in to change notification settings - Fork 21
Add Benchmarks #38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add Benchmarks #38
Conversation
|
@Lyrcaxis Could you review this when you have the time? |
|
@PhilippNaused Hey, thanks for the PR! I've reviewed this for some time now -- code looks good. I've been thinking on whether we want a benchmark project in the repo -- what value it would add, any pitfalls, etc. Pros: I guess it could get curious people somewhat more involved to making edits and comparing performance (?) So I thought I'd give it some time since it's not an engine feature, but sorry, it's indeed been a while 😅 (month+) Are you looking forward to using the Benchmark project for more stuff/automations/metrics/docs / expand on it? |
Lyrcaxis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Few comments and minor styling tweaks here and there.
Let's talk about the potential of this benchmark project for a bit first before merging though.
| var options2 = new Microsoft.ML.OnnxRuntime.SessionOptions(); | ||
| options2.AppendExecutionProvider_CUDA(); // Use CUDA for GPU inference. | ||
| models[(model, true)] = new KokoroModel(KokoroTTS.ModelNamesMap[model], options2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require the benchmarker to tweak a non-highest-level file if they don't use CUDA.
Preferrably this would be an option configurable from the highest-level Program.cs or the CLI args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best option would be to split the benchmark into two classes. One for comparing the models on CPU and one for CPU vs GPU.
e.g.,
public class Inference {
...
}
public class InferenceCPUvsCUDA : Inference {
...
}Then you can select which one you like from the CLI using BenchmarkDotNet built-in benchmark selector.
I'm just not too happy with the name InferenceCPUvsCUDA...
| using BenchmarkDotNet.Running; | ||
|
|
||
| BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args); No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of implicit Program/Main but could make sense here if it's a one-liner.
I think it shouldn't be a one-liner though and could have more options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean you would prefer this?
BenchmarkDotNet.Running.BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);6448a29 to
663be6e
Compare
Add a simple benchmark project.
It tests the performance of the tokenizer, text pre-processing and inference.
You can run it by calling:
dotnet run --project Benchmark -c ReleaseExample output: