Add configurable checkpointing to the larch2 optimization loop - #40
Open
matsen wants to merge 3 commits into
Open
Add configurable checkpointing to the larch2 optimization loop#40matsen wants to merge 3 commits into
matsen wants to merge 3 commits into
Conversation
New CLI flags --checkpoint-after, --checkpoint-prefix, and --resume
let a long-running optimization be interrupted and resumed without
losing progress. The implementation ships in two phases as the issue
requires: Phase 1 writes DAG-only checkpoints (resume == --dag-pb),
Phase 2 writes a full-state larch_checkpoint message (RNG, patience,
results, args fingerprint) that resumes byte-equivalent to an
uninterrupted run with the same seeds.
Determinism fix in build_phylo_dag_from_proto: walk proto IDs in
sorted order and append edges in edge_id order so a save+load round
trip preserves the in-memory phylo_dag's node/edge layout. Sampling
traverses by node-index and children-list order, so the previous
unordered_map iteration broke deterministic resume.
Includes:
- proto/checkpoint.proto and the C++ proto-mirror in
include/larch/checkpoint.hpp.
- Self-contained SHA-256 (FIPS 180-4) for the args fingerprint.
- mt19937 round-trip via std::ostringstream + operator<<.
- Atomic checkpoint write via "<path>.tmp" + rename.
- Unit tests (RNG, proto round-trip, SHA-256 KATs, optional<int64>
presence) and an integration test that asserts canonical-equal
resume across K∈{1,2,3}, multi-resume chain, fingerprint mismatch,
forbidden-input rejection, pre-drift checkpoint emission, and
schema_version-too-new rejection.
Closes #39
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Extract encode_dag_to_proto() helper in save_proto_dag.hpp so the
larch_checkpoint writer no longer duplicates dag-encoding logic.
- Reorder main() resume path to call resume_state_from_msg(ckpt) before
consuming ckpt.dag, so no field is read after a partial move.
- Rename build_checkpoint_msg's completed_iter parameter to
next_iteration to match the proto field semantics.
- static_assert that checkpoint_schema_version > 0, guarding the
looks_like_checkpoint magic-byte test against future field reorders.
- Extend integration test K coverage to {1..5} (every-iteration
interrupt), test --dag-pb as a forbidden flag with --resume, and
guard extract_scores against an empty result.
- Strengthen checkpoint_test round-trip to assert every result/radius
field, and add a non-empty drift_rng round-trip case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Rename radius_result_msg_pb / optimize_result_msg_pb to plain _msg to match the existing patience_state_msg / rng_state_msg / larch_checkpoint_msg naming (the _pb suffix wrongly suggested these were codegen-produced). - Collapse the three opt_u / opt_z / opt_i lambdas in args_fingerprint_payload into a single auto-deduced opt() lambda. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Issue Lead (final review) Category: completed Assessment:
Follow-ups filed: none. The PR body has no DEFERRED section and the changed files have no TODO/FIXME/"defer"/"future work" markers. The reviewer-flagged A4/N3 nits in Action: Phase set to |
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.
Closes #39
Summary
--checkpoint-after <N>,--checkpoint-prefix <P>, and--resume <path>add configurable checkpoint/resume to the larch2 optimization loop.--resumeis functionally equivalent to--dag-pb); Phase 2 writes a full-statelarch_checkpoint(DAG + RNG + patience + results + git SHA + args fingerprint) so a resumed run is canonical-equivalent to an uninterrupted run with the same seeds.--dag-pb,--tree-pb,--fasta,--newick,--refseq,--vcf) since the checkpoint is self-describing.Implementation notes
proto/checkpoint.proto+include/larch/checkpoint.hppdefine thelarch_checkpointmessage family. C++ proto-mirror structs use the existing reflection-based encoder, plus newstd::optional<int64_t>overloads sooptional uint64fields distinguish0from "unset".std::ostringstream/operator<<(the C++ standard guarantees byte-exact round-trip).src/checkpoint.cpp(no new dep).<path>.tmpthenrename.larch_checkpointalways starts with the byte0x08(tag forschema_version, varint), distinguishing it from rawdag_datafiles (0x0aor other length-delimited fields).build_phylo_dag_from_proto: walk proto IDs in sorted order and append edges inedge_idorder so a save+load round trip preserves the in-memoryphylo_dag's node/edge layout. The previous unordered_map iteration and parent-sort silently broke determinism for resume because sampling traverses the DAG by node-index and children-list order.Tests added
test/checkpoint_test.cpp— mt19937 round-trip,larch_checkpointproto round-trip (incl. nesteddag_data, results vector, patience state, RNG state), SHA-256 known-answer tests,optional<int64_t>zero-vs-absent presence semantics.test/checkpoint_integration.sh— end-to-end CLI: Phase 2 deterministic resume for K∈{1,2,3} with canonical-equal final output; multi-resume chain (catches RNG-state non-idempotency); Phase 1 score-not-worse-than-baseline; args fingerprint mismatch refused with the conflicting field named;--resume+--vcfrefused; pre-drift checkpoint emission with--patience 2 --drift 1; futureschema_versionrejected with a clear error.test/dag_canonical_equal.cpp— content-keyed canonical comparison helper used by the integration test (usessample_idfor leaves and the compact-genome string for inner nodes; edge tuples as a multiset). Stronger than the issue's suggested proto-id sort, which would fail when merge state differs across runs and reorders proto-id assignments even though the DAGs are logically equivalent.Test plan
ctest --test-dir build— all 21 tests pass (19 existing + 2 new).--checkpoint-after 1writesout.ckpt-{1..6}.pb.gzand the final output canonical-equals a baseline uninterrupted run for every checkpoint K.--patience 2 --drift 1ondata/testcase/full_dag.pb.gz.🤖 Generated with Claude Code