Low-hanging-fruit bug fixes: W5 gauntlet unpack, W1 atomic result, W11b SL lambda validation - #63
Merged
Merged
Conversation
…042dd1) historical_gauntlet.run_gauntlet did `wins, losses, draws = play_match(...)` but play_match returns a MatchOutcome dataclass without __iter__, so every real gauntlet match raised TypeError caught by the broad per-slot except, silently logging a slot failure and recording zero gauntlet results. Existing tests masked the bug by patching play_match to return raw tuples. Replace the unpack with attribute access, update the four masking mocks to return real MatchOutcome instances, and add TestMatchOutcomeUnpacking with a hand-crafted always-win opponent that asserts the result row is recorded and elo_after > elo_before. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_record_match_result issued record_result, two update_elo calls, and role_elo_tracker.update_from_result as four independent transactions on OpponentStore. A failure between writes left a recorded match pointing at stale Elo, corrupting subsequent ratings silently — and the training loop concurrently wrote chart-continuity rows via carry_forward_elo, so it could observe inconsistent intermediate state. Wrap the four writes in `with self.store.transaction():`. The store's transaction context manager tracks _transaction_depth, so the inner nested transactions inside record_result/update_elo/update_role_elo see depth > 1 and skip commit/rollback — only this outer block decides atomicity. Add TestRecordMatchResultAtomicity: - failure injection on the second update_elo asserts no league_results row, no elo_history rows, no Elo or games_played changes survive - happy-path test sanity-checks all four writes still land Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…i-678359b7aa) SLConfig.__post_init__ validated grad_clip, total_epochs, batch_size, learning_rate, and num_workers but never checked lambda_policy / lambda_value / lambda_score. The trainer applies them directly, so a negative lambda inverts gradient descent into gradient ascent for that loss head while per-head metrics still look positive — silently corrupting SL checkpoints. NaN or inf would poison the combined loss the same way. Add a finite + non-negative check for the three lambdas. Zero is allowed (legitimate disable for ablations), matching the contract already enforced by MultiHeadValueAdapter (RL side) at value_adapter.py:67-70. NOTE: this addresses keisei-678359b7aa (SLConfig validation only). The related keisei-bef32b64a8 (0.0 * NaN = NaN poisoning in MultiHeadValueAdapter when heads are disabled) is a structurally different bug — needs a graph-connected zero helper applied across multiple branches and is intentionally out of scope for this small-fix bundle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies three small, targeted bug fixes identified in the architecture critique: (1) correct handling of MatchOutcome in the historical gauntlet, (2) make tournament match-result persistence atomic to avoid partially-written Elo/result state on failure, and (3) validate SL loss-weight lambdas to prevent negative/NaN/inf values from silently corrupting optimization.
Changes:
- Fix
HistoricalGauntlet.run_gauntlet()to consumeplay_match()’sMatchOutcomereturn type instead of tuple-unpacking. - Wrap
LeagueTournament._record_match_result()’s result + Elo writes in a singleOpponentStore.transaction()for true all-or-nothing persistence. - Add SLConfig validation rejecting negative and non-finite
lambda_*weights, with dedicated regression tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_sl_config.py |
Adds regression coverage ensuring SLConfig rejects negative/non-finite lambdas while preserving existing validation behavior. |
tests/test_league_tournament.py |
Adds rollback/commit regression tests proving _record_match_result is atomic under mid-write failures. |
tests/test_historical_gauntlet.py |
Updates existing gauntlet tests to use real MatchOutcome and adds new regression tests for the unpacking bug. |
keisei/training/tournament.py |
Wraps match result recording + Elo updates (including role Elo) in one store transaction to prevent partial DB state. |
keisei/training/historical_gauntlet.py |
Fixes gauntlet logic to extract wins/losses/draws from MatchOutcome fields. |
keisei/sl/trainer.py |
Adds finite/non-negative validation for SL lambda weights in SLConfig.__post_init__. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Summary
Three independent P1/P1/P1 bug fixes from the architecture critique
(
docs/arch-analysis-2026-05-05-1007/05-architecture-critique.md,weaknesses W5, W1, and the SLConfig half of W11). Each fix is small and
mechanical; each commit is self-contained with regression tests.
keisei-4509042dd1MatchOutcomedataclass — every gauntlet match silently treated as failure089149bkeisei-fa604bad63_record_match_resultwrites match + Elo across four separate transactions — partial state on crashcf52793keisei-678359b7aaSLConfigaccepted negative lambdas — silent gradient ascent on whichever head0d231c3W11a (
keisei-ca5e280cae,flush_timings()not called) was already fixedon main at
katago_loop.py:1618; verified by running the existingtest_torch_compile.py::test_*timing*cases.The deeper NaN-poisoning bug in
MultiHeadValueAdapter(
keisei-bef32b64a8, originally bundled with the SL fix in the critique)is structurally larger — needs a graph-connected zero helper across
multiple branches with NaN-path test coverage — and is intentionally
deferred to its own PR.
Test plan
uv run pytest tests/test_historical_gauntlet.py— 17 pass (15 existing + 2 new)uv run pytest tests/test_league_tournament.py— 46 pass (44 existing + 2 new)uv run pytest tests/test_sl_config.py— 24 pass (new file)uv run pytest tests/test_value_adapter.py tests/test_sl_*.py tests/test_concurrent_round.py— all greenuv run pytest tests/— 1616 passed, 1 skippedNotes
_record_match_resultatomicity fix relies onOpponentStore.transaction()being reentrant via
_transaction_depth— verified atopponent_store.py:436-466.play_matchtoreturn a tuple — it returns a real
MatchOutcomeinstance so theunpacking path is exercised.
sqlite3.OperationalErroron thesecond
update_eloand asserts noleague_resultsrow, noelo_historyrows, and original Elo /games_playedsurvive.🤖 Generated with Claude Code