Repository navigation
fix(fusion): LearnableFusionPolicy must not train on an unranked union after re-enabling the tournament - #308
Merged
Conversation
Birfy
enabled auto-merge (squash)
October 16, 2026 11:53
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Birfy
commented
Oct 16, 2026
Birfy
left a comment
Owner
Author
There was a problem hiding this comment.
Automated review — #308 (Closes #302), head 8e71220
Verdict: LGTM ✅ (posted as a comment; self-approval is blocked, and auto-merge already squash-merged this at 20:10 CST, so the items below are follow-up nits, not blockers).
Correctness
- Root cause is addressed in both places it could bite: the default inner is now always
DefaultFusion(tournament=True), and_run_tournamentforces ranking on a caller-supplied inner left attournament=False, restoring it in afinally. The outertournamentflag now only chooses the path (ranked vs_union_policy), which matches the governor'sfp.tournament = shouldcontract inevolution.py/async_evolve.py. - I checked
DefaultFusion.select: in tournament mode it always appends a rankedFusionTrial, and only_commit_unionrecordsranked=False. So the new "no newly-recorded ranked trial → don't count, don't observe" guard never suppresses a legitimate label from the default inner. The existing default (tournament=Truefrom construction) is byte-for-byte the same behaviour. - The budget-disabled path is untouched: no labels, and unranked trials still land on the shared
trialslist thatfusion_statsreads.
Tests (run locally, Python 3.13, fresh clone)
tests/test_learnable_fusion.pypasses on the branch. Withagentdescent/fusion.pyreverted to main, 5 of the 6 new tests fail, which confirms they reproduce #302.test_true_false_true_round_trip_ranks_on_the_second_enablepasses on main too, because aTrue-built inner was never affected. That's fine as a regression guard, but "new tests fail on main" in the description is 5/6, not 6/6.- Full
pytest -qon the branch: everything passed excepttests/test_era_algotune.py::test_the_reference_scores_about_one_through_the_sandbox, which then passed in isolation on both the branch and main. It looks like a load-sensitive sandbox timing flake, not related to this PR. CI is green on build + 3.9–3.13.
Nits / follow-ups
test_the_engine_governor_flip_to_on_ranks_through_evolvedoesn't go throughevolve. It reproduces the attribute write by hand, which is the same as the first test. #302 asked for the governor path specifically. A test that drivesevolve(orasync_evolve) with a budget that crosses the soft floor and then recovers would actually pin the integration. Otherwise, rename it so it doesn't overstate coverage.- Temporarily mutating a caller-supplied inner (
inner.tournament = True…finally: False) is fine for the engine, because the fusion decision runs at the round barrier and the async comment says no worker reads the flag. Still, it isn't reentrant if a user shares oneDefaultFusioninstance elsewhere. A cleaner long-term shape is a ranked entry point on the inner (e.g. a_rank(artifact, diffs)helper thatselectcalls whentournament=True), so the label path never has to flip shared state. - Unranked-only inners become silent no-op learners. With e.g.
ReflectiveFusionasinner, which recordsranked=Falseby contract and has notournamentattribute so it isn't forced, the wrapper now never counts a run or trains. That's correct per #302, but it would help to say so in the class docstring, or warn once instatus(), so a user doesn't wonder whymodel_observationsstays at 0. - The
trials[before:]slice relies ontrialsbeing append-only. It's documented that way onDefaultFusion, but a short comment here noting the dependency would help future readers. - Docs: the
tournamentparameter's meaning changed slightly (it now picks the path, and no longer configures the inner). Consider one line in the class docstring /docs/api.md. The changelog fragment already describes it well.
Nice, tight fix: the scope stays within the acceptance criteria, and the fragment follows the new changelog.d convention.
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 #302
What
DefaultFusionis now always built withtournament=True; the outerLearnableFusionPolicy.tournamentflag only selects the path (ranked vs. budget-degraded union path), so aFalse -> Trueflip (incl. the engine budget governor) reaches a real ranking._run_tournamentforces ranking on a caller-supplied inner left attournament=Falsefor that call only (restored after), and refuses to count a run / train a label if the inner still recorded only unranked trials.Why: follow-up to the #298 review finding — after
tournament=False -> Truethe inner returned an unranked union that was counted as a measured tournament and a positive label.Tests (
tests/test_learnable_fusion.py): False->True flip, True->False->True, epsilon=1 forced exploration, governor-style attribute flip, supplied unranked inner (restored after), inner that still returns unranked trials (no counters/labels). Instrumented verifier asserts real ranking calls; labels come from the measured winner. New tests fail on main; fullpytest -qpasses locally. Fragment:changelog.d/302-learnable-fusion-ranked-inner.md.