refactor(core): drop dead optimizer-memory API + unused params; fix misleading cache docstring - #212
refactor(core): drop dead optimizer-memory API + unused params; fix misleading cache docstring#212OsherElhadad wants to merge 2 commits into
Conversation
…ache docstring
The issue's "memory.py is write-only" premise is only PARTLY right. The
render/entries API and the note/impact fields ARE dead, but the jsonl files
themselves have a live reader the issue told us to verify: the dashboard's
GET /api/runs/{id}/memory (dashboard/backend/capevolve_dashboard/memory.py),
which feeds the Memory panel and the Insights "dead ends" grouping. So the
WRITES stay; only the genuinely-unread surface goes.
Removed (zero readers, proven by grep):
- RejectedMemory.render / .entries, History.render / .entries, _render_impact,
_store_impact — the prompt-facing API. LEDGER/JOURNAL/RUNMAP replaced it.
- the `note=` and `impact=` kwargs and the `note`/`broke`/`fixed` record fields.
- harness._latest_journal_note — its only caller was that dead `note=`.
- the per-iteration _candidate_task_impact call in run_step, which existed only
to populate those dead fields (re-read rollouts from disk every iteration).
The LEDGER and _reconcile_journal paths keep their own computations.
- the unused `rejected` / `history` params on _augment_instructions and
_build_ledger.
_init_memory_store is untouched, so PR #204's algorithm-label stamp and the
dashboard badge are unaffected.
cache.py's docstring claimed wiring into evaluate_candidate was "OFF by default
and gated behind a flag (see maybe_cached_score)" — no such wiring, flag, or
function exists. Replaced with what the cache actually does: GEPA-only, consumed
solely by gepa._eval_minibatch; evaluate_candidate always pays full price.
Test change: test_rejected_memory_roundtrip_and_render tested the removed
render(); replaced by test_memory_jsonl_record_shape_matches_dashboard_contract,
which pins the exact keys the dashboard reads — the contract that actually
matters. Net test count unchanged.
Closes #114
|
❌ Automatic Labeling Failed An error occurred while trying to automatically label this pull request. Please check the workflow logs for details and add labels manually. |
🔬 EvidenceAll commands run in a clean worktree at 1. Write-only proof — the prompt-facing API has zero callersEvery hit is 2. Every WRITE site (all kept — they feed the dashboard)Note GEPA never passed 3. The live reader that contradicts the issue's parentheticalServed + consumed: 4. Field-level dead/live audit5. No
|
🔍 Review — PR #212Verdict: APPROVE WITH NITS The issue's premise was wrong and the author is right. BlockingNone. Non-blocking
Nits
Dead-vs-live auditGreps run over
Anything KEPT that may be dead:
Algorithm-stamp survivalSurvives. Merged tree built for real at Real zero-API e2e on All three non-blank, matching the author's claim. Merged-tree suite: 203 passed; merged-tree vitest 52 passed; merged-tree backend 43 passed; Did anything live get deleted?NO. Every deleted symbol has zero readers (table above). The live path is intact end to end after this PR — real e2e on this branch, then Non-empty for all three, exactly the 4 keys
Replacement test — is it genuinely stronger?Yes. Mutation-verified rather than assumed. Renamed (reverted). It fails on the rename that would blank the Memory panel's reject line and break
|
…mory->prompt framing Review fixes for #212: 1. cache.py — restore main's intro paragraph verbatim (the sentence #211 rewrites) and confine the correction to a separate "Scope: GEPA only" paragraph, so the #211 conflict is a single textual hunk whose wrong resolution can no longer restore the false maybe_cached_score line — that line's removal now auto-merges outside the conflict region. 2. test_w1_engine.py — new guard pinning that no doc under core/ or skills/ cites maybe_cached_score, so a revert of the docstring fix fails a test. 3. skillopt SKILL.md / concepts.md / skillopt.py — the rejected/history jsonl are dashboard audit records, write-only, never prompt input. 4. MemoryPanel.tsx — drop the "do-not-re-propose" framing this PR disproved.
🔧 Review fixesAll 4 findings addressed. Commit
|
| File | Change |
|---|---|
core/cap_evolve/cache.py |
docstring reshaped so #211 merges clean |
core/tests/test_w1_engine.py |
+8 — maybe_cached_score absence guard |
core/cap_evolve/skillopt.py |
module docstring — write-only framing (3rd surface) |
skills/algorithms/skillopt/SKILL.md |
audit-record framing |
skills/algorithms/skillopt/references/concepts.md |
audit-record framing |
dashboard/frontend/src/components/MemoryPanel.tsx |
doc comment — nit 4 |
#212 narrows the signature (drops rejected/history). Passing them positionally made this a THIRD mechanical merge site — and unlike the two in harness.py it surfaced as a test failure git does not flag as a conflict. Fill trailing params reflectively so the test passes on both signatures.
Fourth mechanical merge site for #212's narrowed signatures (two in harness.py, two here) — all in tests, so git flags none of them as conflicts. Also refresh the module docstring for the renamed sections and the new pinned properties.
Closes #114
The issue's premise is only PARTLY right — scoped accordingly
#114 says
memory.pyis write-only and the jsonl machinery can go. I checked empirically before deleting, and the issue itself asked to "confirm therejected.jsonl/history.jsonlfiles aren't relied on by the dashboard before removing writes … (the dashboard readsevents.jsonl, not these — verify)".Verified: that parenthetical is wrong. The jsonl files have a live reader. So I removed only the genuinely-unread surface and kept the writes.
Write-only proof
The prompt-facing API —
render(),entries(),_render_impact— has zero callers outsidememory.pyitself:Every hit is
memory.pycalling itself.LEDGER.md/JOURNAL.md/RUNMAP.md(via_augment_instructions) fully replaced this as the optimizer's cross-iteration channel. Dead → removed.Same for the per-record
note/broke/fixedfields — written on every iteration, read by nobody:Three CSS/prose coincidences, zero real readers. Field-level audit against the dashboard's consumers:
candidate_idsummaryvalreasonnotebrokefixedWhat I did NOT delete, and why
read_memory()is served atGET /api/runs/{run_id}/memory(app.py:60), consumed byMemoryPanel.tsx(the Deep-Dive Memory tab) andinsights.ts::deadEnds()(the Insights "what not to try" grouping), and baked intoexport_static.py:83. Deleting the writes would have blanked two shipped UI panels. The.add()writes and both jsonl files stay — they are audit/UI records, which is now what the module docstring says they are.Deleted
RejectedMemory.render/.entries,History.render/.entries_render_impact,_store_impactnote=/impact=kwargs +note/broke/fixedfieldsharness._latest_journal_notenote=_candidate_task_impactcall inrun_step_build_ledgerand_reconcile_journalkeep their own computations, so no signal is lostrejected/historyparams on_augment_instructions+_build_ledger82 insertions, 177 deletions across 9 files (net −95).
memory.py: 145 → 54 lines. No files removed (memory.pystill carries the two live writers).Algorithm-label stamp: preserved, untouched
_init_memory_storeis not renamed, moved, or inlined — my change does not touch it at all, so PR #204'salgorithmstamp and the dashboard badge are structurally unaffected. Proven on a locally-built#199 + #204 + thismerge, all three deterministic loops:and the dashboard summary is non-blank for each:
'hill-climb:all','gepa','skillopt'.Corrected cache docstring
Was — describing a flag, a wiring, and a function that do not exist:
There is no
maybe_cached_scoreanywhere in the repo. Now:#199's
_IGNORE_DIRS/INJECTED_*change is untouched (it merges clean).Note for #128 / #129
Those add a genuinely-read memory. Build on
_augment_instructions, not onmemory.py. That is the only function whose output reaches the optimizer prompt, and it is now 3 params instead of 5 — add one there and every algorithm (hill-climb, gepa, skillopt) picks it up, since all three route through it.memory.py's docstring now says this explicitly so nobody re-adds arender()expecting it to reach a prompt. I deliberately built no speculative scaffolding for them.Expected merge order
#199 → #204 → this PR. Based on
origin/main; #199 and #204 conflict with each other (both edit the_init_memory_storecall sites) — that is pre-existing and independent of this PR. After both land, this PR conflicts on exactly one hunk ingepa.py:614(#199'srender_instructionsvs my 2-arg_augment_instructions); resolution is mechanical — keep #199's call, drop, rejected, history:I verified that resolution locally: 203 passed on the merged tree.
Verification
Full core suite on this branch (baseline 179)
179 passed, 0 failed — no net test loss. One test was replaced, not dropped:
test_rejected_memory_roundtrip_and_renderasserted on the removedrender(), so it could not survive. It is superseded bytest_memory_jsonl_record_shape_matches_dashboard_contract, which pins the exact key set the dashboard reads ({candidate_id, summary, reason, val}/{candidate_id, summary, val}) — a stronger guard, since it fails if anyone renames or adds a field and silently breaks the Memory panel.Dashboard backend (would have caught a removed write)
compileall
Real e2e, all three deterministic algorithms, zero API cost
Every one reaches the sealed test number 1.0 ± 0.0 and the memory jsonl still round-trips through the real dashboard reader:
algorithmcand_0001hill-climb:allgepa_0001gepaso_e01s01skilloptRecords carry exactly the four live keys and no dead
note/broke/fixed— the deletion is visible on disk.Zero new runtime deps; tests stay in
core/tests/.