feat(cli): add --json output for benchmark - #158
Open
yashksaini-coder wants to merge 4 commits into
Open
Conversation
…ctive#69) Nanotune's output is Ink-rendered boxes and colour — fine for humans, unparseable for scripts. Reading a pass rate out of `nanotune status` meant either parsing a box-drawn table or reaching into `.nanotune/benchmarks/*.json` behind the CLI's back. `--json` on `status` and `data validate` prints a single JSON document to stdout instead. Ink never mounts in this mode: `render()` writes to stdout the moment it does, and a box-drawn frame in the middle of a JSON document is not something a consumer can recover from. The CLI calls a plain collector and hands the result to `emitJson`. The contract, documented in docs/guides/json-output.md: - stdout carries the payload or nothing — never a diagnostic, never a partial document - diagnostics (including config unknown-key warnings) go to stderr - the exit code carries the status, unchanged from the Ink path, so `data validate --json` still exits 1 on invalid data — and still prints its report, because the report is the useful part - absent values are null, never omitted - timestamps are ISO 8601, sizes are raw bytes; "2 hours ago" and "1.2 GB" stay in the view layer Both commands' data gathering moves into lib/ as `collectStatus` and `collectValidation`, shared with the Ink views rather than duplicated, so the two can never report different facts. `status.tsx` and `validate.tsx` become pure presentation. Two fixes fall out of that, both required for a schema that can be called stable: - `validateTrainingData` already computed the duplicate and inconsistent-context counts, then threw the numbers away, stringified them into English warnings, and had the view parse the English back out with `warnings.some(w => w.includes('duplicate'))`. It now returns the counts, and both views read those — rewording a warning can no longer silently flip a check. - the "minimum example count" check is no longer applied to a validation set, matching `validateTrainingData`, which already skips the 50-example warning there, and the documented behaviour in docs/commands/data.md. `benchmark --json` is deliberately not included: its run lives inside a ~550-line useCallback in the command component and wants extracting into a lib/ async generator first. Tracked as follow-up on Nano-Collective#69. 21 new tests. `buildJsonOutcome` is split from `emitJson` so the contract is testable without capturing real stdout.
Completes the `--json` work started for `status` and `data validate`.
`nanotune benchmark --json` prints the finished run as a single JSON
document on stdout — exactly the document already written to
`.nanotune/benchmarks/benchmark-<timestamp>.json`, so there is one
schema rather than two.
Ink cannot mount in JSON mode, and the entire benchmark run lived inside
a ~550-line useCallback in the command component, so the run moves to
`src/lib/benchmark-run.ts` as `runBenchmark` — an async generator
yielding progress with the finished BenchmarkResult on its final `done`
event, the same shape `ensureModelDownloaded` uses for its resolved
path. `benchmark.tsx` drops from 1038 lines to 249 and now only
translates events into view state.
The move is mechanical, with one deliberate change: the ~10
`setError(...); setStatus('error'); return` paths become `throw`. An
error path that returns normally is a smell, and a throw is what lets
one function serve both consumers — the component's existing catch turns
it into the error frame, and `emitJson` turns it into a stderr line and
a non-zero exit.
Three pure functions come out of the extraction and get the unit tests
they never had inside the component:
- `resolveRunOptions` — preset vs. individual flags (~55 lines of
branching). Now also runs before model resolution, so a bad
`--preset` fails immediately instead of after a multi-minute base
model download.
- `summarizeResults` — the latency/tok-s/TTFT/judge averaging and the
partial-run arithmetic (~80 lines).
- `validateTests` — every test needs `prompt` or `messages`.
`BenchmarkResult` gains an optional `warning`, recording why a run
stopped early. `summary` already scores against the tests that actually
ran, so a saved partial run was previously indistinguishable from a
complete one with a worse pass rate — a consumer would read it as a real
score. The terminal already showed this; now the result carries it.
`--base` needed one structural change: its two concurrent jobs run in
nested async functions, which cannot `yield`. They report into a shared
`latest` string that a drain loop turns into events. They already shared
a single progress line before this was a generator, so no information is
lost, and the settle handler is attached at creation because an
unhandled rejection is fatal on Node 22.
Progress goes to stderr during a `--json` run, since a suite can take
minutes and stdout must stay a single document. Exit codes are
unchanged: a completed run exits 0 whatever its pass rate, and a run
where the server died before any test completed still exits 1.
`emitJson`/`buildJsonOutcome` become async to accept the run.
35 new tests, 522 passing.
Note: this branch is stacked on Nano-Collective#157 and should merge after it.
CI's coverage gate failed on this branch: 69.98% against a 71% floor and a 71.65% baseline on main. Root cause is not a real regression. c8 runs without `all: true`, so it only measures files a test actually imports. No spec imports `BenchmarkCommand`, so `src/commands/benchmark.tsx` never appeared in the report at all and its ~800 uncovered lines were invisible to the metric. Moving that code into `benchmark-run.ts` — which its spec does import — made the same uncovered lines visible for the first time. `generateMarkdownReport` is the largest genuinely testable piece of it: ~170 lines of branchy template producing a user-facing artefact, never exercised in the repo's history. Exported and covered across its branches — base vs fine-tuned runs, the partial-run notice, optional config and summary fields, multi-turn conversations, sampling stats, judge scores and criteria, and the failures table including its pipe escaping and newline flattening. Coverage 69.98% -> 72.19%, above the 71.65% baseline rather than below it. benchmark-run.ts 45.45% -> 64.81%. 537 tests passing.
`runBenchmark` takes an optional `deps` argument defaulting to the real startLlamaServer/chatCompletion/stopLlamaServer/callJudge, so the scoring loop can run against a fake. Production always takes the default. That loop decides every pass and fail the product reports — sampling iteration and per-sample seeds, timeout handling, judge-vs-string-match dispatch, category tallying, failure recording, partial-run abort — and had no tests at all, because it needed a live llama-server and a real model. 14 tests now cover it: matching and non-matching responses, timings carried through, a timed-out call becoming a failed test rather than a crash, seeds varying per sample with the pass rate and variance recorded, llm-judge scoring, the server dying mid-run leaving a partial result with its `warning` set, the JSON and Markdown reports both being written and matching what `--json` prints, and the event sequence. Also guards `passRate` against an empty dataset: 0/0 is NaN, which JSON.stringify writes as `null`, and a field documented as a number must not reach a consumer as null. Coverage 72.19% -> 74.71%, above both the current 71.65% baseline and the 73.04% this branch will face once Nano-Collective#157 merges. benchmark-run.ts 45.45% -> 86.53%. 552 tests passing.
Contributor
Author
|
@will-lamerton @akramcodez — ready for review, though #157 should go first since this is stacked on it. Adds One ask before merging: I'm on Linux, so I couldn't run a real benchmark — everything past |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the remaining piece of #69:
--jsonfornanotune benchmark.What
Prints exactly the document already written to
.nanotune/benchmarks/benchmark-<timestamp>.json— one schema, not two. The issue notes people are already reading that file directly; this just makes it a first-class output. Progress goes to stderr while the suite runs, since a benchmark takes minutes and stdout has to stay a single parseable document.Why this needed a refactor
Ink can't mount in JSON mode, and the entire benchmark run lived inside a ~550-line
useCallbackin the command component. There was no seam — so the run moves tosrc/lib/benchmark-run.tsasrunBenchmark, an async generator yielding progress with the finishedBenchmarkResulton its finaldoneevent. That's the same shapeensureModelDownloadedalready uses to return its resolved path.benchmark.tsx: 1038 → 249 lines. It now only translates events into view state.The move is mechanical. One deliberate change: the ~10
setError(…); setStatus('error'); returnpaths becomethrow. An error path that returns normally is a smell, and a throw is what lets one function serve both consumers — the component's existingcatchturns it into the error frame,emitJsonturns it into a stderr line and exit 1.Tests the run never had
Three pure functions fall out of the extraction, all previously buried in the component with zero coverage:
resolveRunOptionssummarizeResultsvalidateTestsPlus
formatEventForStderr(3) and 11 tests drivingrunBenchmarkthrough every guard that fires beforestartLlamaServer.One behaviour improvement came free:
resolveRunOptionsnow runs before model resolution, so a mistyped--presetfails immediately instead of after a multi-minute--basedownload.BenchmarkResult.warningNew optional field recording why a run stopped early. When llama-server dies mid-suite the run saves what it has — and
summarycorrectly scores against the tests that actually ran, which means a saved partial run was previously indistinguishable from a complete one with a worse pass rate. The terminal already showed this warning; now the result carries it, so a--jsonconsumer can't mistake a partial score for a real one:Optional and additive, so existing files and
benchmark compareare unaffected.One structural wrinkle worth flagging
--baseruns two jobs concurrently (installing llama.cpp, downloading the base model) inside nested async functions — which cannotyield. They now report into a sharedlateststring that a drain loop turns into events. They already shared a single progress line before this was a generator, so no information is lost. The settle handler is attached at promise creation rather than later, becauseworkcan reject while the drain loop is sleeping and an unhandled rejection is fatal on Node 22 — the same reasoning ascreateServerHandle.Exit codes unchanged
A completed run exits
0whatever its pass rate — there's no configured threshold and inventing one would be surprising. A run where the server died before any test completed still exits1with nothing on stdout.Testing
522 passing (+35).
test:lint,test:format,test:types,test:knip,test:audit,test:avaall green locally.Verified against the built binary:
--model+--base→ mutual-exclusion error--preset→ rejected before model resolutiontests.jsonwritten, exit 1llm-judgetest with no judge configured → correct hintnanotune benchmarkstill renders its error frame, exit 1assertSupportedPlatformgates--baseand there's no llama.cpp binary here, so every path fromstartLlamaServeronward is verified by inspection and by keeping the move mechanical, not by execution. Please run one real suite on Apple Silicon before merging — ideally one with--samples > 1and one that exercises--base. If it'd help, I'm happy to split the extraction out as a pure no-op refactor commit to make that diff easier to eyeball.Update: coverage gate, and what it turned up
The first push failed
Unit Tests & Coverage Analysis— 69.98% against a 71% floor and a 71.65% baseline. Worth writing up, because it wasn't a real regression.c8runs withoutall: true, so it only measures files a test actually imports. No spec importsBenchmarkCommand, sosrc/commands/benchmark.tsxnever appeared in the coverage report at all — its ~800 uncovered lines were invisible to the metric. Moving that code intobenchmark-run.ts, which its spec does import, made the same uncovered lines visible for the first time. The number went down because the measurement got more honest.Rather than paper over it, I covered what was genuinely coverable:
generateMarkdownReport— ~170 lines of branchy template producing a user-facing artefact, never exercised in this repo's history. Now covered across base-vs-fine-tuned runs, the partial-run notice, optional config and summary fields, multi-turn conversations, sampling stats, judge scores and criteria, and the failures table including its pipe-escaping and newline flattening.The scoring loop, via an optional
depsargument onrunBenchmarkdefaulting to the realstartLlamaServer/chatCompletion/stopLlamaServer/callJudge. Production always takes the default; tests pass a fake. That loop decides every pass and fail the product reports — sampling and per-sample seeds, timeout handling, judge-vs-match dispatch, category tallying, failure recording, the partial-run abort — and had no tests at all, because exercising it needed a live server and a real model.I'm aware "add a parameter for tests" deserves scrutiny. My argument: making the run callable outside React is the same property as making it testable, and 186 lines of untested scoring logic is the highest-value target in the file. Happy to drop it if you'd rather.
The branch now raises repo coverage rather than lowering it — including against the 73.04% baseline it will face once #157 merges, which I measured on that branch to be sure this doesn't just fail again after the stack lands.
Two things the new tests turned up
Fixed here: an empty
tests.jsonmadepassRatecompute0/0, andJSON.stringifywritesNaNasnull— a field documented as anumberreaching consumers as null. One-line guard plus a test.Not fixed, flagging only: the
abortReason && allResults.length === 0branch looks unreachable.void serverHandle.exited.then(...)is registered after theawait startLlamaServer, and the loop begins synchronously, soserverDiedis always false on the first iteration — a server that dies instantly still runs test 1 (which fails to connect) and saves a 1-test partial run instead. Pre-existing and unchanged by this PR, so I left it alone; say the word if you'd like it handled.Suggestion for a separate issue
Consider running
c8withall: trueso untested files count as 0% instead of vanishing from the report. It would drop the headline number repo-wide and need the threshold retuned, so it's your call — but right now the figure means "of the code we already test, how much do we test", and moving code between files can swing it with zero behavioural change. Happy to open an issue if useful.