Added Benchmarking; Added Sampler to Prelude for Benchmarking #1071

Riley-Kilgore · 2024-12-11T14:06:39Z

No description provided.

Riley-Kilgore · 2024-12-16T18:39:24Z

After having these two types unified as Generator, I think it actually may make sense to have two separate prelude types Fuzzer and Sampler. The changes involved are minimal either way, but it seems to make the Fuzzer API somewhat uncomfortable if we are leveraging Generator due to the extra (and unused) parameter.

A lot of users won't be exposed to this, as they will simply use the pre-defined Fuzzers in the Fuzz library, but for users crafting their own Fuzzers, do we want to introduce breaking changes?

KtorZ · 2024-12-17T10:56:00Z

crates/aiken-lang/src/parser/token.rs

@@ -182,6 +183,7 @@ impl fmt::Display for Token {
            Token::Once => "once",
            Token::Validator => "validator",
            Token::Via => "via",
+            Token::Benchmark => "benchmark",


Let's call it bench; not so much because it's shorter but because it's quite often used in benchmarking terminology (bench refer to the individual tests of a benchmark, which aggregates all benches).

KtorZ · 2024-12-17T10:56:50Z

crates/aiken-lang/src/test_framework.rs

+    pub name: String,
+    pub on_test_failure: OnTestFailure,
+    pub program: Program<Name>,
+    pub fuzzer: Fuzzer<Name>,


That last one is suspicious? It ought not to be a Fuzzer here I believe.

KtorZ · 2024-12-17T10:58:06Z

crates/aiken-lang/src/test_framework.rs

 pub enum Prng {
-    Seeded { choices: Vec<u8>, uplc: PlutusData },
-    Replayed { choices: Vec<u8>, uplc: PlutusData },
+    Seeded {
+        choices: Vec<u8>,
+        uplc: PlutusData,
+        iteration: usize,
+    },
+    Replayed {
+        choices: Vec<u8>,
+        uplc: PlutusData,
+        iteration: usize,
+    },
 }


Why is changing the PRNG necessary? I don't think that it is right? The size parameter should be holistic for an entire PRNG and influence all random choices down the line.

KtorZ · 2024-12-17T11:00:56Z

A lot of users won't be exposed to this, as they will simply use the pre-defined Fuzzers in the Fuzz library, but for users crafting their own Fuzzers, do we want to introduce breaking changes?

That'll certainly require quite a lot of change in the Fuzz library, which I think we can avoid by making the context be provided as a closure instead of passed as an extra parameter. So, fundamentally, have:

Sampler<a> = fn(Int) -> Fuzzer<a>

instead of

Fuzzer<a> = fn(Void, PRNG) -> Option<(PRNG, a)>
Sampler<a> = fn(Int, PRNG) -> Option<(PRNG, a)>

KtorZ · 2025-01-10T15:39:49Z

crates/aiken-lang/src/parser/definition/benchmark.rs

This could probably be unified with the Fuzzer parser (by passing the expected keyword as a parameter to the parser?)

KtorZ · 2025-01-10T15:40:08Z

crates/aiken-lang/src/format.rs

@@ -633,6 +642,43 @@ impl<'comments> Formatter<'comments> {
            .append("}")
    }

+    #[allow(clippy::too_many_arguments)]
+    fn definition_benchmark<'a>(


I believe this could be unified with the formatting of fuzzer definitions as well.

KtorZ · 2025-01-10T15:40:28Z

crates/aiken-lang/src/test_framework.rs

+    pub name: String,
+    pub on_test_failure: OnTestFailure,
+    pub program: Program<Name>,
+    pub sampler: Fuzzer<Name>,


Should be Sampler<Name> ?

KtorZ · 2025-01-10T15:40:44Z

crates/aiken-lang/src/test_framework.rs

@@ -379,7 +382,7 @@ impl PropertyTest {
            let mut counterexample = Counterexample {
                value,
                choices: next_prng.choices(),
-                cache: Cache::new(|choices| {
+                cache: Cache::new(move |choices| {


Curious? Seems like an artifact from previous (reverted) changes.

KtorZ · 2025-01-10T15:41:16Z

crates/aiken-lang/src/test_framework.rs

+    pub fn benchmark(
+        self,
+        seed: u32,
+        n: usize,


Suggested change

n: usize,

max_iterations: usize,

Calling it n kind of suggests that it is what's going to be incremented.

KtorZ · 2025-01-10T15:49:27Z

crates/aiken-lang/src/tipo/infer.rs

+                                location: arg.arg.location,
+                                expected: inferred_inner_type.clone(),
+                                given: provided_inner_type.clone(),
+                                situation: Some(UnifyErrorSituation::FuzzerAnnotationMismatch),


Probably want to either:

Rework the annotation / situation here so that the error message look proper

Adjust the message to be more generic and fit both Fuzzer and Sampler

KtorZ · 2025-01-10T15:49:47Z

crates/aiken-lang/src/tipo/infer.rs

+                    let (inferred_annotation, inferred_inner_type) = match infer_sampler(
+                        environment,
+                        provided_inner_type.clone(),
+                        &typed_via.tipo(),
+                        &arg.via.location(),
+                    ) {
+                        Ok(result) => Ok(result),
+                        Err(err) => Err(err),
+                    }?;


The match seems excessive / useless here?

KtorZ · 2025-01-10T15:50:01Z

crates/aiken-lang/src/tipo/infer.rs

+                    let (inferred_annotation, inferred_inner_type) = match infer_fuzzer(
                        environment,
                        provided_inner_type.clone(),
                        &typed_via.tipo(),
                        &arg.via.location(),
-                    )?;
+                    ) {
+                        Ok(result) => Ok(result),
+                        Err(err) => Err(err),
+                    }?;


Unnecessary match?

KtorZ · 2025-01-10T15:54:28Z

crates/aiken-project/src/lib.rs

@@ -402,8 +427,7 @@ where
                seed,
                property_max_success,
            } => {
-                let tests =
-                    self.collect_tests(verbose, match_tests, exact_match, options.tracing)?;
+                let tests = self.collect_tests(false, match_tests, exact_match, options.tracing)?;


Suggested change

let tests = self.collect_tests(false, match_tests, exact_match, options.tracing)?;

let tests = self.collect_tests(verbose, match_tests, exact_match, options.tracing)?;

KtorZ · 2025-01-10T15:54:34Z

crates/aiken-project/src/lib.rs

+            } => {
+                // todo - collect benchmarks
+                let tests =
+                    self.collect_benchmarks(false, match_tests, exact_match, options.tracing)?;


Suggested change

self.collect_benchmarks(false, match_tests, exact_match, options.tracing)?;

self.collect_benchmarks(verbose, match_tests, exact_match, options.tracing)?;

KtorZ · 2025-01-10T16:01:12Z

crates/aiken-project/src/lib.rs

+                    // Write benchmark results to CSV
+                    use std::fs::File;
+                    use std::io::Write;
+
+                    let mut writer = File::create(&output).map_err(|error| {
+                        vec![Error::FileIo {
+                            error,
+                            path: output.clone(),
+                        }]
+                    })?;
+
+                    // Write CSV header
+                    writeln!(writer, "test_name,module,memory,cpu").map_err(|error| {
+                        vec![Error::FileIo {
+                            error,
+                            path: output.clone(),
+                        }]
+                    })?;


Avoid writing to file directly; instead, default to stdout and let user redirect to file if needed. We should stick to the same behavior as for tests: have a pretty input for ANSI-capable terminals, but default to JSON otherwise so that we can pipe the output into scripts and processing pipelines.

KtorZ · 2025-01-10T16:06:55Z

examples/acceptance_tests/117/lib/tests.ak

+fn simple_sampler(): Sampler<Int> {
+  fn(n: Int) {
+    fn(prng: PRNG) {
+      n
+    }
+  }
+}


This looks like it should NOT compile; if it does, then something's wrong with the typechecker (which I'll more thoroughly review eventually).

Riley-Kilgore requested a review from a team as a code owner December 11, 2024 14:06

Riley-Kilgore mentioned this pull request Dec 11, 2024

Move fuzzers here (DEPENDS ON AIKEN v1.1.8) aiken-lang/stdlib#103

Closed

Riley-Kilgore added 8 commits December 16, 2024 10:48

Dump (benchmarking wip)

c7d80f5

Dump (benchmarking wip)

8f0df45

Basic benchmarking functionality.

cace20a

Formatting

e4df9eb

Fixed basic benchmarking functionality

b081dc4

Added ScaledFuzzer capabilities

e59b109

Formatting

e3f8310

Added benchmark keyword and unified Samplers and Fuzzers as Generator

70daa4e

Riley-Kilgore force-pushed the benchmarking-wip branch from 3efd57a to 70daa4e Compare December 16, 2024 18:57

KtorZ reviewed Dec 17, 2024

View reviewed changes

Riley-Kilgore added 3 commits December 17, 2024 06:01

Old Fuzzer, new Sampler

896d0af

Formatting

7b80e32

Uh, formatting again..

b94150b

Riley-Kilgore changed the title ~~Add Benchmarking command with CSV output; Add ScaledFuzzer to prelude and PBT runner~~ Added Benchmarking; Added Sampler to Prelude for Benchmarking Dec 17, 2024

Added basic Sampler acceptance test

f8d4e8a

KtorZ reviewed Jan 10, 2025

View reviewed changes

Riley-Kilgore added 3 commits January 14, 2025 05:16

Addressed comments on benchmarking PR

c7657ce

Move acceptance test 117 to 118

231d0ce

Ran fmt

0bf42e9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Benchmarking; Added Sampler to Prelude for Benchmarking #1071

Added Benchmarking; Added Sampler to Prelude for Benchmarking #1071

Riley-Kilgore commented Dec 11, 2024

Riley-Kilgore commented Dec 16, 2024

KtorZ Dec 17, 2024

KtorZ Dec 17, 2024

KtorZ Dec 17, 2024

KtorZ commented Dec 17, 2024

KtorZ Jan 10, 2025

KtorZ Jan 10, 2025

KtorZ Jan 10, 2025

KtorZ Jan 10, 2025

KtorZ Jan 10, 2025

KtorZ Jan 10, 2025

KtorZ Jan 10, 2025

KtorZ Jan 10, 2025

KtorZ Jan 10, 2025

KtorZ Jan 10, 2025

KtorZ Jan 10, 2025

KtorZ Jan 10, 2025

	let tests = self.collect_tests(false, match_tests, exact_match, options.tracing)?;
	let tests = self.collect_tests(verbose, match_tests, exact_match, options.tracing)?;

	self.collect_benchmarks(false, match_tests, exact_match, options.tracing)?;
	self.collect_benchmarks(verbose, match_tests, exact_match, options.tracing)?;

Added Benchmarking; Added Sampler to Prelude for Benchmarking #1071

Are you sure you want to change the base?

Added Benchmarking; Added Sampler to Prelude for Benchmarking #1071

Conversation

Riley-Kilgore commented Dec 11, 2024

Riley-Kilgore commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment