Repository navigation
feat(fusion): LearnableFusionPolicy — the merge learns when merging wins - #298
Conversation
An online logistic over diff-only features (candidate count, key overlap, diff sizes; zero verifier calls) is trained on the tournament's own FusionTrial labels. Once confident that the union is the right answer for a merge shape, it returns the union without buying the cheap ranking sweep; uncertain merges still run the tournament and observe a fresh label. Safety (the allocator's Birfy#204 pattern applied to the fusion decision): - default is the default: no trials -> DefaultFusion(tournament=True) verbatim; - counterfactual exploration (epsilon) re-verifies confident skips; - rollback guard retires skipping permanently after a confident-and-wrong counterfactual (max_wrong_skips, default 1); - tournament=False (the budget governor's flip) degrades to the plain union; - commits are unaffected: a skip returns the same union the non-tournament default would have committed, and the acceptance gate still scores it on the full held-out set.
Birfy
left a comment
There was a problem hiding this comment.
Verdict: request changes
Good idea and solid framing — this really is the allocator (#204) pattern applied to the fusion tournament: opt-in, default-is-the-default, epsilon counterfactuals, and a hard rollback. The test suite pins the right safety properties, docs/api.md is regenerated, and the policy satisfies FusionPolicy (select + engine-visible trials on the happy path). Locally: tests/test_learnable_fusion.py (10) + changelog/public-surface checks all green. CI at review time: build green; test (3.9–3.13) still pending.
Two real holes in the advertised safety / engine contract need fixing before approve; the rest are nits.
Blocking
1. Rollback guard scores confidence after update() — misses confident-and-wrong
In _observe:
self._model.update(features, fused)
...
predicted_skip = self._model.predict(features) >= 1.0 - self.threshold
if predicted_skip and not fused:
self._wrong_skips += 1
...The docstring / PR promise is: “a counterfactual merge the model was confident about skipping but the tournament ranked a single instead” retires skipping. That question is about the pre-update prediction (the rule that would have skipped). After a negative label, SGD pulls p down; often it falls below the confidence cut in a single step, so the guard never counts the miss.
Reproduced with the default lr=0.2, uncertainty_threshold=0.15 (cut 0.85), same two-diff fixture as the tests:
pre-update p |
post-update p |
guard (pre) | guard (post, as coded) |
|---|---|---|---|
| 0.863 | 0.727 | fire | miss |
| 0.903 | 0.790 | fire | miss |
| 0.923 | 0.826 | fire | miss |
test_the_rollback_guard_retires_a_wrong_learner still passes only because 25 union-wins push p≈0.95, where one update stays above 0.85. Right after min_trials (and often later) the safety guarantee does not hold.
Fix: snapshot predict(features) before update, and use that for the rollback check (still update every tournament label as today). Please add a regression that starts from a barely-confident p (or injects weights) so a single wrong counterfactual retires skipping.
2. tournament=False (budget governor) drops engine-visible trials
if not self.tournament:
return self._union_policy.select(artifact, diffs)self.trials is aliased to self.inner.trials, but the union path appends to _union_policy.trials. After the governor does fusion_policy.tournament = False:
DefaultFusion: still records an unrankedFusionTrialon the same list the engine reads.LearnableFusionPolicy: engine-visibletrialsstay empty; the record lands on a private list.
So under budget pressure, EvolutionResult.fusion_stats goes silent for this policy — a behaviour change vs the tournament flag this attribute is meant to mirror.
Fix: route the degrade path through inner with tournament flipped off (and restored), or otherwise append onto the same list self.trials points at. Keep “no learning labels” (don’t call _observe); do keep the unranked trial instrumentation.
Also required (repo convention)
3. Changelog fragment name
changelog.d/999-learnable-fusion.md is a placeholder. Per changelog.d/README.md, use the issue or PR number: changelog.d/298-learnable-fusion.md, and mention (#298) in the body. python -m tools.changelog_fragments check accepts 999 today, but release ordering / uniqueness is why the number exists.
Nits (non-blocking)
- Unused import:
import collectionsat the top offusion.pyis never referenced. status()["skips_per_merge"]: implemented asskipped / tournament_runs, so it can be3.0after a warm skip-heavy run. Either rename (e.g.skips_per_tournament_run) or useskipped / (skipped + tournament_runs).- Doc overclaim:
selectsays “when the union provably wins” — it is a learned logistic, not a proof. “Confidently predicted to win” matches the code. - Duplicated
_union_of: byte-for-byte copy ofDefaultFusion._union_of. Calling throughinner(or a shared helper) avoids silent drift if the three-way “nothing-to-fuse” rule ever changes again. - Knob validation:
uncertainty_threshold/epsilon/max_wrong_skipsare checked;min_trials,lr,reg,max_logitare not (NaN / non-positive). Mild, given recent numerical-safety work elsewhere. _FusionModelvsLearningValueModel: same idea, slightly different weight layout (biasat[-1]here vs allocator’s documented layout). Fine as a private twin; a one-line “deliberately not shared” note would help the next reader.
Strengths
- Opt-in via
Policies(fusion=LearnableFusionPolicy(...)); shippedDefaultFusionunchanged. - Diff-only features (no verifier in the skip path); skip returns the same union
DefaultFusion(tournament=False)would commit; acceptance gate untouched — the “commits unaffected” claim is right. - Epsilon is applied before the confidence gate so exploration only matters on would-be skips; uncertain merges still always rank.
- Tests cover default-is-tournament, no-skip-when-union-loses, conservative ties, bind forwarding, bad knobs, and an
evolveend-to-end withfusion_stats. - Package export + api doc count bump look correct.
Happy to approve once (1)+(2) are fixed and the fragment is renamed — the design is the right one.
|
P2 — Re-enabling tournaments can train on an unranked union (source review at
Please ensure the learning/exploration path enables the inner tournament. Suggested regression: construct with |
…trials on degrade, knob validation - Rollback guard now snapshots the prediction *before* update(): the question is whether the rule in force would have skipped, and a negative label drags the logistic below the confidence cut in one step — scoring after the update missed exactly the confident-and-wrong counterfactual the guard exists for. Regression injects barely-confident weights (p 0.86 -> 0.84 after one negative label) and proves a single wrong skip retires skipping. - The budget-degraded union path shares the inner's trials list, so fusion_stats stays live after the governor flips tournament=False (unranked trials recorded, as DefaultFusion does). - _union_of delegates to the inner policy's own rule instead of a byte-for-byte copy, so the three-way nothing-to-fuse rule cannot silently drift. - Knob validation extended to min_trials/lr/reg/max_logit; skips_per_merge is now skipped/(skipped+tournament_runs); unused import removed; _FusionModel documents why it is a deliberate twin of the allocator's model. - Changelog fragment renamed to 298-learnable-fusion.md with (Birfy#298).
|
All items addressed in Blocking 1 — guard scores confidence after update() — fixed. Blocking 2 — tournament=False drops engine-visible trials — fixed. The budget-degraded union path now shares the inner's trials list ( Blocking 3 — changelog fragment name — renamed to Nits — all addressed: unused Regression: 91 tests (fusion/aggregator/api) green locally. |
Birfy
left a comment
There was a problem hiding this comment.
Re-review at 42f3231: blocking items fixed
Verified against the prior REQUEST_CHANGES:
- Pre-update rollback prediction —
_observenow snapshotspredict(features)beforeupdate.test_the_guard_scores_the_pre_update_predictioninjects barely-confident weights (p≈0.86) and proves a single wrong counterfactual retires skipping. - Budget-degrade
trials—_union_policy.trialsis aliased toself.inner.trials, sotournament=Falsestill appends an unrankedFusionTrialon the listfusion_statsreads. Covered bytest_turning_the_tournament_off_degrades_to_the_union_path. - Fragment — renamed to
changelog.d/298-learnable-fusion.mdwith(#298)in the body.
Also picked up the nits that landed in the same fix commit: knob validation for min_trials/lr/reg/max_logit, _union_of delegated through inner, skips_per_merge denominator, doc wording, and the _FusionModel "deliberately not shared" note.
Exact-head CI: build green; tests 3.10–3.11 green at review time (remaining versions still finishing). Approving the design and the safety fixes.
feat(fusion): LearnableFusionPolicy — the merge learns when merging wins
The problem
The project's founding claim is "gradients add, diffs do not" — and the fusion decision is the heart of it. But
DefaultFusion's tournament (the only instrument that answers "does merging beat the best single?") scores every candidate on the cheap layer every merge and then throws the answer away into aFusionTrial. That history is the answer to "when does merging win", and nothing learns it. Meanwhile the tournament's cost is why it is opt-in: an unconditional cheap sweep per merge for a conditional, workload-dependent gain.What this PR adds
LearnableFusionPolicy(agentdescent/fusion.py, installed asPolicies(fusion=...)) — an online logistic over diff-only features (candidate count, key overlap, diff sizes; zero verifier calls) trained on the tournament's ownFusionTriallabels. Once it is confident that the union is the right answer for a merge shape, it returns the union without buying the cheap ranking sweep; uncertain merges — or merges where the prediction says the tournament would change the outcome — still run the tournament and observe a fresh label.This is the allocator's idea (#204, value-directed spending learned from the run's own history) applied to the fusion decision: the optimizer learns when to optimize.
The safety it ships with
DefaultFusion(tournament=True)verbatim — every merge ranked, every trial recordedepsilonof the confident skips are still run through the tournament, so a wrong prediction is observed rather than reinforced (the allocator's counterfactual exploration)max_wrong_skipssuch events (default 1) — a rule that learned to be wrong is disabled, not patched; a false alarm only costs the saving, a wrong skip commits a worse mergetournament=False(the budget governor's flip) degrades to the plain union path, no labels collectedBackward compatibility
Policies(fusion=)NoneDefaultFusionunchangedLearnableFusionPolicyDefaultFusion(tournament=True)verbatim.trials/fusion_statsaggregator.fusion_policy.trials, which the learner exposes as the inner's real measured trialstournamentattributeTruefp.tournament = shouldflip keeps workingbind/configureinstall_policyhooks honouredVerification
tournament=Falsedegrades to the plain unionbindforwards the verifier to inner + union pathevolve(Policies(fusion=...)),fusion_statsreporteddocs/api.mdregenerated, changelog fragment valid, api reference greenFiles
5 files changed, +443, -3
Modified:
agentdescent/fusion.py(+~400) —LearnableFusionPolicy,_FusionModel(mirrors the allocator'sLearningValueModel)agentdescent/__init__.py— exporttests/test_learnable_fusion.py(+~200) — 10 testsdocs/api.md— regeneratedchangelog.d/999-learnable-fusion.md— fragment (per the new fragments convention; the number is a placeholder for the PR)