Make backend names in JSON reports match burnbench CLI #1375

errordeveloper · 2024-02-27T16:27:42Z

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 · 2024-02-27T16:32:06Z

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 · 2024-02-27T16:39:35Z

backend-comparison/src/persistence/base.rs

+/// "min": "duration in microseconds",
+/// "max": "duration in microseconds",
+/// "median": "duration in microseconds",
+/// "mean": "duration in microseconds",
+/// "variance": "duration in microseconds"


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 · 2024-03-06T14:20:31Z

@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 · 2024-03-13T11:54:43Z

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

errordeveloper · 2024-03-13T11:57:22Z

crates/burn-jit/src/backend.rs

@@ -27,6 +27,14 @@ impl<R: Runtime> Backend for JitBackend<R> {
 format!("jit<{}>", R::name())
 }

+ fn config_name(_device: &Self::Device) -> Option<String> {
+ // match R::config_name(device) {


no so sure what to do here...

errordeveloper · 2024-03-13T12:10:42Z

crates/burn-ndarray/src/backend.rs

@@ -21,6 +21,18 @@ impl Default for NdArrayDevice {
 }
 }

+fn config_name() -> String {


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 · 2024-03-13T19:37:06Z

crates/burn-candle/src/backend.rs

+ fn config_name(device: &Self::Device) -> Option<String> {
+ match device {
+ CandleDevice::Cpu => Some(String::from("candle-cpu")),
+ CandleDevice::Cuda(_) | CandleDevice::Metal(_) => Some(String::from("candle-gpu")),


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

syl20bnr · 2024-03-19T12:09:18Z

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 · 2024-03-19T13:24:13Z

@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 · 2024-03-19T13:58:30Z

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 · 2024-03-20T11:18:45Z

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 · 2024-03-27T19:50:12Z

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 · 2024-03-28T15:57:49Z

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

syl20bnr · 2024-03-28T16:07:51Z

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

errordeveloper · 2024-03-28T18:07:19Z

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

antimora · 2024-03-28T18:48:30Z

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

- add `config_name` to `Backend` trait - add `backend_config_name` to `Benchmark` trait - fix documentation for JSON reports to use correct unit of time

This reverts commit a09edb6.

syl20bnr · 2024-03-30T03:41:46Z

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 · 2024-03-30T04:02:35Z

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 · 2024-03-30T09:32:15Z

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 · 2024-04-01T13:48:21Z

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

errordeveloper force-pushed the backend-name-in-reports branch from 86b297b to 4757e32 Compare February 27, 2024 16:33

errordeveloper commented Feb 27, 2024

View reviewed changes

nathanielsimard requested changes Mar 6, 2024

View reviewed changes

errordeveloper force-pushed the backend-name-in-reports branch 4 times, most recently from 5da0e08 to 55b5941 Compare March 13, 2024 11:51

errordeveloper changed the title ~~Make backend names appearing in JSON reports matching CLI~~ Make backend names in JSON reports match burnbench CLI Mar 13, 2024

errordeveloper force-pushed the backend-name-in-reports branch 4 times, most recently from d4943e3 to 899ad6e Compare March 13, 2024 12:05

errordeveloper requested a review from nathanielsimard March 13, 2024 12:05

errordeveloper force-pushed the backend-name-in-reports branch 2 times, most recently from 6c44c34 to cf2962a Compare March 13, 2024 12:11

errordeveloper commented Mar 13, 2024

View reviewed changes

errordeveloper marked this pull request as ready for review March 13, 2024 12:13

nathanielsimard reviewed Mar 13, 2024

View reviewed changes

errordeveloper force-pushed the backend-name-in-reports branch 3 times, most recently from 9349b24 to 36f4364 Compare March 19, 2024 11:15

errordeveloper force-pushed the backend-name-in-reports branch 2 times, most recently from fded72d to 0b96ec6 Compare March 20, 2024 12:40

antimora assigned syl20bnr Mar 28, 2024

syl20bnr added benchmark performance Anything related to performance labels Mar 28, 2024

errordeveloper and others added 2 commits March 29, 2024 18:37

Make backend names in JSON reports match burnbench CLI

a09edb6

- add `config_name` to `Backend` trait - add `backend_config_name` to `Benchmark` trait - fix documentation for JSON reports to use correct unit of time

Revert "Make backend names in JSON reports match burnbench CLI"

17158e9

This reverts commit a09edb6.

syl20bnr force-pushed the backend-name-in-reports branch from 0b96ec6 to a6d8c2d Compare March 30, 2024 03:20

syl20bnr requested a review from nathanielsimard March 30, 2024 03:20

syl20bnr force-pushed the backend-name-in-reports branch from a6d8c2d to 6a40f6d Compare March 30, 2024 03:43

[backend-comparison] Serialize the feature name passed to burnbench

6a40f6d

syl20bnr merged commit 67994c0 into tracel-ai:main Apr 1, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make backend names in JSON reports match burnbench CLI #1375

Make backend names in JSON reports match burnbench CLI #1375

errordeveloper commented Feb 27, 2024

errordeveloper commented Feb 27, 2024 •

edited

Loading

errordeveloper Feb 27, 2024

nathanielsimard left a comment

errordeveloper commented Mar 6, 2024

errordeveloper commented Mar 13, 2024

errordeveloper Mar 13, 2024

errordeveloper Mar 13, 2024 •

edited

Loading

nathanielsimard left a comment

nathanielsimard Mar 13, 2024

syl20bnr commented Mar 19, 2024

errordeveloper commented Mar 19, 2024

syl20bnr commented Mar 19, 2024

errordeveloper commented Mar 20, 2024

syl20bnr commented Mar 27, 2024 •

edited

Loading

antimora commented Mar 28, 2024

syl20bnr commented Mar 28, 2024

errordeveloper commented Mar 28, 2024 via email

antimora commented Mar 28, 2024

syl20bnr commented Mar 30, 2024 •

edited

Loading

codecov bot commented Mar 30, 2024

errordeveloper commented Mar 30, 2024 via email

syl20bnr commented Apr 1, 2024

Make backend names in JSON reports match burnbench CLI #1375

Make backend names in JSON reports match burnbench CLI #1375

Conversation

errordeveloper commented Feb 27, 2024

Pull Request Template

Checklist

Related Issues/PRs

Changes

Testing

errordeveloper commented Feb 27, 2024 • edited Loading

errordeveloper Feb 27, 2024

Choose a reason for hiding this comment

nathanielsimard left a comment

Choose a reason for hiding this comment

errordeveloper commented Mar 6, 2024

errordeveloper commented Mar 13, 2024

errordeveloper Mar 13, 2024

Choose a reason for hiding this comment

errordeveloper Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

nathanielsimard left a comment

Choose a reason for hiding this comment

nathanielsimard Mar 13, 2024

Choose a reason for hiding this comment

syl20bnr commented Mar 19, 2024

errordeveloper commented Mar 19, 2024

syl20bnr commented Mar 19, 2024

errordeveloper commented Mar 20, 2024

syl20bnr commented Mar 27, 2024 • edited Loading

antimora commented Mar 28, 2024

syl20bnr commented Mar 28, 2024

errordeveloper commented Mar 28, 2024 via email

antimora commented Mar 28, 2024

syl20bnr commented Mar 30, 2024 • edited Loading

codecov bot commented Mar 30, 2024

Codecov Report

errordeveloper commented Mar 30, 2024 via email

syl20bnr commented Apr 1, 2024

errordeveloper commented Feb 27, 2024 •

edited

Loading

errordeveloper Mar 13, 2024 •

edited

Loading

syl20bnr commented Mar 27, 2024 •

edited

Loading

syl20bnr commented Mar 30, 2024 •

edited

Loading