bench_tools: run and compare benchmark

[AvivYossef-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[AvivYossef-starkware]

• bench_tools: add absolute limits to benchmark runs #9861 : 3 dependent PRs (#9526 , #9874 , #9876 )
• bench_tools: dont capture output when running a benchmark #9860
• ci: use benchtools to benchmark the committer #9707
• bench_tools: fix committer benchmark config #9807
• bench_tools: run and compare benchmark #9699 👈 (View in Graphite)
• bench_tools: support deserialization of criterion change #9698
• bench_tools: save specific benchmark result #9742
• bench_tools: add criterion benchmark names to benchmark config #9697
• bench_tools: add bench tools to allowed scopes #9696
• ci: run benchmark command #9636
• infra: copy dir rec #9802
• main-v0.14.1

This stack of pull requests is managed by Graphite. Learn more about stacking.

[avi-starkware]

@avi-starkware reviewed 3 of 5 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @meship-starkware)

[avi-starkware]

@avi-starkware reviewed 1 of 2 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @meship-starkware)

---

crates/bench_tools/src/comparison_test.rs line 5 at r2 (raw file):

#[test]
fn test_get_regression_percentage() {

I don't think this test is needed
(it's just a function that multiplies by 100)
Better to test check_regressions

Code quote:

fn test_get_regression_percentage() {

---

crates/bench_tools/src/comparison.rs line 51 at r2 (raw file):

    bench_names: &[&str],
    regression_limit: f64,
) -> Result<Vec<BenchmarkComparison>, (String, Vec<BenchmarkComparison>)> {

Consider defining a RegressionError type

Suggestion:

) -> Result<(), RegressionError> {

---

crates/bench_tools/src/runner.rs line 118 at r2 (raw file):

/// Runs benchmarks and compares them against previous results, failing if regression exceeds limit.
pub async fn run_and_compare_benchmarks(

Suggestion:

pub fn run_and_compare_benchmarks(

---

crates/bench_tools/src/main.rs line 91 at r2 (raw file):

                regression_limit,
            )
            .await;

Suggestion:

            );

[AvivYossef-starkware]

Reviewable status: 2 of 5 files reviewed, 4 unresolved discussions (waiting on @avi-starkware and @meship-starkware)

---

crates/bench_tools/src/comparison_test.rs line 5 at r2 (raw file):

Previously, avi-starkware (Avi Cohen) wrote…

I don't think this test is needed
(it's just a function that multiplies by 100)
Better to test check_regressions

Done.

---

crates/bench_tools/src/runner.rs line 158 at r2 (raw file):

Previously, avi-starkware (Avi Cohen) wrote…

Please remove emojis

Why? look here
it looks nice

---

crates/bench_tools/src/main.rs line 91 at r2 (raw file):

                regression_limit,
            )
            .await;

Done.

---

crates/bench_tools/src/runner.rs line 118 at r2 (raw file):

/// Runs benchmarks and compares them against previous results, failing if regression exceeds limit.
pub async fn run_and_compare_benchmarks(

Done.

[avi-starkware]

@avi-starkware reviewed 1 of 3 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)

---

crates/bench_tools/src/runner.rs line 158 at r2 (raw file):

Previously, AvivYossef-starkware wrote…

Why? look here
it looks nice

Nevermind
I retracted this comment

[graphite-app]

Merge activity

• Oct 30, 9:16 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.