feat(magic): support per-token per-query MAGIC - #413
Merged
luciaquirke merged 5 commits intoAug 6, 2026
Merged
Conversation
luciaquirke
force-pushed
the
fix/magic-reject-per-token-per-query
branch
from
August 6, 2026 09:13
f32f1b2 to
3f0538a
Compare
luciaquirke
force-pushed
the
fix/magic-reject-per-token-per-query
branch
from
August 6, 2026 09:55
3f0538a to
012e8e2
Compare
attribute_tokens=True with query_method="none" produced an unusable score
tensor. Per-token weights are [rows, seq_len] and the per-query stack used
dim=1, giving [rows, num_queries, seq_len] — query axis in the middle,
which nothing downstream reads. validate_scores takes shape[-1] as the
query count, so it compared seq_len against the query document count and
died naming the wrong dimension:
ValueError: scores has 8 query columns but the query dataset has 2
documents
on a run with 2 queries and seq_len 8.
Stack on dim=-1 instead. The query axis then comes last in both modes —
[rows, num_queries] per-doc, [rows, seq_len, num_queries] per-token —
matching the layout Scores.to_grid already produces for multi-query token
score directories, which load_attribution_scores already flags multi_query.
validate_scores needed no change: shape[-1] is the query count and
reshape(-1, num_queries) flattens the leading axes into leave-out units,
documents or token positions as appropriate. dim=-1 is identical to dim=1
for 1-D inputs, so per-doc per-query scores are unchanged.
Fix the padding trim in the same path, which applied weight_pad_count
regardless of rank while the main scoring path picks by rank. The two
differ once doc_ids are present (pad rows route to one synthetic doc id),
so a 5-doc dataset at batch_size 4 kept 7 of its 8 padded rows instead of
trimming to 5, leaving pad rows in the saved scores.
Teach both .pt classifiers about 3-D: scores_are_per_token so a reloaded
run sizes its weights per-token, and _pt_scores_are_per_query so it is
recognised as multi-query. 3-D needs no config lookup to disambiguate,
unlike 2-D.
The aggregation test is the numerical gate: per-token per-query scores
summed over each document's tokens reproduce the per-doc per-query run.
Both new end-to-end tests fail on the parent commit with shape (7, 2, 8)
against the expected (5, 8, 2) — wrong axis order and untrimmed padding
together.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
luciaquirke
force-pushed
the
fix/magic-reject-per-token-per-query
branch
3 times, most recently
from
August 6, 2026 10:21
9527dbc to
0162a22
Compare
Score layout was inferred from tensor rank in several places, which is guesswork: a 2-D .pt is [docs, seq_len] or [docs, queries] depending only on how the run was configured. #407 established the fix for one of those call sites — read the config.yaml that save_run_config writes next to scores.pt — but scores_are_per_token was left sniffing shapes, and the parsing lived inline rather than beside the other config readers. Add read_first_step_config to config_io, next to read_config and load_subconfig, and route both classifiers through it. The flags say everything the rank could: query_method attribute_tokens layout none yes [docs, seq_len, queries] none no [docs, queries] mean/sum yes [docs, seq_len] mean/sum no [docs] so a run is per-query iff query_method is none, whatever rank results, and per-token iff attribute_tokens is set. Neither needs the shape. Score directories keep reading info.json, and load_attribution_scores now takes the query count from the store's num_scores rather than re-deriving it from the grid it just built. Shape survives in exactly one place: a .pt with no config beside it, where nothing else is knowable and only rank 3 is unambiguous. cfg_attributes_tokens reads attribute_tokens with the deprecated per_token as an alias, in one place, so a run written with the current field name is no longer missed. test_load_attribution_scores_pt_per_query asserted that a 2-D tensor whose config said query_method: none and per_token: true was single-query. No run produces that pair — attributing tokens per query yields rank 3 — so the case was describing an unreachable artifact and pinning the shape-derived answer for it. Repointed at the 3-D tensor such a run does produce. Drop six tests that asserted torch's own view()/reshape() indexing semantics. They called no bergson code, so they could not fail unless PyTorch itself changed, and the behaviour they stood in for is covered end to end by the per-query aggregation test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
save_sequence_scores delegates to MemmapSequenceScoreWriter, but its token counterpart reimplemented MemmapTokenScoreWriter inline: the memmap creation, offsets.npy, and an info.json payload identical field for field. So the on-disk token score format was written from two places that had to be kept in step by hand. That matters more now that scores_are_per_token reads info.json["attribute_tokens"] as authoritative: a drift between the two writers stops being a cosmetic inconsistency and becomes a misclassification. Delegating needs the writer to accept what it actually uses. It only ever took a Dataset to call compute_num_token_grads on it, while save_token_scores already holds the offsets those counts came from, so __init__ now takes num_token_grads and a from_dataset classmethod covers the callers that hold a dataset. Also gives the token writer the overwrite flag its sequence twin already had, which delegation needs: save_token_scores wrote with mode="w+" unconditionally, and without overwrite the writer would silently reuse a stale scores.bin instead of replacing it. Net 19 lines out of score_writer.py, and one place left that knows the format. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
luciaquirke
force-pushed
the
fix/magic-reject-per-token-per-query
branch
from
August 6, 2026 10:36
766a9b7 to
239a3f9
Compare
bergson never writes a .npy score file — every writer emits a score directory — so .npy was an ingest-only path for arrays produced outside the scoring pipeline, and nothing in the repo feeds one: no config sets scores: to a .npy, and the examples that save scores.npy read it straight back with np.load rather than through load_attribution_scores. Remove the branch from load_attribution_scores, scores_are_per_token and worker's score-path dispatch, and with it ArrayScores, which existed only to give a bare array the Scores interface. It also carried its own rules. A .npy could not have a score_cfg, so it alone skipped the higher_is_better negation and had to be supplied in the loss-diff convention already; and it was the one input whose multi-query flag came from a raw column count. Score directories record num_scores in info.json, so the surviving formats all describe themselves. The bank-loss-cache tests used .npy as a convenient way to hand a score matrix to evaluate_retrained. They now write a score directory via save_sequence_scores, which is what a caller would reach for, and the multi_query parametrization still passes both ways. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cfg_attributes_tokens had one caller left once _pt_scores_are_per_query was inlined, and reaching across from magic.cli into validate for a two key dict lookup bought nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Opus slop, ignore:
Problem
attribute_tokens=Truewithquery_method="none"produced an unusable score tensor.Per-token weights are
[rows, seq_len]and the per-query stack useddim=1, giving[rows, num_queries, seq_len]— query axis in the middle, which nothing downstream reads.validate_scorestakesshape[-1]as the query count, so it comparedseq_lenagainst the query document count and died naming the wrong dimension:on a run with 2 queries and
seq_len8.Fix
Stack on
dim=-1. The query axis then comes last in both modes —[rows, num_queries]per-doc,[rows, seq_len, num_queries]per-token — matching the layoutScores.to_gridalready produces for multi-query token score directories, whichload_attribution_scoresalready flagsmulti_query.validate_scoresneeded no change:shape[-1]is the query count andreshape(-1, num_queries)flattens the leading axes into leave-out units — documents in 2-D, token positions in 3-D.tests/test_per_token_lds.pyalready asserts that expression against a[docs, seq_len, queries]grid.dim=-1is identical todim=1for 1-D inputs, so per-doc per-query scores are unchanged.Fix the padding trim in the same path, which applied
weight_pad_countregardless of rank while the main scoring path picks by rank. The two differ oncedoc_idsare present (pad rows route to one synthetic doc id):so the run kept 7 of its 8 padded rows instead of trimming to 5, leaving pad rows in the saved scores.
Teach both
.ptclassifiers about 3-D —scores_are_per_tokenso a reloaded run sizes its weights per-token, and_pt_scores_are_per_queryso it is recognised as multi-query. 3-D needs no config lookup to disambiguate, unlike 2-D.Second commit: format from the config, not the shape
9527dbcis a separate commit, revertable on its own.Layout was inferred from tensor rank in several places, which is guesswork — a 2-D
.ptis[docs, seq_len]or[docs, queries]depending only on how the run was configured. #407 established the fix for one call site (read theconfig.yamlbesidescores.pt), butscores_are_per_tokenwas left sniffing shapes and the parsing lived inline rather than beside the other config readers.Adds
read_first_step_configtoconfig_io, next toread_configandload_subconfig, and routes both classifiers through it. The flags say everything the rank could:query_methodattribute_tokensnone[docs, seq_len, queries]none[docs, queries][docs, seq_len][docs]So a run is per-query iff
query_methodisnone, whatever rank results, and per-token iffattribute_tokensis set. Score directories keep readinginfo.json, andload_attribution_scoresnow takes the query count from the store'snum_scoresinstead of re-deriving it from the grid it just built.Shape survives in exactly one place: a
.ptwith no config beside it, where nothing else is knowable and only rank 3 is unambiguous.cfg_attributes_tokensreadsattribute_tokenswith the deprecatedper_tokenas an alias, in one place, so a run written with the current field name is no longer missed.Test cleanup
Dropped six tests that asserted torch's own
view()/reshape()indexing semantics. They called no bergson code, so they could not fail unless PyTorch itself changed; the behaviour they stood in for is covered end to end by the per-query aggregation test.One existing test changed
test_load_attribution_scores_pt_per_queryasserted that a 2-D tensor whose config saidquery_method: noneandper_token: truewas single-query. No run produces that pair — attributing tokens per query yields rank 3 — so the case was describing an unreachable artifact and pinning the shape-derived answer for it. Repointed at the 3-D tensor such a run does produce.Net change in
bergson/: +19 / −17, four of them functional.Third commit: one writer for the token score format
766a9b7is also revertable on its own.save_sequence_scoresdelegates toMemmapSequenceScoreWriter, but its token counterpart reimplementedMemmapTokenScoreWriterinline — the memmap creation,offsets.npy, and aninfo.jsonpayload identical field for field. The on-disk token score format was written from two places that had to be kept in step by hand.That matters more after the commit above:
scores_are_per_tokennow readsinfo.json["attribute_tokens"]as authoritative, so drift between the two writers stops being cosmetic and becomes a misclassification.Delegating needs the writer to accept what it actually uses. It only ever took a
Datasetin order to callcompute_num_token_gradson it, whilesave_token_scoresalready holds the offsets those counts came from — so__init__now takesnum_token_grads, and afrom_datasetclassmethod covers the four callers that hold a dataset.It also gives the token writer the
overwriteflag its sequence twin already had, which delegation requires:save_token_scoreswrote withmode="w+"unconditionally, and withoutoverwritethe writer would silently reuse a stalescores.binrather than replacing it.Net 19 lines out of
score_writer.py, and one place left that knows the format.Fourth commit: drop
.npyscore support12f0d1f, also revertable on its own.bergson never writes a
.npyscore file — every writer emits a score directory — so.npywas an ingest-only path for arrays produced outside the pipeline, and nothing in the repo feeds one: no config setsscores:to a.npy, and the examples that savescores.npyread it straight back withnp.loadrather than throughload_attribution_scores.Removes the branch from
load_attribution_scores,scores_are_per_tokenandworker's score-path dispatch, and with itArrayScores, which existed only to give a bare array theScoresinterface.It also carried its own rules. A
.npycould not have ascore_cfg, so it alone skipped thehigher_is_betternegation and had to arrive in the loss-diff convention already; and it was the one input whose multi-query flag came from a raw column count. Score directories recordnum_scoresininfo.json, so the surviving formats all describe themselves.The bank-loss-cache tests used
.npyto hand a score matrix toevaluate_retrained; they now write a score directory viasave_sequence_scores, and themulti_queryparametrization still passes both ways.Net −49 lines.
Verification
End-to-end on CPU, 5 docs / 2 queries /
seq_len8,attribute_tokens=True,query_method="none":and the leave-subset-out validation that previously raised now completes:
(Correlations are meaningless at 3 subsets on a randomly-initialised model — the point is the machinery runs.)
Testing
test_per_query_per_token_aggregates_to_per_docis the numerical gate: per-token per-query scores summed over each document's tokens reproduce the per-doc per-query run to 1e-5.expected (5, 8, 2), got (7, 2, 8)— wrong axis order and untrimmed padding together.test_three_dim_scores_load_as_per_token_multi_querycovers the reload path.pre-commitclean.🤖 Generated with Claude Code