feat(reproducibility): add initial-state loading and checkpoint resume - #157
Conversation
- let initialize publish manifested states that cdt validates before starting a new seeded transition stream - persist PCG state, global pass cadence, transition trace, counters, and producer identity for exact continuation - expose load and resume workflows for local and interrupted HPC runs - canonicalize CGAL flip descriptors and reject ambiguous coordinate states at evolution boundaries - reorganize user and maintainer documentation around the supported archival workflows
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThe change adds initial-triangulation input and exact checkpoint resumption. It extends persistence metadata with provenance, RNG state, counters, and validation. It hardens move determinism, adds integration tests, updates workflow recipes, and expands archival documentation. ChangesArtifact workflows and reproducibility
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant initializer
participant cdt
participant read_initial_triangulation
participant read_checkpoint
participant Metropolis
initializer->>initializer: write initial-triangulation artifact
cdt->>read_initial_triangulation: validate --input artifact
read_initial_triangulation->>Metropolis: provide new-run triangulation
cdt->>read_checkpoint: validate --resume artifact
read_checkpoint->>Metropolis: provide exact continuation state
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmake/RunInitializeToCdtTest.cmake`:
- Around line 166-180: Create a checkpoint artifact in a separate temporary
directory within the test flow, then invoke the CDT executable with that
checkpoint via --input using the same initial-artifact validation pattern as the
final-artifact case. Require a nonzero result and verify the output/error log
contains “CDT input must be an initial-triangulation artifact,” using the
existing checkpoint-generation symbols and cleanup conventions.
In `@docs/ergodic-moves.md`:
- Around line 123-128: Update the list around the malformed-handle and
facet-centroid items by removing the trailing “and” connector from the earlier
item, leaving the connector only on the final list item.
In `@include/Utilities.hpp`:
- Around line 2415-2418: Introduce a role-neutral artifact type for the shared
structure currently represented by Initial_triangulation_artifact, then define
both Initial_triangulation_artifact and Checkpoint_artifact as aliases to it.
Update the relevant template documentation and preserve existing reader
interfaces while ensuring read_checkpoint exposes the neutral type name.
- Around line 1443-1455: Ensure the provenance construction used by Metropolis_3
supplies max_threads for resumable checkpoints, or marks metadata non-resumable
when the caller omits it. Update the relevant default-provenance/API path while
preserving max_threads in complete_resume_state, since resume handling requires
it to determine the effective thread count.
In `@tests/Ergodic_moves_3_audit_test.cpp`:
- Around line 727-728: Update the assertion around cell_iteration_order in the
affected test to avoid requiring CGAL’s raw finite-cell traversal order; either
change REQUIRE_FALSE to WARN_FALSE for diagnostic-only output or remove the
check, while preserving canonical-state equality as the test contract.
In `@tests/Utilities_test.cpp`:
- Around line 757-773: Extract the duplicated resumable-checkpoint metadata
assignments into a local helper named make_resumable_checkpoint_metadata near
the existing test helpers, returning the fully initialized
Reproducibility_metadata. Replace the repeated setup in both scenarios with
calls to this helper, preserving all current values and ensuring both tests use
the same complete resume contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef7a04ea-2390-41d0-9c6d-d4518f84c813
📒 Files selected for processing (23)
.github/CONTRIBUTING.mdJustfileREADME.mdcmake/RunCheckpointResumeTest.cmakecmake/RunInitializeToCdtTest.cmakedocs/api-boundary.mddocs/ergodic-moves.mddocs/reproducibility.mdinclude/Ergodic_moves_3.hppinclude/Metropolis.hppinclude/Move_always.hppinclude/Move_run.hppinclude/Random.hppinclude/Utilities.hppreference/README.mdscripts/validate_generated_site.pysrc/CMakeLists.txtsrc/cdt.cpptests/Ergodic_moves_3_audit_test.cpptests/Metropolis_test.cpptests/Move_run_test.cpptests/Random_test.cpptests/Utilities_test.cpp
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
+ Coverage 91.11% 92.43% +1.32%
==========================================
Files 19 19
Lines 3476 3990 +514
Branches 1614 1908 +294
==========================================
+ Hits 3167 3688 +521
+ Misses 309 302 -7
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
- Reject checkpoint artifacts supplied as fresh CDT input. - Advertise checkpoints as resumable only with complete thread and RNG state. - Validate restored PCG streams against the engine’s normalized stream. - Share one artifact representation across initialization and checkpoint roles.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/Metropolis.hpp (1)
251-266: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate the supplied generator against checkpoint state.
Lines 251-266 only require that
transition_random_stateexists. They do not verify thatm_generatoris restored to that state. A caller can pass anothercdt::Randomwith resumable metadata. The strategy then restores counters but advances a different PCG state. The generated checkpoint can falsely claim exact continuation.Compare the supplied generator seed, stream, and serialized state with the recorded checkpoint state before clearing
transition_random_state. Add a rejection test that supplies a different generator.Proposed fix
if (m_completed_passes > 0 || m_reproducibility.transition_random_state) { if (!m_reproducibility.configured_passes || *m_reproducibility.configured_passes != total_passes || !m_reproducibility.transition_random_state) { throw std::invalid_argument( "Checkpoint resume state does not match its pass range."); } + if (m_generator.seed() != m_reproducibility.seed || + m_generator.stream() != m_reproducibility.transition_stream || + m_generator.serialized_state() != + *m_reproducibility.transition_random_state) + { + throw std::invalid_argument( + "Checkpoint resume generator does not match saved state."); + } restore_statistics(m_reproducibility); m_resume_pending = true; }As per path instructions: "Focus on design patterns, interfaces, and encapsulation".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Metropolis.hpp` around lines 251 - 266, Update the resume validation in the constructor or setup method containing m_reproducibility so it compares the supplied m_generator’s seed, stream, and serialized state with transition_random_state before restore_statistics and before clearing it. Reject any mismatch, while preserving the existing pass-range validation and successful continuation for an identical generator; add a test that resumes with a different cdt::Random and expects rejection.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/Metropolis.hpp`:
- Around line 550-554: Update the checkpoint branch in Metropolis serialization
to require metadata.max_threads to contain a positive value, not merely any
optional value, before assigning transition_random_state. Preserve the existing
checkpoint and state-serialization behavior for valid positive limits, and add
coverage for max_threads = 0.
---
Outside diff comments:
In `@include/Metropolis.hpp`:
- Around line 251-266: Update the resume validation in the constructor or setup
method containing m_reproducibility so it compares the supplied m_generator’s
seed, stream, and serialized state with transition_random_state before
restore_statistics and before clearing it. Reject any mismatch, while preserving
the existing pass-range validation and successful continuation for an identical
generator; add a test that resumes with a different cdt::Random and expects
rejection.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45249292-030a-43cc-bc61-b86bd6f2dc7e
📒 Files selected for processing (8)
cmake/RunInitializeToCdtTest.cmakedocs/ergodic-moves.mdinclude/Metropolis.hppinclude/Random.hppinclude/Utilities.hpptests/Ergodic_moves_3_audit_test.cpptests/Metropolis_test.cpptests/Utilities_test.cpp
- reject generators and thread limits that cannot reproduce saved state - restore PCG checkpoints when using emulated 128-bit arithmetic
1 participant
Closes #90
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests