Estimator overhaul (M-PROGRESS Phase C) + calculation checkpoints (M-CHECKPOINT); v0.7.0 - #46
Open
NCCU-Schultz-Lab wants to merge 8 commits into
Open
Estimator overhaul (M-PROGRESS Phase C) + calculation checkpoints (M-CHECKPOINT); v0.7.0#46NCCU-Schultz-Lab wants to merge 8 commits into
NCCU-Schultz-Lab wants to merge 8 commits into
Conversation
Phase C was opened to fix a pre-run time estimate that M-EST's tier-4 validation had shown sitting outside its +/-25% band, on the assumption that the cost model needed work. Replaying the recorded history through the estimator said otherwise: the median *signed* error was ~0 for every calc type, so the predictor was not biased. It just had enormous spread. The data explains why. The history held 448 records for the identical tuple (frequency, H2O, RHF, 6-31G) with wall times from 0.34 s to 143 s. No cost model fits ground truth that disagrees with itself by 400x. Root cause: QuantUI's own tests were writing into ~/.quantui/logs/perf_log.jsonl. The *_analysis_history suites drive _do_run with a mocked calculation, so each one recorded a fabricated "H2O frequency" whose elapsed_s was really pytest-xdist wall time under contention. Roughly four fifths of the 2773 records were test artifacts. conftest isolated QUANTUI_RESULTS_DIR and QUANTUI_SETTINGS_PATH but never QUANTUI_LOG_DIR. Second contaminant of the same shape: benchmarks._calibration_worker started its perf_counter before importing quantui/pyscf, charging every calibration record with a fresh subprocess's import cost that an in-app run never pays. Worth recording that the pollution was *flattering* the score. Excluding the test-fixture tuples moves median |err| from 37% to 74.5%: 448 identical records are trivially self-predicting, so they inflated apparent accuracy while teaching the model nothing about chemistry. So this commit is about what gets measured and how it is labelled: - conftest._isolate_log_dir points QUANTUI_LOG_DIR at a temp dir for the whole session. The fix at the source; the rest is damage limitation. - log_calculation gains source/warm/import_s/stages, all additive and all omitted when unknown, so "untagged" stays distinguishable from "known to be an app run". - estimate_time partitions the pool by source the way it already did by gpu_used, falling back with a confidence downgrade. The downgrades compose, so falling back on both axes doesn't read as merely "medium". Two tagged records switch a calc type's pool over, so a polluted history is superseded rather than needing to be deleted. - Per-stage wall times are collected in _LogCapture at the existing emit_status boundaries, so no calc module needed new plumbing. _stage_key strips step counters so per-step re-announcements accumulate into one entry. - estimator_eval.py replays the history causally and reports accuracy next to coverage, so a model that stays silent can't look good by refusing to answer. The stage-aware frequency model is deliberately NOT built yet: no record carries stage data, and shipping an unvalidatable model is the exact mistake this phase exists to correct. 59 tests, mutation-checked against three seeded defects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An interrupted long run currently discards everything it finished. A geometry optimization eleven steps into twelve, a PES scan on its last point, a job killed by a closing laptop lid — all start again from nothing. This adds the storage layer and wires it into the three calc types that can usefully continue. The design decision that shaped the rest: resume and warm start are different operations and must not share a key. Resume continues *this* run, so its key includes geometry and calc type. Strict on purpose: resuming into a checkpoint from a different molecule would silently splice two calculations together, which is much worse than not resuming. Warm start reuses a converged density as an initial guess, so its key deliberately omits coordinates. An exact geometry match is the wrong test — a density from a nearby geometry is a good guess, which is exactly what a geometry optimization exploits internally. Checkpoints live in ~/.quantui/checkpoints, not under the result directory as originally sketched: a result directory is created when a calculation succeeds, and the runs that need a checkpoint are the ones that never get there. The constraint that outranks the feature: a checkpoint must never break a calculation. Corrupt metadata, a truncated append, a foreign schema version and an unwritable directory all behave exactly like "no checkpoint". load_state() returning None is the normal failure mode, not an exception — which matters here because a checkpoint is read precisely when something has already gone wrong. Hence os.replace for metadata, append-only for scan points, and treating a bare directory as "no progress" rather than offering a saving that doesn't exist. CHK.1: session_calc points mf.chkfile at the checkpoint and looks up a compatible earlier density. Never reuses the file it is about to overwrite — at best this run's previous attempt, at worst a partial write from the crash being recovered from. CHK.2: the ASE trajectory moves out of the TemporaryDirectory that was discarding it, and resume reloads the BFGS Hessian as well as the geometry. Without the Hessian, BFGS restarts as steepest descent and spends several steps relearning curvature it already had. Starting fresh into an existing checkpoint clears the stored Hessian first, since inheriting curvature from a run this one isn't continuing would be silent and undetectable. CHK.3: points are banked as they finish, and the cache is self-validating — each record stores the coordinate value it was computed at, so changing the scan range makes the match fail and the point recompute. Reusing a point also moves the live ASE atoms onto the stored geometry: each point relaxes from where the last one finished, so skipping without moving would start the next computed point from the wrong place and quietly change the profile. CHK.5: the offer lives on the Calculate tab rather than in History, where the app can say "8 of 20 scan points already computed" instead of asking a bare "Resume?". It refreshes on the same triggers as the time estimate, so it can never describe a calculation the user moved on from. CHK.4 (frequency displacement restart) is deferred. The displacement loop has no natural sequence point to checkpoint at, and freq_ir_workers can run displacements concurrently, so it would need to be safe under parallel append and resume a set rather than a prefix — a design task rather than a wiring one. 111 tests, mutation-checked. Full suite: 2321 passed, 25 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two milestones land here: M-PROGRESS Phase C (the estimator overhaul, which turned out to be a measurement problem rather than a modelling one) and M-CHECKPOINT (CHK.1/2/3/5 — resume an interrupted calculation). Minor rather than patch: checkpointing is a new user-facing capability, and perf records gained new fields. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both files were still untracked when `pre-commit run --all-files` was run before committing, and that command only covers files git knows about — so black never saw them and reported a clean pass. CI ran on the committed tree and reformatted them. Formatting only; no test logic changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three gaps between "the checkpoint layer works" and "a user can actually use it after a failure", found by walking the failure path rather than the happy one. The resume flag was read from the checkbox alone. That checkbox defaults to ticked and is *hidden* when nothing is resumable, so every ordinary run asked to resume — and the optimizer answered with a "no usable checkpoint to resume" warning on a calculation the user had started from scratch. Now gated on real stored progress, read before begin() stamps a fresh "running" status over the interrupted one. The calc-type observer never refreshed the estimate or the resume offer. refresh_resume_notice correctly hides an offer whose calc type no longer matches, but nothing called it on the change most likely to invalidate it — the function was right and unreachable. The pre-run time estimate had the same gap and was equally stale: a Frequency estimate could sit above a Single Point run. One refresh covers both. The failure card said nothing about resuming. The offer lives by the Run button, which is not where anyone looks after a calculation fails, so the feature was undiscoverable in exactly the situation it exists for. The card now names the saved work and the control to tick. Docs: a "Resuming an interrupted calculation" help topic, reachable in-app. It leads with the question users will actually hit — why the offer disappears when a setting changes — and covers which calc types resume, that warm starts need no action, where checkpoints live, and that deleting them is safe. Also notes that resuming a run which keeps failing at the same point just repeats the failure. 18 tests, mutation-checked against all three reverted fixes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Resuming was discoverable only in the session that failed. Both paths that surfaced the offer -- update_estimate and the post-run refresh -- require the calculation to be already reconstructed, and settings other than viz/compute are not persisted. So after a restart the user had to rebuild the exact molecule, method, basis and calc type from memory before QuantUI would admit the checkpoint existed. That is backwards. The runs worth resuming are the long ones, and a long run is exactly the one you walk away from and come back to tomorrow. The History tab now lists every checkpoint with unfinished work, independent of what is currently configured -- molecule, type, theory, progress, age. "Load these settings" restores it; the existing offer above the Run button then takes over and confirms the checkpoint was recognised. Restore deliberately stops short of starting the run: the user should see what they are about to continue. Making that possible needed the starting geometry in meta.json, which was not there -- a listing could otherwise report work it had no way to restore. Since no released version writes checkpoints, this needed no migration. Where coordinates are absent anyway, the entry is still listed but the button is disabled and says why, rather than offering something that cannot work. PES scan settings are stored too. Scan range is not part of the resume key, so a restore that skipped it would reinstate the molecule and method while silently leaving a different scan -- and every stored point, matched by coordinate value, would miss. Ordering that matters: resumability is read before begin() stamps a fresh "running" status, and the molecule is set before the dropdowns so their observers have something to describe. The round trip is tested directly -- a restored checkpoint must rebuild to a matching resume key, or the offer never appears. 33 tests, mutation-checked. One of them caught a weak assertion of my own: startup refreshes through an io_loop when one exists and directly otherwise, and a count-based test passed with one branch removed. Full suite: 2372 passed, 23 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Left over from an earlier draft that constructed a Checkpoint to delete it; the handler removes the directory directly, so the import only existed to carry a noqa. Caught by CI, not locally: pre-commit fixes files and exits non-zero, so a single pass isn't enough when black's output changes ruff's input. Running to a fixed point is the reliable check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every checkpoint state change now writes a [checkpoint] line into the run log, and therefore into the archived pyscf.log: opened (with its path), each save with what changed, scan points by index, completion, and discarding. This deliberately breaks the Phase D precedent that kept the heartbeat out of the archive. The distinction is what the line is for. A heartbeat says "still alive", which is worthless once the run is over. A checkpoint line is provenance, and the resume case makes that concrete: a resumed run's pyscf.log contains only the continuation, because the earlier steps were written to a different run's log in a different result directory. Without a banner saying so, the file reads as a complete calculation that started from the geometry at the top -- which would misrepresent how the result was obtained. So the resume banner states outright that the log is only the continuation and where the rest is. A save is logged only after the write succeeds. A line claiming work was banked when it wasn't is worse than silence, because it is exactly the line someone would later rely on. Warm starts now name their source file rather than saying "a previous run": the SCF iteration count in that log is only interpretable if the reader knows which density it started from. Also answers the question that prompted this: resuming never overwrites anything. save_result builds a microsecond timestamp plus a collision counter and calls mkdir without exist_ok, so each run gets its own directory. Added tests asserting that, since the checkpoint feature now depends on a property owned by another module. Moving the checkpoint open after the log exists caught a NameError that the tests found immediately -- _do_run creates its _LogCapture well after the point where I had put the checkpoint. 22 tests, mutation-checked. Full suite: 2390 passed, 23 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Two milestones. They landed together because the first one's finding — that
QuantUI's own tests were writing into the user's real performance log — was
found while grounding the second.
M-PROGRESS Phase C — the model was never the problem
Phase C existed to fix a pre-run time estimate that M-EST's tier-4 validation
showed sitting outside its ±25% band. The assumption was that the cost model
needed work. Replaying the recorded history through the estimator said
otherwise:
Signed error ≈ 0 everywhere — no bias, which is what a wrong cost model looks
like. Just enormous spread. And the same predictor gets NMR to 87%, so the
spread had to be coming from the data.
It was. The history held 448 records for the identical tuple (frequency,
H2O, RHF, 6-31G) with wall times from 0.34 s to 143 s. No cost model fits
ground truth that disagrees with itself by 400×.
Root cause: the
*_analysis_historysuites drive_do_runend-to-end witha mocked calculation, and
conftest.pyisolatedQUANTUI_RESULTS_DIRandQUANTUI_SETTINGS_PATHbut neverQUANTUI_LOG_DIR. Each test recorded afabricated "H2O frequency" whose
elapsed_swas really pytest-xdist wall timeunder contention. Confirmed directly — running one test file appended 8 records
claiming 56.67 s and 57.47 s. Roughly four fifths of the 2 773 records were
test artifacts.
Second contaminant of the same shape:
benchmarks._calibration_workerstartedits
perf_counterbefore importing quantui/pyscf, charging every calibrationrecord with a fresh subprocess's import cost that an in-app run never pays.
test-fixture tuples moves median |err| from 37% to 74.5% — 448 identical
records are trivially self-predicting, so they inflated apparent accuracy while
teaching the model nothing about chemistry.
So this half is about what gets measured and how it's labelled:
conftest._isolate_log_dir— the fix at the source.log_calculationgainssource/warm/import_s/stages, all additive andomitted when unknown, so "untagged" stays distinguishable from "known to be
an app run".
estimate_timepartitions bysourcethe way it already did bygpu_used,with composing confidence downgrades. Self-healing: two tagged records
switch a calc type's pool over, so the polluted history is superseded rather
than needing deletion.
_LogCaptureat the existingemit_statusboundaries — no calc module needed new plumbing.
python -m quantui.estimator_eval— causal replay reporting accuracy nextto coverage, so a model that stays silent can't look good by refusing to
answer.
The stage-aware frequency model is deliberately not built: no record
carries stage data yet, and shipping an unvalidatable model is the exact
mistake this phase exists to correct. Tracked as PROG.C6.
M-CHECKPOINT — CHK.1, CHK.2, CHK.3, CHK.5
The design decision that shaped the rest: resume and warm start are
different operations and must not share a key. Resume continues this run, so
its key includes geometry — resuming into a checkpoint from a different
molecule would silently splice two calculations together. Warm start reuses a
converged density as an initial guess, so its key deliberately omits
coordinates: a density from a nearby geometry is a good guess, which is exactly
what a geometry optimization exploits internally.
Checkpoints live in
~/.quantui/checkpoints, not under the resultdirectory as the roadmap sketched — a result directory is created when a
calculation succeeds, and the runs that need a checkpoint never get there.
The constraint that outranks the feature: a checkpoint must never break a
calculation. Corrupt metadata, a truncated append, a foreign schema version
and an unwritable directory all behave exactly like "no checkpoint".
mf.chkfileinto the checkpoint dir; compatible earlier densityas the initial guess. Never reuses the file it's about to overwrite.
TemporaryDirectorythat wasdiscarding it. Resume reloads the BFGS Hessian as well as the geometry;
without it BFGS restarts as steepest descent and relearns curvature it had.
record stores the coordinate value it was computed at). Reusing a point also
moves the live ASE atoms, since each point relaxes from where the last
finished.
"8 of 20 scan points already computed" rather than asking a bare "Resume?".
CHK.4 (frequency displacements) deferred —
freq_ir_workerscan rundisplacements concurrently, so it needs to be safe under parallel append and
resume a set rather than a prefix. A design task, not a wiring one.
Testing
170 new tests. Both suites mutation-checked against seeded defects. The
checkpoint wiring tests are structural on purpose — the failure mode here is a
checkpoint created but never passed to the calculation, or a control built and
never added to its container, both of which have precedent in this repo.
Full suite: 2321 passed, 25 skipped.
🤖 Generated with Claude Code