fix(benchmark): create the benchmarks directory before writing into it - #117
Merged
will-lamerton merged 4 commits intoAug 30, 2026
Merged
Conversation
`nanotune benchmark` read and wrote `.nanotune/benchmarks/` but nothing ever created it. `initializeProjectDirs` is the only thing that does, and it runs only in `nanotune init` — so since `.nanotune/.gitignore` lists `benchmarks/`, anyone who obtains a project by cloning rather than initialising it has no such directory, and every write under it died with a bare ENOENT. That broke two separate things: - The first run. The "no dataset yet, create a sample" path was unreachable, because the write that creates the sample was itself the call that threw. - A completed run's results. With `--dataset` pointing outside the project the early write is skipped, so the whole suite ran to completion and *then* both result writes threw. The JSON and Markdown reports were lost and the command reported a generic "Benchmark failed" — potentially hours of GPU time on a large suite. `ensureBenchmarksDir` creates the directory and returns it, and benchmark resolves its path through that rather than `getBenchmarksDir`, so the destination exists before a single test runs. `benchmark compare` already got this right with its own inline `existsSync`/`mkdirSync`; it now shares the helper instead of keeping a second copy. The three writes go through `writeFileAtomic`, which renames a sibling temp into place. rename(2) is atomic, so an interrupted write leaves either the previous file or the complete new one — a partial report can never be mistaken for a finished run, and a failed write leaves no temp litter behind. It does not create parent directories, so a typo'd `--dataset` path still fails loudly rather than quietly building directories outside the project. `adapters/`, `models/` and `chats/` are gitignored for the same reason and were checked too: `chats` already calls `mkdirSync` before saving a transcript, and `adapters`/`models` are created by `mlx_lm`'s own `--adapter-path`/`--save-path` handling, so none of them share the bug. Tests cover both failure paths: rendering the command against a project with no `benchmarks/` — which reproduced the exact reported ENOENT before the fix — and unit coverage for directory creation, the atomic write's replace, cleanup and missing-parent behaviour, and a saved run staying discoverable by `listBenchmarks`/`resolveBenchmarkPath`. Closes Nano-Collective#115
Member
|
Please could you fix the conflict @addyCooks? It's good to go apart from that :) |
The conflict was in src/commands/commands.spec.tsx, where both sides appended new tests to the same region of the file. Kept both: this branch's BenchmarkCommand cloned-project test alongside main's new DataListCommand --eval and DataExportCommand --eval tests, each closing its own try/finally. The import list keeps existsSync, which the benchmark test needs. Everything else merged cleanly; ensureBenchmarksDir and writeFileAtomic survive intact in benchmark.tsx and config.ts.
It was the only test anywhere to import src/commands/benchmark.tsx, which put all 1038 of that file's lines into c8's coverage set at 29% — c8 runs without `all: true`, so a file counts only once some test loads it. That alone moved the total from 71.46% to 66.01% and tripped the new fail-on-coverage-drop gate, even though every per-directory number went up. Covering enough of benchmark.tsx to offset it is not reachable: everything past the startLlamaServer call needs a live server, and the project has no mocking library, so the ~440 lines required are out of range. The fix itself stays covered. ensureBenchmarksDir and writeFileAtomic are exercised by seven tests in config.spec.ts, including the rename-failure and missing-parent paths. What is lost is the end-to-end proof that BenchmarkCommand calls ensureBenchmarksDir before writing. Measured on one machine: main 71.21%, this branch 71.36%.
addyCooks
force-pushed
the
fix/115-benchmark-missing-dir
branch
from
August 30, 2026 20:15
471bb46 to
b9aad49
Compare
Contributor
Author
|
Hey @will-lamerton ! |
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
2 participants
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.
Closes #115
Description
nanotune benchmarknever created.nanotune/benchmarks/, so on any project obtained by cloning rather thannanotune initevery write under it failed with a bare ENOENT breaking the first run outright, and discarding a completed run's reports when--datasetpointed outside the project. This creates the directory before the run starts and writes all three files atomically. Rebased onto currentmain, so it merges cleanly alongside #103, #110 and #116.Type of Change
Testing
Automated Tests
pnpm test:allcompletes successfully)Manual Testing
nanotune initnanotune datacommands (add/import/list/validate)nanotune trainnanotune exportnanotune benchmarkChecklist
pnpm format)