fix(llm): keep bandit accumulators linear when asymmetric scaling is off - #191
fix(llm): keep bandit accumulators linear when asymmetric scaling is off#191yurekami wants to merge 2 commits into
Conversation
AsymmetricUCB and ThompsonSampler derive use_exponential_scaling from
exponential_base alone, but only the asymmetric branch of update() and
decay() actually accumulates in log-space; the other branch adds and
scales linearly. So with asymmetric_scaling=False and exponential_base
left at its default of 1.0, the accumulator s is initialised to -inf for
log-space and then updated linearly, and no reward can move it:
s=[-inf -inf -inf] mean=[-inf -inf -inf] obs=(-inf,-inf)
posterior=[0.333 0.333 0.333]
The first decay() also folds those -inf means into the adaptive
observation range, so reward scaling collapses to its fallback and the
same reward maps to a different posterior update before and after.
Model selection degenerates to uniform, silently, for the default ucb
selector as well as thompson. Every constructor kwarg is reachable
through llm_dynamic_selection_kwargs, so the combination is one config
line away, and the existing tests only exercise asymmetric_scaling=False
together with exponential_base=None, where the two flags agree.
Log-space accumulation is only defined when every shifted reward is
>= 0, which is exactly what asymmetric clamping guarantees, so tie
use_exponential_scaling to both flags. Every other site already keys on
that conjunction; __init__, _mean, and the print labels now agree with
them, and the linear mode gives the same numbers whether or not
exponential_base is set.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VZA548KwueeKS1BCRhowZq
|
Thanks for digging into this — the UCB bug is real, the direction of the fix looks right, and the added tests pass locally. Before merge, could you please address these points?
Verification here: targeted bandit/persistence tests |
|
The decay() RuntimeWarning listed under "not touched" above now has its own PR: #192. It is based on main and touches different lines of prioritization.py, so the two merge in either order. |
…on load Async resume loads bandit_state.pkl automatically. A state saved by a run with asymmetric_scaling=False before exponential scaling was tied to that flag carries s = -inf for every arm; interpreted as a linear sum after the fix, s += reward can never move it, so the loaded bandit would stay broken for the rest of the run. Detect that state in set_state (linear mode with -inf in s, which no linear run can produce), reset the reward statistics to the prior, and log a warning saying why and that deleting bandit_state.pkl starts cleanly. The reward history in such a file is not recoverable, so a reset is the safe migration. Unsampled arms in the default log-space mode also sit at -inf and are left alone. Also correct the Thompson claims: on the previous code Thompson's s and means were -inf and its adaptive reward range collapsed after the first decay, but its posterior kept learning through alpha/beta and did not become uniform; that outcome was specific to AsymmetricUCB. The CHANGELOG entry now says so and credits the contributor. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VZA548KwueeKS1BCRhowZq
|
Thank you for the careful read. All three points are addressed in a follow-up commit on this branch. 1. Thompson claims. You are right. I re-ran the reproduction and get the same 2. Resumed state. Went with the preferred option.
3. CHANGELOG attribution. Added Locally on the new head: the two bandit test files 12 passed, |
Summary
AsymmetricUCBandThompsonSamplerderiveuse_exponential_scalingfromexponential_base is not None and asymmetric_scaling, matching whatupdate(),decay(),_impute_worst_reward(), and_have_obs_range()already key on.set_state()detects a state saved by the previous behaviour (linear mode withs = -inf), resets the reward statistics to the prior, and logs a warning saying why and how to start cleanly.tests/test_bandit_linear_scaling.py(6, both bandits) andtests/test_bandit_state_migration.py(6, both bandits).CHANGELOG.mdentry.Why
Both bandits set
use_exponential_scaling = self.exponential_base is not Noneand use that flag alone to decide how to initialise and read the reward accumulators:but
update()anddecay()only accumulate in log-space under the conjunction withasymmetric_scaling, and fall through to plain addition otherwise:So with
asymmetric_scaling=Falseandexponential_baseleft at its default of1.0,sstarts at-inffor a log-space sum and is then updated with linear addition, and no reward can ever move it. Both bandits reachs = [-inf, -inf, -inf]after six real rewards, but what that does to selection differs by bandit:AsymmetricUCB: its scores are built from_mean(), so every arm scores the same and the posterior is uniform,[0.333, 0.333, 0.333]against[0.1, 0.8, 0.1]for the same rewards withexponential_base=None. Model selection is a coin flip.ThompsonSampler: its posterior samplesalpha/beta, which are updated from the rescaled reward rather than froms, so it keeps learning; with the same rewards and seed the posterior is[0.05, 0.9, 0.05]. The damage is in the rescaling: the firstdecay()folds the-infmeans into the adaptive observation range,_obs_min/_obs_maxbecome-inf,_have_obs_range()flips toFalse, andreshift_in_rangefalls back to the sigmoid. The same reward of1.5maps tou = 0.667before that decay andu = 0.818after it, so the posterior updates change meaning mid-run.The first version of this description said selection collapsed to uniform for both; that was accurate for UCB only, and is corrected above and in the CHANGELOG.
Every constructor kwarg is reachable through
llm_dynamic_selection_kwargs, soasymmetric_scaling: falseis one config line away and does not require touchingexponential_baseat all.Why this direction rather than making the combination log-space end to end: a log-space sum is only defined when every shifted reward is
>= 0, andr = max(r, 0.0)under asymmetric clamping is exactly what guarantees that. Withasymmetric_scaling=False, negative shifted rewards are allowed, and_logexpm1(z)forz < 0is the log of a negative number. Exponential scaling presupposes asymmetric clamping, so the linear branch is the only one that can be correct without it. Tying the flag to both makes__init__,_mean, and the print labels agree with the sites that already do this; the linear mode then produces identical numbers whether or notexponential_baseis set.Resumed state
Async resume loads
bandit_state.pklautomatically (AsyncEvolutionRunner._load_bandit_state). A state saved by an affected run carriess = -inffor every arm; loaded into the fixed linear mode it would be interpreted as a linear sum, ands += rewardcan never move it, so the bandit would stay broken for the rest of the run.set_state()in both bandits now calls_discard_invalid_linear_state()before restoring the observation range. The condition isnot use_exponential_scaling and any(s == -inf), which no linear run can produce; in the default log-space mode-infmarks an unsampled arm and is left alone. When it fires, the reward statistics (s,divs,n_submitted,n_completed, the observation range, and for Thompsonalpha/beta) go back to the prior and a warning is logged:The reward history in such a file is not recoverable (the sums were never accumulated), so a reset is the safe migration. Cost statistics and the baseline are kept; they were not affected. The CHANGELOG entry documents the reset.
Linked issue or context
No open issue. Found while reading
shinka/llm/prioritization.py. The existing tests only exerciseasymmetric_scaling=Falsetogether withexponential_base=None(test_bandit_persistence.py), where the two flags agree, so the mismatch never surfaced.Testing
uv run ruff check tests --exclude tests/file.py-> All checks passeduv run ruff check shinka/llm/prioritization.py-> All checks passeduv run ruff format --check shinka/llm/prioritization.py tests/test_bandit_linear_scaling.py tests/test_bandit_state_migration.py-> already formatteduv run mypy --follow-imports=skip --ignore-missing-imports tests/test_bandit_linear_scaling.py tests/test_bandit_state_migration.py tests/conftest.py-> Successuv run pytest -q -m "not requires_secrets"-> see resultstests/test_bandit_linear_scaling.py(6): fail onmain, pass with the change; the one that passes either way is the Thompsonalpha/betacredit guard, which never reads, consistent with the corrected description above.tests/test_bandit_state_migration.py(6): a state from a broken run is reset with the warning and learning resumes from the prior (UCB then prefers the best arm, Thompson credits it); a valid linear state loads unchanged with no warning; a default-mode state whose unsampled arms sit at-infloads unchanged with no warning. The first of these fails without the migration code (no warning,sstays-inf).Full non-secret suite locally: the same 3 pre-existing environment failures as before (
wolframscript,claudebinary, Windows subprocess timeout), everything else green; exact counts in the review thread.Risks and compatibility
asymmetric_scaling=True, exponential_base=1.0keepsuse_exponential_scaling=True; every existing test passes unmodified.asymmetric_scaling=Falseruns change behaviour, from degenerate to correct. They now accumulate linearly, exactly asexponential_base=Nonealready did.exponential_baseis ignored whenasymmetric_scaling=False. That was already true ofupdate(); this makes it true of__init__and_mean()too. No warning is emitted for it, becauseexponential_basehas a non-Nonedefault and most users in this mode never set it; happy to add one gated on a non-default value if you would rather it be loud.decay()RuntimeWarningnoted earlier is handled separately in fix(llm): silence the discarded np.where branch in ThompsonSampler.decay #192.Core evolution pipeline evidence
This changes which LLM the bandit samples. For
AsymmetricUCB(asymmetric_scaling=False)with the defaultexponential_base(rewards1.0, 5.0, 0.2, 1.2, 4.8, 0.3across three arms,shift_by_baseline=False,shift_by_parent=False, onedecay(0.95)):Before:
After:
which is bit-identical to the
exponential_base=Nonerun. ForThompsonSamplerthe posterior was already[0.05, 0.9, 0.05]before; what changes is thats,_mean(), and the observation range are finite and the reward rescaling no longer collapses on the first decay. I have not run a full evolution benchmark.Docs and UI
No docs or UI changes.
asymmetric_scalingandexponential_baseare not documented indocs/bandit_selection.mdtoday; the CHANGELOG entry describes the change and the reset-on-load behaviour.Prepared with Claude Code (agent-assisted); every command and result above was run locally.
🤖 Generated with Claude Code
https://claude.ai/code/session_01VZA548KwueeKS1BCRhowZq