Bencher Test Suite #242
Bencher Test Suite #242
Conversation
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
Great; thanks for getting this started! I have a few granular coding suggestions before we merge.
Also, can we please put these new scripts into a directory? We could reuse the bench/ top-level directory that I made for my old infrastructure or pick a new, different name.
|
|
||
|
|
There was a problem hiding this comment.
Let's revert these changes, because they are kinda "diff noise."
| } | ||
| } | ||
|
|
||
| print(json.dumps(bencher_json)) |
There was a problem hiding this comment.
This can be more succinctly written as:
json.dump(bencher_json, sys.stdout)
| size_bytes_1 = float(benchmark("tests/chr6.C4.gfa")) / 1000.0 | ||
| size_bytes_2 = float(benchmark("tests/DRB1-3123.gfa")) / 1000.0 | ||
| size_bytes_3 = float(benchmark("tests/LPA.gfa")) / 1000.0 |
There was a problem hiding this comment.
It seems like this will be a little easier to maintain if we make this more "data-driven," i.e., use a list for the filename and loop over it? Like this:
gfa_files = ["tests/chr6.C4.gfa", "tests/DRB1-3123.gfa", "tests/LPA.gfa"]
sizes = {name: float(benchmark(name)) / 1000.0 for name in gfa_files}
Then use that size dictionary in the Bencher results here.
| @@ -0,0 +1,44 @@ | |||
| on: | |||
| push: | |||
| branches: john-benchmark | |||
There was a problem hiding this comment.
Let's do this for all branches, so we can see this working on main when it's merged.
| import json | ||
| import subprocess | ||
|
|
||
| subprocess.run(["cargo", "build", "--release"], check = True) |
There was a problem hiding this comment.
Maybe this is silly, but do we really want to do the cargo build here? It seems somewhat odd that the benchmarks script would do the rebuild itself rather than just using whatever had already been built. Furthermore, the fact that the responsibilities for "installing" fgfa are spread across two places (here and the GitHub Actions stuff, which amends $PATH) is a tad confusing.
I gently suggest that we remove this and instead add a step to the Actions script to do the build.
Adds a test suite for benchmarking performance and file size via Bencher.