Adding a few Rust doc examples to Executor and ExecutorEnv #572
Conversation
Benchmark for Linux-cuda 955df88Click to hide benchmark
Benchmark for Linux-default 955df88Click to hide benchmark
Benchmark for macOS-default 955df88Click to hide benchmark
Benchmark for macOS-metal 955df88Click to hide benchmark
|
risc0/zkvm/src/exec/env.rs
Outdated
| @@ -151,6 +213,39 @@ impl<'a> ExecutorEnvBuilder<'a> { | |||
| } | |||
|
|
|||
| /// Add initial input that can be read by the guest from stdin. | |||
| /// This step must occur before the executor environment is built. | |||
There was a problem hiding this comment.
I don't understand this comment because structurally, it's not possible to add_input after the env has been built.
There was a problem hiding this comment.
Yeah this is trivially true, pulling it out.
risc0/zkvm/src/exec/env.rs
Outdated
| /// # ExecutorEnv, | ||
| /// # ExecutorEnvBuilder}; |
There was a problem hiding this comment.
Why are these commented out? Seems useful to have in the example.
There was a problem hiding this comment.
You're right -- going off of TimZ's feedback, I'm going to un-comment a majority of the omitted code, including includes.
risc0/zkvm/src/exec/env.rs
Outdated
| /// # use risc0_zkvm::{ | ||
| /// # ExecutorEnv, | ||
| /// # ExecutorEnvBuilder}; | ||
| /// # use std::collections::HashMap; |
There was a problem hiding this comment.
Oops! Copied and pasted from prev example without pulling that out.
risc0/zkvm/src/exec/env.rs
Outdated
| /// # use risc0_zkvm::{ | ||
| /// # ExecutorEnv, | ||
| /// # ExecutorEnvBuilder, | ||
| /// serde::to_vec}; |
| /// | ||
| /// let a: u64 = 400; | ||
| /// | ||
| /// let env = ExecutorEnv::builder().build(); |
There was a problem hiding this comment.
This example doesn't seem especially useful
There was a problem hiding this comment.
Maybe instead of an example, add a redirection to other parts of the doc, e.g. "Combine with [ExecutorEnvBuilder] functions like [...add_input] to set up the environment and I/O data your guest needs. Finalize with [ExecutorEnvBuilder::build] to get a configured [ExecutorEnv]."
risc0/zkvm/src/exec/env.rs
Outdated
| /// The executor environment holds configuration details that inform how the | ||
| /// guest environment is set up prior to guest program execution. This | ||
| /// includes initial guest program inputs, session size limits, segment size | ||
| /// limits, and environment variables. |
There was a problem hiding this comment.
Adding more context is good, but enumerating the fields in this description isn't good for maintenance and it doesn't actually help understand what this thing does any better (IMHO).
There was a problem hiding this comment.
That makes a lot of sense, outdated explanatory text is still an update cost
risc0/zkvm/src/exec/env.rs
Outdated
| /// Most of the time, the best practice is to combine guest program inputs | ||
| /// into a single struct. For cases where you want to add inputs that | ||
| /// the guest will access using consecutive reads, you can call | ||
| /// `ExecutorEnvBuilder::add_input()` iteratively: |
There was a problem hiding this comment.
Instead of making a prescription, I'd just say what the behavior of calling add_input multiple times is, which is to concatenate the inputs together
There was a problem hiding this comment.
Removing the prescriptive first line but making note that guest can access concatenated input with consecutive reads, can take it out if that's still too prescriptive.
| /// deciding where a zkVM program should be split into [Segment]s and what | ||
| /// work will be done in each segment. This is the execution phase: | ||
| /// the guest program is executed to determine how its proof should be | ||
| /// divided into subparts. | ||
| pub fn new(env: ExecutorEnv<'a>, image: MemoryImage, pc: u32) -> Self { |
There was a problem hiding this comment.
Now that the PC is part of the MemoryImage, we should drop the pc argument here
risc0/zkvm/src/exec/mod.rs
Outdated
| /// # use risc0_zkvm::{serde::to_vec, Executor, ExecutorEnv, Session}; | ||
| /// # use risc0_zkvm_methods::{BENCH_ELF, bench::{BenchmarkSpec, SpecWithIters}}; | ||
| /// # | ||
| /// # let spec = SpecWithIters(BenchmarkSpec::SimpleLoop, 1); | ||
| /// # let env = ExecutorEnv::builder() | ||
| /// # .add_input(&to_vec(&spec).unwrap()) | ||
| /// # .build(); |
There was a problem hiding this comment.
I think showing this is useful for this example
There was a problem hiding this comment.
I'm glad to see examples for these, thanks!
@flaub pointed out specific places where this would be helpful, but in general with code snippets I think showing context is one of the main benefits. More concretely, when documenting a function, the automatically generated signature documentation gives the reader a pretty good sense of what the exact line where you call the function looks like. What isn't generated that we need to manually write is what happens around that line: are there expectations or common techniques for other functions to be called ahead of time? Is there a standard or common way to construct one or more of the inputs? What sorts of things can you do with the result of the function? Is any cleanup needed? What do you need to import to be able to use the function in typical ways?
There's definitely a tradeoff here, for the general reasons that concision is good (readers won't bother to read, or read carefully, snippets that are too long). Nevertheless, in general I would hide less of the supporting code relative to what you're doing here. This is especially true when you only give one snippet: if you have three snippets and they each use the same set up code, you sometimes only need to show it on the first snippet, but if you only have one snippet -- well you still have to show it on the first snippet, but that's the only snippet.
risc0/zkvm/src/exec/env.rs
Outdated
| /// # use risc0_zkvm::{ | ||
| /// # ExecutorEnv, | ||
| /// # ExecutorEnvBuilder, | ||
| /// serde::to_vec}; |
There was a problem hiding this comment.
This use is dangling. Probably just un-hide the entire use block, but if you really want to show just to_vec, make it a separate statement, e.g.
# use risc0_zkvm::{
# ExecutorEnv,
# ExecutorEnvBuilder
# };
use risc0_zkvm::serde::to_vec;
risc0/zkvm/src/exec/mod.rs
Outdated
| /// # let env = ExecutorEnv::builder() | ||
| /// # .add_input(&to_vec(&spec).unwrap()) | ||
| /// # .build(); | ||
| /// # let mut exec = Executor::from_elf(env, BENCH_ELF).unwrap(); |
There was a problem hiding this comment.
I think showing the env & exec gives useful context. If the spec and BENCH_ELF names seem confusing (and they probably are), this could be further upgraded by hidden lines like let EXAMPLE_ELF = BENCH_ELF and naming spec to instead by example_input or something similar.
Co-authored-by: Frank Laub <flaub@risc0.com>
Co-authored-by: Frank Laub <flaub@risc0.com>
Co-authored-by: Frank Laub <flaub@risc0.com>
|
Thank you for writing this out, these are excellent guidelines. |
Benchmark for Linux-cudaBenchmark for Linux-default ae6ff3bClick to hide benchmark
Benchmark for macOS-default ae6ff3bClick to hide benchmark
Benchmark for macOS-metal ae6ff3bClick to hide benchmark
|
Benchmark for Linux-cuda af58725Click to hide benchmark
Benchmark for Linux-defaultBenchmark for macOS-default af58725Click to hide benchmark
Benchmark for macOS-metal |
Benchmark for Linux-cudaBenchmark for Linux-defaultBenchmark for macOS-default b93e304Click to hide benchmark
Benchmark for macOS-metal |
| /// | ||
| /// let a: u64 = 400; | ||
| /// | ||
| /// let env = ExecutorEnv::builder().build(); |
There was a problem hiding this comment.
Maybe instead of an example, add a redirection to other parts of the doc, e.g. "Combine with [ExecutorEnvBuilder] functions like [...add_input] to set up the environment and I/O data your guest needs. Finalize with [ExecutorEnvBuilder::build] to get a configured [ExecutorEnv]."
Co-authored-by: Tim Zerrell <tim.zerrell@risczero.com>
Benchmark for Linux-cudaBenchmark for Linux-defaultBenchmark for macOS-default 0c3eaa8Click to hide benchmark
Benchmark for macOS-metal 0c3eaa8Click to hide benchmark
|
* adding examples to ExecutorEnv and Executor * Update risc0/zkvm/src/exec/env.rs Co-authored-by: Frank Laub <flaub@risc0.com> * Update risc0/zkvm/src/exec/env.rs Co-authored-by: Frank Laub <flaub@risc0.com> * Update risc0/zkvm/src/exec/mod.rs Co-authored-by: Frank Laub <flaub@risc0.com> * updating executor/executor env doc examples after code review * Update risc0/zkvm/src/exec/env.rs Co-authored-by: Tim Zerrell <tim.zerrell@risczero.com> --------- Co-authored-by: Frank Laub <flaub@risc0.com> Co-authored-by: Tim Zerrell <tim.zerrell@risczero.com>
No description provided.