Make backend names in JSON reports match burnbench CLI

[errordeveloper]

Pull Request Template

Checklist

• Confirmed that run-checks all script has been executed.
• Made sure the book is up to date with changes in this PR.

Related Issues/PRs

Provide links to relevant issues and dependent PRs.

Changes

Summarize the problem being addressed and your solution.

Testing

Describe how these changes have been tested.

[errordeveloper]

I found this quite handy and wanted to open it up for comments from maintainers.

I've used this to plot the data, which I can also contribute somewhere.

import json
import os
import seaborn as sns
import matplotlib.pyplot as plt
from pandas import DataFrame

data = {
    "benchmark": [],
    "backendConfigName": [],
    "mean": [],
    "median": [],
    "min": [],
    "max": []
}

for file in os.listdir("./"):
    if file.endswith(".json"):
        with open(file, "r") as f:
            result = json.load(f)

            data["benchmark"].append(result["name"])
            data["backendConfigName"].append(result["backendConfigName"])
            data["mean"].append(result["mean"] / 1000.0)  # convert to miliseconds keeping decimal point
            data["median"].append(result["median"])
            data["min"].append(result["min"])
            data["max"].append(result["max"])

plt.figure(figsize=(28, 20))

benchmarks = DataFrame(data)

p = sns.swarmplot(
    data=benchmarks,
    x="backendConfigName", y="mean", hue="benchmark",
    log_scale=False,
    legend=True,
    palette="Set2",
    s=10, marker="D", linewidth=1,
)

for i in p.get_ygridlines():
    i.set_linestyle(':')
    i.set_linewidth(0.5)
    i.set_visible(True)

plt.ylabel("Mean Duration (miliseconds)")
plt.xlabel("Backends")

plt.legend(title="Benchmark")

plt.xticks(rotation=45)

plt.show()

[errordeveloper]

I suppose this should be split into a separate commit. Ideally, it'd be great if this was in the benchmarking doc. It took me an attempt to change the formatting of results on stdout before I found out about the JSON files.

[nathanielsimard]

I don't think we should pass the backend name in all function calls, I would put it into the benchmark trait. In the future we might change that backend_name to any runtime information that we need to collect.

[errordeveloper]

@nathanielsimard sure, I've not spent any time thinking about the design, was just getting something to work and figured I'd open a PR and see if this is deemed useful at all.

[errordeveloper]

@nathanielsimard PTAL, I've moved the logic into Backend and Benchmark crates :)

[errordeveloper]

no so sure what to do here...

[errordeveloper]

I've moved this one to the top of the file since it seem right to have features config macros up-front, not inside the trait defintion, so this way it seemed more readable to me - no surprises lower-down surprises ;)

[nathanielsimard]

There is something that bothers me with the naming/implementation. There is a lot of overlapping information.

I think it might be better to have somthing as such:

fn device_features(device: &B::Device) -> Vec<String> {
    let mut features = Vec::new();
    
    #[cfg(feature = "std")]
    features.push("std".into());

    #[cfg(feature = "blas-openblas")]
    features.push("openblas".into());
    
    match device {
        Cuda => features.push("cuda".into());
        Metal => features.push("metal".into());
    };

    features
}

@syl20bnr do you have an opinion on this?

[nathanielsimard]

I would add candle-cuda and candle-metal in this case.

[syl20bnr]

I feel that we can do much simpler by sticking closer to what this PR is all about. We could just store the arguments passed on the command line. The serialized arguments would be an object burnbenchArgs with a field backend and a field bench (although this one is less valuable). If later we add some new options to the CLI it might be handy to add it to the benchmark results so that we can reconstruct the original command line for other people to try executing the same benchmark.

[errordeveloper]

@syl20bnr I agree in principle, although I'm not sure if I have more time for this. I did say in the PR title that it's about matching the names of backends, which is one surface area, but there are other ways of looking at it. Namely, I just want to plot the results and need keys that I could relate to. If burnbench just produced a plot for me, I'd also be quite happy and wouldn't need to worry about all this. So I'm not necessarily sure preservation of CLI arguments is the right UX facet to lean on, perhaps producing plots could be more useful, the rest is implementation detail. Of course, it depends on how far you want to take burnbench.

[syl20bnr]

I understand and the current interaction between burnbench and the actual benchmarks is a bit contrieved and it would need a bit of refactoring. Currently the benchmark itself is responsible of persistence and upload of results which should now be the responsibility of our burnbench runner. Once we moved these responsibilities to our runner it will be easier to serialize more contextual informations and more.

For the plots we are indeed working on it and it will soon be announced on our discord, so stay tune!

[errordeveloper]

So actually right now preserving CLI args would require some additional tricks, because results are stored per benchmark (and, e.g. data benchmark produces multiple files). Any burnbench invocations with multiple backends and multiple benchmarks would require some special treatment, as backends and benchmarks pass in the CLI would need to be mapped to results actual results and translated to a single invocation. Maybe it's not too hard as there is cargo bench invocation, but that will add more usability friction too, as backends map to features ;)

[syl20bnr]

We have been thinking about this and adding a new function to the backend trait just for this is too invasive. In the current situation the probably best option is to modify the macro in lib.rs to pass the feature name to the bench function and then to the save function for serialization. Then the name can be retrieved by the CLI to be displayed in the report.

[antimora]

@syl20bnr what do we do with this PR? Keep it open?

[syl20bnr]

Yes we can keep it open. @errordeveloper do you have time to change the implementation ? If not I can do it,

[errordeveloper]

The suggestion makes sense, however I don’t think I will find the time to implement it any time soon.

[antimora]

OK. I have assigned to @syl20bnr then. Thank you, @errordeveloper , for the update and PR.

[syl20bnr]

I made the changes. Here is an example of a report:

Benchmark	Feature	Backend	Device	Median
unary	wgpu	jit<wgpu>	BestAvailable	415.000µs
unary	wgpu-fusion	fusion<jit<wgpu>>	BestAvailable	295.000µs
unary	tch-cpu	tch	Cpu	3.226ms
unary	tch-gpu	tch	Cuda(0)	233.000µs
binary	wgpu	jit<wgpu>	BestAvailable	498.000µs
binary	wgpu-fusion	fusion<jit<wgpu>>	BestAvailable	404.000µs
binary	tch-cpu	tch	Cpu	3.175ms
binary	tch-gpu	tch	Cuda(0)	360.000µs
to_data	wgpu	jit<wgpu>	BestAvailable	29.006ms
from_data	wgpu	jit<wgpu>	BestAvailable	20.764ms
to_data	wgpu-fusion	fusion<jit<wgpu>>	BestAvailable	28.169ms
from_data	wgpu-fusion	fusion<jit<wgpu>>	BestAvailable	21.100ms
to_data	tch-cpu	tch	Cpu	12.120ms
from_data	tch-cpu	tch	Cpu	19.934ms
to_data	tch-gpu	tch	Cuda(0)	14.957ms
from_data	tch-gpu	tch	Cuda(0)	27.786ms
matmul	wgpu	jit<wgpu>	BestAvailable	867.000µs
matmul	wgpu-fusion	fusion<jit<wgpu>>	BestAvailable	894.000µs
matmul	tch-cpu	tch	Cpu	14.179ms
matmul	tch-gpu	tch	Cuda(0)	519.000µs
max_pool2d	wgpu	jit<wgpu>	BestAvailable	3.265ms
max_pool2d	wgpu-fusion	fusion<jit<wgpu>>	BestAvailable	2.981ms
max_pool2d	tch-cpu	tch	Cpu	552.786ms
max_pool2d	tch-gpu	tch	Cuda(0)	2.605ms

[codecov]

Codecov Report

Attention: Patch coverage is 11.53846% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 86.30%. Comparing base (edcd92f) to head (6a40f6d).
Report is 1 commits behind head on main.

Files	Patch %	Lines
backend-comparison/src/persistence/base.rs	11.53%	23 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1375      +/-   ##
==========================================
- Coverage   86.31%   86.30%   -0.02%     
==========================================
  Files         683      683              
  Lines       78091    78107      +16     
==========================================
+ Hits        67408    67412       +4     
- Misses      10683    10695      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[errordeveloper]

Great we came to a consensus, it is quite similar to my original change, albeit argument is passed to ‘save’ instead of ‘run_benchmark’.

[syl20bnr]

Indeed we went a bit full-circle on this one 😁
Thank you for the PR!