Bladeren bron

bench: --label optional (default 'perf' for perf-only), depth suite now opt-in (--depth), parse perf-box caveat asterisks, quiet json warning for perf-only

Alexander Korolev 1 week geleden
bovenliggende
commit
425da04ac3
6 gewijzigde bestanden met toevoegingen van 98 en 56 verwijderingen
  1. 44 40
      src/bench.rs
  2. 13 8
      src/bench_remote.rs
  3. 7 5
      src/cli.rs
  4. 2 2
      src/main.rs
  5. 7 1
      src/report/parse.rs
  6. 25 0
      src/report/perf_box_tests.rs

+ 44 - 40
src/bench.rs

@@ -1,12 +1,8 @@
 //! `sm121 bench` — run `tool-eval-bench` against a live serve and land the
-//! reports in the repo.
-//!
-//! The bench tool needs babysitting this command does instead of the
-//! operator: it must run in a scratch cwd (it writes
-//! `runs/<date>/<run>.md` relative to cwd), needs an absolute `--json-file`
-//! path, and the `.md` report must be copied into
-//! `recipes/<lane>/reports/<UTC-date>_<label>.{md,json,log}` by hand.
-//! Campaign convention (seed 42, hardmode, perf, concurrency 1,2) is the
+//! reports in `recipes/<lane>/reports/`. The tool needs a scratch cwd (it
+//! writes `runs/<date>/<run>.md` relative to cwd), an absolute `--json-file`
+//! path, and the `.md` copied into the lane — this command does the
+//! babysitting. Campaign convention (seed 42, hardmode, perf, c1,2) is the
 //! default; `sm121 bench -d` prints the exact command.
 
 use std::fs;
@@ -19,23 +15,21 @@ use anyhow::Result;
 use crate::config::{Config, Recipe, load_recipe, select_cluster_by_name};
 
 /// Fields of the `sm121 bench` subcommand (mirrored by the clap variant in
-/// `cli.rs`). Served model name and port come from the recipe
-/// (`--served-model-name` in args / `port`); the bench binary is looked up on PATH.
+/// `cli.rs`). Served name/port come from the recipe; the tool is on PATH.
 #[derive(Clone)]
 pub struct Args {
     pub recipe: String,
     pub cluster: String,
-    pub label: String,
+    pub label: Option<String>,
     pub concurrency: String,
     pub seed: u64,
-    /// Scenario streams run concurrently (tool `--parallel`). None = the
-    /// tool default (serial), preserving earlier lanes' comparability.
+    /// Scenario streams run concurrently (tool `--parallel`; None = serial).
     pub parallel: Option<u32>,
     /// Skip the scenario phase (tool `--perf-only`).
     pub perf_only: bool,
-    /// Skip the context-depth suite (16K..256K) — run only the main pass
-    /// (scenarios + the typical-depth perf rows).
-    pub no_depth: bool,
+    /// Run the context-depth suite (16K..256K) after the main pass — off by
+    /// default (a full sweep takes ~45 min on a 2x lane).
+    pub depth: bool,
     /// Local tokenizer source for llama-benchy (tool `--tokenizer`): a
     /// tokenizer.json or a dir holding one. Needed when the served model
     /// id is not resolvable in the local HF cache (custom export lanes).
@@ -168,13 +162,12 @@ pub async fn run(config: &Config, a: &Args, dry_run: bool) -> Result<()> {
         .ok_or_else(|| anyhow::anyhow!("recipe file not found for '{}'", a.recipe))?
         .canonicalize()?;
     let cluster = select_cluster_by_name(config, &a.cluster)?;
-    let head = match cluster.nodes.iter().find(|n| n.rank == 0) {
-        Some(n) => n,
-        None => cluster
-            .nodes
-            .first()
-            .ok_or_else(|| anyhow::anyhow!("cluster '{}' has no nodes", cluster.name))?,
-    };
+    let head = cluster
+        .nodes
+        .iter()
+        .find(|n| n.rank == 0)
+        .or_else(|| cluster.nodes.first())
+        .ok_or_else(|| anyhow::anyhow!("cluster '{}' has no nodes", cluster.name))?;
     let port = recipe.port;
     let model = served_model(&recipe).ok_or_else(|| {
         anyhow::anyhow!(
@@ -184,7 +177,14 @@ pub async fn run(config: &Config, a: &Args, dry_run: bool) -> Result<()> {
     })?;
     let base_url = format!("http://{}:{}/v1", head.ssh_ip, port);
     let stamp = crate::stamp::utc_stamp();
-    let label = sanitize(&a.label);
+    // Label: required for quality runs, optional for perf-only (default `perf`).
+    let label = match (&a.label, a.perf_only) {
+        (Some(l), _) => sanitize(l),
+        (None, true) => String::from("perf"),
+        (None, false) => {
+            anyhow::bail!("--label is required for a quality run (the campaign record)")
+        }
+    };
     let stem = format!("{stamp}_{label}");
     let reports_dir = file
         .parent()
@@ -197,10 +197,8 @@ pub async fn run(config: &Config, a: &Args, dry_run: bool) -> Result<()> {
     // downloaded here after the run so the local report generation is unchanged.
     let scratch = dirs::home_dir()
         .ok_or_else(|| anyhow::anyhow!("no home dir"))?
-        .join(".cache")
-        .join("sm121")
-        .join("bench")
-        .join(format!("{stamp}_{label}"));
+        .join(".cache/sm121/bench")
+        .join(&stem);
     // The scratch dir the tool writes to (local by default, or the remote host
     // when `--ssh-host` is set — the bench client's network then stays off the
     // measurement path).
@@ -241,11 +239,9 @@ pub async fn run(config: &Config, a: &Args, dry_run: bool) -> Result<()> {
         a.seed.to_string(),
         "--hardmode".into(),
         "--perf".into(),
-        // Depth rows must decode a WARM prefix: llama-benchy's default
-        // branch concatenates context+prompt into one 67K request whose
-        // serialized dual prefills measured 13.7/5.6/2.1 t/s at c2 — an
-        // admission artifact, not the engine (diagnosis 2026-09-29,
-        // wire capture + this flag flipping c2 13.7 → 52.1 in one run).
+        // Warm-prefix decode: without it llama-benchy's default branch
+        // serializes dual 67K prefills (c2 collapses to 13.7 t/s — an
+        // admission artifact, not the engine; 2026-09-29 diagnosis).
         "--benchy-args=--enable-prefix-caching".into(),
         "--concurrency".into(),
         a.concurrency.clone(),
@@ -268,10 +264,11 @@ pub async fn run(config: &Config, a: &Args, dry_run: bool) -> Result<()> {
         println!("bench: cwd {}", tool_scratch.display());
         println!("bench: {} {}", bin, args.join(" "));
         println!("bench: depth suite: {} {}", bin, depth_args.join(" "));
-        println!("bench: reports -> {}", reports_dir.display());
         println!(
-            "bench: run package -> {}.recipe.toml + {}.manifest.json",
-            stem, stem
+            "bench: reports -> {} (run package: {}.recipe.toml + {}.manifest.json)",
+            reports_dir.display(),
+            stem,
+            stem
         );
         return Ok(());
     }
@@ -298,8 +295,8 @@ pub async fn run(config: &Config, a: &Args, dry_run: bool) -> Result<()> {
     // Phase 2 — context-depth suite (16K..256K @ c2, 2 repeats): appended
     // to the same log so `parse_perf_rows` picks its rows up too. A depth
     // failure must not cost the finished quality run — warn, keep going.
-    // `--no-depth` skips the whole phase (quick-grasp campaigns).
-    if !a.no_depth {
+    // Opt-in (`--depth`): a full sweep takes ~45 min on a 2x lane.
+    if a.depth {
         eprintln!(
             "bench: depth suite: {} @ c{}, {} repeats{}",
             crate::bench_depth::DEPTHS,
@@ -320,13 +317,20 @@ pub async fn run(config: &Config, a: &Args, dry_run: bool) -> Result<()> {
             );
         }
     } else {
-        eprintln!("bench: depth suite skipped (--no-depth)");
+        eprintln!("bench: depth suite skipped (opt-in: --depth)");
     }
 
     // Remote host: download the json + the md from the remote scratch dir so
     // the local report generation is unchanged.
     if let Some(host) = ssh_host {
-        crate::bench_remote::collect(host, &tool_json, &json_path, &tool_scratch, &scratch)?;
+        crate::bench_remote::collect(
+            host,
+            &tool_json,
+            &json_path,
+            &tool_scratch,
+            &scratch,
+            !a.perf_only,
+        )?;
     }
 
     let md_src = crate::bench_io::latest_md(&scratch)

+ 13 - 8
src/bench_remote.rs

@@ -59,21 +59,26 @@ pub fn prepare(host: &str, tool_scratch: &Path, tokenizer: &Option<String>) -> R
 
 /// Download the json + the md from the remote scratch dir so the local report
 /// generation is unchanged. A download failure warns (the run is not lost);
-/// the local json is simply missing (the run package is skipped).
+/// the local json is simply missing (the run package is skipped). Perf-only
+/// runs write no JSON by design — pass `expect_json = false` to skip the
+/// scp (and the warning) entirely.
 pub fn collect(
     host: &str,
     tool_json: &Path,
     json_path: &Path,
     tool_scratch: &Path,
     scratch: &Path,
+    expect_json: bool,
 ) -> Result<()> {
-    let st = Command::new("scp")
-        .arg(format!("{host}:{}", tool_json.display()))
-        .arg(json_path)
-        .output()
-        .map_err(|e| anyhow::anyhow!("scp json from {host}: {e}"))?;
-    if !st.status.success() {
-        eprintln!("bench: WARNING could not download json from {host}");
+    if expect_json {
+        let st = Command::new("scp")
+            .arg(format!("{host}:{}", tool_json.display()))
+            .arg(json_path)
+            .output()
+            .map_err(|e| anyhow::anyhow!("scp json from {host}: {e}"))?;
+        if !st.status.success() {
+            eprintln!("bench: WARNING could not download json from {host}");
+        }
     }
     let st = Command::new("ssh")
         .arg(host)

+ 7 - 5
src/cli.rs

@@ -176,8 +176,10 @@ pub enum Commands {
         recipe: String,
         #[arg(long, required = true)]
         cluster: String,
-        #[arg(long, required = true)]
-        label: String,
+        /// Label for the run (campaign record). Optional for `--perf-only`
+        /// runs (default `perf`); required for quality runs.
+        #[arg(long)]
+        label: Option<String>,
         #[arg(long, default_value = "1,2")]
         concurrency: String,
         #[arg(long, default_value_t = 42)]
@@ -195,10 +197,10 @@ pub enum Commands {
         /// (use to re-baseline the perf table after a cold first row).
         #[arg(long)]
         perf_only: bool,
-        /// Skip the context-depth suite (16K..256K) — run only the main pass
-        /// (scenarios + typical-depth perf rows).
+        /// Run the context-depth suite (16K..256K) after the main pass — off
+        /// by default (a full sweep takes ~45 min on a 2x lane).
         #[arg(long)]
-        no_depth: bool,
+        depth: bool,
         /// Run tool-eval-bench on a remote host over SSH instead of locally,
         /// so the bench client's own network (a WiFi laptop) stays off the
         /// measurement path. The tokenizer is uploaded to the host; the

+ 2 - 2
src/main.rs

@@ -229,7 +229,7 @@ async fn main() -> Result<()> {
             seed,
             parallel,
             perf_only,
-            no_depth,
+            depth,
             tokenizer,
             ssh_host,
         } => {
@@ -241,7 +241,7 @@ async fn main() -> Result<()> {
                 seed,
                 parallel,
                 perf_only,
-                no_depth,
+                depth,
                 tokenizer,
                 ssh_host,
             };

+ 7 - 1
src/report/parse.rs

@@ -115,8 +115,14 @@ pub(crate) fn parse_perf_box(content: &str) -> Vec<PerfCell> {
             .find_map(|tok| tok.strip_prefix("c").and_then(|n| n.parse::<u32>().ok()))
             .unwrap_or(1);
         // numbers from the label onward: pp t/s, tg t/s, TTFT, total, 2048, 128
+        // (llama-benchy marks caveat/cold cells with a trailing `*`, e.g. `675*`)
         let nums: Vec<f64> = split_tokens(&line[at..])
-            .filter_map(|tok| tok.replace(',', "").parse::<f64>().ok())
+            .filter_map(|tok| {
+                tok.trim_end_matches('*')
+                    .replace(',', "")
+                    .parse::<f64>()
+                    .ok()
+            })
             .collect();
         if nums.len() >= 4 && nums[1] > 0.0 {
             out.push(PerfCell { d, c, tg: nums[1] });

+ 25 - 0
src/report/perf_box_tests.rs

@@ -45,6 +45,31 @@ fn parse_perf_box_reads_both_table_shapes() {
     assert!(super::parse::parse_perf_box("no perf here").is_empty());
 }
 
+#[test]
+fn parse_perf_box_strips_caveat_asterisk() {
+    // llama-benchy marks caveat/cold cells with a trailing `*` on the pp
+    // column (`675*`). Without the strip the token fails to parse and the
+    // column indices shift — ttft would land in the tg slot.
+    let md = "| pp2048 tg128 @ d0 | 675* | 39.6 | 3,034 | 6,242 | 2048+128 |\n\
+| pp2048 tg128 @ d0 c2 | 1,369 | 68.5 | 2,843 | 6,323 | 2048+128 |\n";
+    let cells = super::parse::parse_perf_box(md);
+    assert_eq!(
+        cells,
+        vec![
+            super::PerfCell {
+                d: 0,
+                c: 1,
+                tg: 39.6
+            },
+            super::PerfCell {
+                d: 0,
+                c: 2,
+                tg: 68.5
+            },
+        ]
+    );
+}
+
 #[test]
 fn parse_perf_box_ignores_progress_spinner_soup() {
     // llama-benchy redraws its progress bar on one \r-overwritten line that