Repository navigation
feat: write the run record, config.json, into every new run folder - #291
Conversation
jgieringer
left a comment
There was a problem hiding this comment.
Looks good. Love that commit info makes it into the run config.
Just a few questions, but nothing blocking!
| if run_record is None: | ||
| return None |
There was a problem hiding this comment.
Should we log if the run_record is None?
There was a problem hiding this comment.
yeah that;a great question. maybe we should error out. would would it be None?
There was a problem hiding this comment.
It was None in two cases:
- Continuing a run with
--into.vera_clipassesNoneon purpose, because that folder already has itsconfig.jsonfrom when it started. - Direct calls into the domain (tests). Only this case could leave a new run folder without a
config.json.
Every vera command already passes a record for a new run, so erroring out is safe. Done in 044ef03:
create_run_rootnow requires the record.- Starting a new generation, judging, or pooling run without one raises
ValueError, namingbuild_run_record(require_run_recordinutils/run_record.py). --intonever creates a run folder, so continuing a run is unaffected.
Tests that start runs by calling the domain directly now pass tests/mocks/run_record.minimal_run_record(). That's about 10 call sites; the ones writing into an existing folder (output_folder=) are exempt. I chose an error over a log line because a log would still let folders without a record appear, and in practice it would mostly fire in tests.
| def _git_commit() -> str | None: | ||
| """The checkout's commit, or None outside a git checkout.""" |
| ) | ||
| output_folder = str(create_run_root(output_root, Path(new_folder).name)) | ||
| record = ( | ||
| with_start_time(run_record, started_at) if run_record is not None else None |
There was a problem hiding this comment.
if record is none then we also through away the started_at, is that okay?
There was a problem hiding this comment.
Good catch on how it reads, but the start time isn't lost. Two lines up, timestamp comes from started_at, so it's already in the evaluation folder's name. run_record is None only when judging is called outside a vera command (tests, direct calls into the domain). Those callers have no resolved config, so there's no config.json to write, and nothing to stamp the time into. Every vera judge / vera pipeline run passes a record, and --into never reaches this branch.
The conditional did make it look as though the time were dropped, so b61b987 moves the None check into with_start_time itself, with a docstring saying the above. This call site and the matching ones in generate/run.py and score/pool.py are now a plain with_start_time(run_record, started_at). There's also a test for the None case.
create_run_root now writes the resolved config with every default filled in, the sha256 of each input file, and provenance (vera version, git commit, start time), plus config.json.sha256 over everything except provenance and invocation. The record is a valid config: --config accepts its extra keys and stops if an input file changed since it was written. A generate run with several user models writes one record per folder, naming only that folder's model. --into continues a run without touching its record. This is PR C of docs/design/config-and-manifest.md, and what keeps the short configs allowed by #289 reproducible. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AD-18 said the hash covers debug and sample. config-and-manifest.md, the newer decision, treats them as execution settings outside the hash, and the run record follows it. AD-18 now says so, and why: debug only adds logging and sample only runs a subset, so neither changes how any conversation or evaluation is produced. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d2080f6 to
4e31440
Compare
Review on #291: callers wrote `with_start_time(r, t) if r is not None else None`, which read as though the start time were thrown away. It isn't: the folder name already carries it, and with no record there is no config.json to stamp. with_start_time now handles None itself and says so, so the three callers are one plain call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review on #291: a run record of None meant a new run folder silently got no config.json. It was None in only two cases: --into, which continues a folder that already has its record, and direct calls into the domain (tests). Now create_run_root requires the record, and starting a new generation, judging, or pooling run without one raises, naming build_run_record. --into never creates a root, so it is unaffected. Tests that start runs by calling the domain directly pass tests/mocks/run_record.minimal_run_record(). with_start_time is back to taking a record, since it can no longer be None. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
PR C of
config-and-manifest.md. Stacked on #290, which is stacked on #289. Merge in order, each with a merge commit, and retarget the next PR after each merge.Every new run folder (generation, judging, pooling) now gets:
config.json, the run record. It holds the resolved config with every default written out (the same content--printemits), plus:format_version: 1inputs: sha256 of each file the run reads (personas, context template, rubric files)provenance:vera_version, the code'sgit_commit, andstarted_atconfig.json.sha256, a hash of everything exceptprovenanceandinvocation.New module:
src/vera_mh/utils/run_record.py.create_run_root(#290) writes the record when it's given one.Why it's built this way
Why now, and together with #289. #289 lets a config file leave behavior fields to their defaults. After that, the config file alone no longer pins a run down: if a default later changes, rerunning the same file quietly gives a different run. The design accepts that trade only because the run record writes every value out. Merging #289, #290 and this PR together means
release/2.1never has defaults without a record.Why
create_run_rootwrites it. #268's order of events puts writingconfig.jsonin the same step that creates the run folder. Every run folder comes into being through that one function, so no run can skip its record, and the AST tests from #290 keep it that way.Why one record per run folder.
vera generate -u a -u bmakes two folders, one per user model. If both records named both models,--config <folder>/config.jsonwould regenerate the sibling folder too. So each folder's record names only its own user model, and rerunning a folder repeats exactly that folder.Why
started_atis stamped late. The record is built invera_clibefore the domain runs. The domain then reads the clock once (run_started_at, #290) and uses that single reading for both the folder name and the record'sstarted_at, viawith_start_time. A test checks that the two match.Why the hash leaves out
provenanceandinvocation.provenancedescribes where the run came from, not what it did. A docs-only commit shouldn't make every run look like a different experiment.invocation(debug,sample) is how this execution behaved, which is the same splitRunConfigalready draws.max_concurrentstill lives insidegeneration/judging, so it counts toward the hash until PR E moves it to anexecutionsection. I left that move out to keep this PR to one change.Why input checksums, and why a changed input stops a rerun. Editing a rubric or personas file changes what a run measures, so the checksums are part of the hash: two runs are never mistaken for the same experiment when their inputs differed. On rerun,
--configaccepts the record's extra keys (format_version,inputs,provenance), and a checksum that no longer matches is an error naming the file. Rerunning against an edited rubric under the old record's name would be a different experiment. A plain config has noinputsand is unaffected.Why folders aren't checksummed. Conversations to judge and evaluations to pool are the output of an earlier run, which has its own record.
Why
intois left out. It continues a folder that already exists, which can't be part of that folder's record (seeRunConfig's docstring). A continued run keeps the record it started with and gets no new one.Signature changes (flagged, per AGENTS.md).
run_generation,run_for_user_models,run_judging,judge_conversations, andrun_poolingeach gain one optionalrun_recordargument. Callers outside averacommand (tests, direct domain calls) pass nothing and write no record.Architecture change (escalation): AD-18.
ARCHITECTURE-SPINE.md's AD-18 said theconfig.jsonhash coversdebugandsample. This PR follows the newerconfig-and-manifest.mddecision and leaves them out, so AD-18 now says so and links that decision. The reason:debugonly adds logging andsampleonly runs a subset, so neither changes how any conversation or evaluation is produced. This needs maintainer sign-off.Not in this PR
executionsectionstate.json, the runtime record, which comes after the job loop (docs: design draft — one job loop for generate and judge #266).Testing
tests/unit/utils/test_run_record.py(13 tests) covers:run_generation--intoThe pipeline pooling fake now checks the pooled record names the same evaluations. Locally, unit (1025) and non-live integration (134) pass; ruff, pyright, and import-linter are clean. CI is started by hand because this PR is stacked.
🤖 Generated with Claude Code