Skip to content

PR0.5 — make the stack's base green: the tokenizer witness, and #45's unfinished metric-table migration - #49

Merged
can-goodfire merged 2 commits into
can/protocol-refactorfrom
can/metrics-first-token
Aug 28, 2026
Merged

can-goodfire merged 2 commits into
can/protocol-refactorfrom
can/metrics-first-token

Conversation

@can-goodfire

Copy link
Copy Markdown
Collaborator

can/protocol-refactor — the base the Qwen3.6 hookpoint stack (#46 merged, #47, #48) builds on — was red by 17 tests, not the 2 recorded in the plan note. Two unrelated regressions, one commit each. pytest -m "not golden" is now 1740 passed, 8 deselected, 0 failed.

1. The transformers 5.16.1 bump moved a tokenizer witness (2 tests)

#46 floored transformers at 5.16.1. That changed tokenization, which the parity goldens do not cover — 5.16.1 drops sentencepiece's legacy dummy prefix, so on the tiny Llama tokenizer encode(" Thursday") is now [Th, urs, day], not [▁, Th, urs, day].

The question was whether column_first_token_id's "skip the piece that decodes to whitespace" filter is now a no-op to delete, or is silently eating a real content token. Measured: neither — it is load-bearing. The lone still appears whenever the first character has no merged ▁X piece (digits, non-Latin scripts, emoji, ligatures — encode(" 3.14") is still [29871, 29941, 29889, 29896, 29946]), and byte-level BPE still splits a whitespace run off the front (gpt2 " ?"' ' + ' ?'). It fires on 20/25 probe values on tiny Llama, 5/25 on the Qwen MoE fixture, 2/25 on gpt2, and only ever skips a piece whose decode is empty-or-whitespace — which no content piece is.

Verified by mutation: with the filter removed, first_token credits (29871) instead of "3" (29941), and both tests below catch it.

So the code stays and the tests change:

  • the lone-space contract is pinned against a stub tokenizer that cannot drift, because which values trigger it is a released-tokenizer property that just moved. A second test keeps a live witness (" 3.14") and reports "witness moved" rather than "behaviour broke" if it shifts again.
  • the non-fallback contract needs a value whose bare form is one token while its space-prefixed form is several. 5.16.1 leaves no such value on sentencepiece, so the witness moves to gpt2, whose byte-level BPE is byte-identical across the bump ("haus" is [30404], " haus" is [387, 385]).

One finding worth flagging. On a sentencepiece family that has dropped the dummy prefix, " X" and "X" now encode to the same id — so token_form has one row to name there and _ambiguous_under_auto can never fire. The once-per-column warning exists because a punctuation match read a flat 0.000 across all 48 layers of a real gpt2-xl scan; that detector is now structurally dark on sentencepiece. Nothing to fix in the resolver, but it means the punctuation trap is a BPE-family hazard and a document cannot be checked against it by pinning token_form on a sentencepiece model. Recorded as a test plus a note in the module docstring.

Also corrects two docstrings that had inverted into passing for the opposite reason they state, with the reason now pinned so they cannot lie again silently.

2. #45 left its metric-table migration unfinished (15 tests)

Bisected: these pass at f55efa8 (the #46 merge) and fail at 6e467ea (after #45), so they arrived with #45 (workflow-v2), not the bump. #45 tightened §2.12 to "JSON and safetensors are the only two formats" and changed the validator — expected_ext = ".json" if is_metric else ".safetensors", was ".parquet" — but left the call sites declaring .parquet behind.

Nothing in production needed changing, which is the reassuring part: write_outputswrite_table already emits JSON unconditionally, so the declared extension was the only thing out of step. That also means test_run_corpus's two pd.read_parquet read-backs could never have run — pd is not imported in that module — so they move to table_frame, the JSON-table reader the rest of that file already uses.

Corpus digest pins regenerated with update_corpus_digests.py; the diff is confined to 12_probe_variable_im.json (2 lines, its save block changed) and the other 11 documents' pins are byte-identical.

Gates

  • pytest -m "not golden"1740 passed, 8 deselected (the GPU-only golden tier), 0 failed
  • pre-commit run --all-files — passes repo-wide (ruff 0.14.5 pinned)
  • the parity goldens are untouched

Plan note: notes/causalab-hookpoint-vocabulary-plan.md §6.1.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

Test Results

1 739 tests  +2   1 731 ✅ +20   51s ⏱️ -6s
    1 suites ±0       8 💤 ± 0 
    1 files   ±0       0 ❌  - 18 

Results for commit ae0f88c. ± Comparison against base commit 8af9f89.

♻️ This comment has been updated with latest results.

can-goodfire and others added 2 commits August 28, 2026 10:28
…ness

`pytest -m "not golden"` went red on two metrics tests after the transformers
5.16.1 floor landed in #46. Both were pinning a *tokenizer artifact* rather
than a behaviour, and the artifact moved: 5.16.1 drops sentencepiece's legacy
dummy prefix, so on the tiny Llama tokenizer `encode(" Thursday")` is now
[Th, urs, day] instead of [▁, Th, urs, day], and `encode(" Monday")` is the
single ▁Monday piece instead of two.

The semantic question was whether `column_first_token_id`'s "skip the piece
that decodes to whitespace" filter is now a no-op to delete, or is eating a
real content token. Measured under 5.16.1: neither — it is load-bearing. The
lone ▁ still appears whenever the first character has no merged ▁X piece
(digits, non-Latin scripts, emoji, ligatures: `encode(" 3.14")` is still
[29871, 29941, 29889, 29896, 29946]), and byte-level BPE still splits a
whitespace run off the front (gpt2 "  ?" → ' ' + ' ?'). It fires on 20/25
probe values on tiny Llama, 5/25 on the Qwen MoE fixture, 2/25 on gpt2. It
only ever skips a piece whose decode is empty-or-whitespace, which no content
piece is. Removing it makes `first_token` credit ▁ (29871) instead of "3"
(29941) — verified by mutation, both tests below catch it.

So the code stays and the tests change:

- the lone-space contract is pinned against a stub tokenizer that cannot
  drift, since *which values* trigger it is a released-tokenizer property
  that just moved; a second test keeps a live witness (" 3.14") and says
  "witness moved" rather than "behaviour broke" if it ever shifts.
- the non-fallback contract needs a value whose bare form is one token while
  its space-prefixed form is several. 5.16.1 leaves no such value on
  sentencepiece — both forms now encode identically — so the witness moves to
  gpt2, whose byte-level BPE is unchanged across the bump ("haus" is [30404],
  " haus" is [387, 385]).

That collapse is itself worth recording, and is now a test plus a note in the
module docstring: where " X" and "X" name the same id, `token_form` has one
row to name and `_ambiguous_under_auto` can never fire, so the punctuation
trap the warning exists for is a BPE-family hazard and cannot be checked by
pinning `token_form` on a sentencepiece model.

Two docstrings that had inverted into passing for the opposite reason they
claim are corrected, with the reason pinned so they cannot lie again silently.

Refs the plan note §6.1 (cockpit: notes/causalab-hookpoint-vocabulary-plan.md).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rquet

The other 15 failures on `can/protocol-refactor`. #45 (workflow-v2) tightened
§2.12 to "JSON and safetensors are the only two formats" and changed the
validator accordingly — `expected_ext = ".json" if is_metric else
".safetensors"`, was `".parquet"` — but left the call sites that still declare
`.parquet` behind. Bisected: the 9 affected protocol/metrics tests pass at
f55efa8 (the #46 merge) and fail at 6e467ea (after #45), so this arrived with
#45 and is not the transformers bump.

Nothing in production needed changing, which is the reassuring part: the writer
already emits JSON unconditionally (`write_outputs` → `write_table`), so the
extension in `file_path` was the only thing out of step. That also means
`test_run_corpus`'s two `pd.read_parquet` read-backs could never have run —
`pd` is not even imported in that module — so they move to `table_frame`, the
JSON-table reader the rest of that file already uses.

- `tests/protocols/12_probe_variable_im.json`: the two metric saves become
  `.json`; `where.safetensors` is a read and stays.
- the declared paths in test_generate_plan / test_validation_rules /
  test_generate_metrics follow.
- corpus digest pins regenerated with `update_corpus_digests.py`. The diff is
  confined to `12_probe_variable_im.json` (2 lines) — the canonical form of
  that one document changed because its save block did; the other 11
  documents' pins are byte-identical.

`pytest -m "not golden"`: 1740 passed, 8 deselected (the GPU-only golden
tier), 0 failed — the base is green for the first time since #45.
`pre-commit run --all-files` passes repo-wide.

Refs the plan note §6.1/§8 (cockpit: notes/causalab-hookpoint-vocabulary-plan.md).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@can-goodfire
can-goodfire force-pushed the can/metrics-first-token branch from a20c728 to ae0f88c Compare August 28, 2026 09:29
can-goodfire pushed a commit that referenced this pull request Aug 28, 2026
…n't carry

Rebasing onto #49 (PR0.5) dropped this branch's own "green the suite" commit
wholesale: #49 fixed the same two base failures deliberately and better —
the stub tokenizer pins the lone-▁ rule while " 3.14" keeps a live witness,
where this branch's stub replaced the witness entirely. Keeping both copies
was a guaranteed semantic conflict for whichever landed second.

Two things from the dropped commit were not duplicates and come back here:

- `auto` resolving "haus" after `space_prefixed` refuses it. #49's test pins
  the `bare` contrast; `auto` is the form that is actually *allowed* to fall
  back, so asserting it lands on the same row is what makes the refusal a
  pin rather than an accident.
- A windowed-path test for `vocab_axis`. The reduction is covered in the
  prompt frame only, and the generated frame reaches it through
  compute_windowed_metric — the one hop where a non-vocabulary read could
  silently regain its `True` default and start decoding neuron indices as
  token ids. Measured: the same call with vocab_axis=True gains a `tokens`
  column, so the assertion is load-bearing, not decorative. It also pins the
  flatten/cat/split regrouping over rows of 2, 1 and 0 positions.

Digests regenerated with tests/protocol/update_corpus_digests.py rather than
hand-merged; docs 11 and 12 move, and doc 12 lands back on 4e695af5… because
this branch adds `"by": "prob"` to it.

1768 passed. pre-commit green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
can-goodfire pushed a commit that referenced this pull request Aug 28, 2026
Two review notes, both about a reader's next wrong assumption rather than
about behaviour — no code changes.

`to_contract` returns a view for every layout we have, and
`test_an_in_place_edit_through_the_contract_reaches_the_native_tensor` passes
whether or not it does: the edit flows through the returned chain, never back
into `native`. So the suite does not pin aliasing, and correctness does not
need it — the hooks hand `from_contract(contract)` back to the model and
`_address_writer` writes through that. A layout forced to copy would be
correct and merely slower. Recorded in the docstring because the unpinned
direction is the dangerous one: a later change that *relies* on aliasing
would go in green.

`LayoutError` carries no P/V code, and shouldn't: `layout` is set by the site
table on ResolvedSite and is never authorable, so the error can only mean our
table disagrees with a real module. The protocol codes name rules a
*document* broke. Written down with the condition that would flip it — making
layout authorable — so the next reader doesn't re-derive it.

1763 passed (green on this branch for the first time, via the #49 rebase, with
no code change — the two failures were the base's). pre-commit green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
can-goodfire pushed a commit that referenced this pull request Aug 28, 2026
… tables

This branch forked 18 commits back, before the #45 workflow-v2 merge, so its
CI had never run and GitHub called it CONFLICTING. Merging rather than
rebasing: one resolution of 19 files instead of six replays of the same
semantic conflicts.

What the base moved underneath this branch, and how each is resolved:

- **The CLI split in two.** #45 moved document-type dispatch into a new
  top-level `causalab/cli.py`, leaving `protocol/` linked against nothing in
  the workflow layer. So this branch's parser changes move there: `--dtype` is
  now the `--set model.dtype=…` shorthand (precision is a document fact, §2.1)
  and is folded into `args.parsed_set`, and `ensure_model_registered` flattens
  a split document before looking for the model — it lives in the
  `application` half. `_workflow_main` was already gone; its two changes here
  (the `--dtype` refusal, and flattening an inner document before
  pre-registering its model) move to `causalab/workflow/cli.py`. A **method**
  file still dispatches from `protocol/cli.py`: telling one apart needs
  `document_type`, which is that package's.

- **`protocol/workflow.py` became the `causalab/workflow/` package.** The
  flatten-the-inner-document change ports to `workflow/document.py` line for
  line.

- **The step vocabulary changed.** `protocol` → `intervention_protocol`, and
  `select`/`plot`/`transform` became `script`. This branch's `test_workflow.py`
  tested the old types, so the base's file is taken whole and only the genuinely
  new test — a step pointing at a split document — is ported onto its helpers.

- **`causal_model` was dropped from the protocol** (dfe737d) and **metric
  tables became `.json`** (V10, #45). Both apply to this branch's documents,
  including the two the rename hid from git: `configs/protocols/interchange.json`
  (renamed here from `configs/methods/`) and the new
  `configs/runs/weekdays_8b_interchange.json` never received the base's edits,
  because they landed on the old path.

- **`configs/methods/` now means method HALVES**, so the shipped full documents
  are `configs/protocols/`. Every reference follows: the torch-free test, the
  workflow presets, the spec's worked example, `test_isolation.py`.

Digests regenerated with the three update scripts, never hand-merged.

1796 passed. pre-commit green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@can-goodfire
can-goodfire merged commit f6ea4b4 into can/protocol-refactor Aug 28, 2026
4 checks passed
can-goodfire pushed a commit that referenced this pull request Aug 31, 2026
The base's engine rename landed while this branch sat: `Backend` became
`Engine`, `causalab/neural/pytorch_hooks/` moved under `engines/`, and the
per-point execution loop was lifted out of the reference engine into the
engine-neutral `causalab/neural/shared/`. Git saw three modify/delete
conflicts because the two files this branch rewrote no longer exist by those
names, so the interning was re-applied onto the new structure rather than
merged textually.

Where each piece landed, and why:

- `RunResult.forwards` -> `protocol/engine.py` (was `protocol/backend.py`).
- `campaign_plans` / `_data_identity` / `_tap_union` -> `shared/execution.py`,
  not the pytorch_hooks engine: the campaign loop lives there now, and
  interning is a property of the point *set*, so it has to be derived where
  the set is known. `PytorchHooksBackend.campaign_plans` is therefore
  `shared.execution.campaign_plans`.
- `ForwardCache` / `Interning` and the `_group_digest` / `_interned` /
  `_publish` helpers -> `shared/executor_base.py`, keyed on the base's
  `TapKey` rather than the old `(module id, side)` pair. `TapKey` carries
  shape, tuple index, interface slot and expert, so two taps that share a
  module but mean different tensors no longer collide in the store — the old
  key would have.
- `ForwardCache.routing` is new: round 3 added an experts-interface routing
  table beside each capture (`idx_capture`), and a capture replayed without
  the dispatch indices it joins on is not the same value. The two are
  published and served together.
- `_forward_group` extraction -> `engines/pytorch_hooks/executor.py`, over the
  base's much larger group body (attention/experts/DeltaNet interface taps,
  the interior refusals). `_refuse_interior` now runs on this point's own
  sites *before* the cache is consulted, so a cache hit cannot smuggle past a
  refusal, and on the union sites as they are installed.
- Publishing filters unfilled placeholders: the base pre-seeds `capture[key]`
  with `torch.empty(0)` for dedup, and handing a later point an empty capture
  would be worse than letting it run the forward.

`execute_request` gained `intern_forwards`, opted into by the reference engine
only. The nnsight engine's trace executor does not consult the cache, so it
takes the handle and drops it and reports `forwards=0` — "not measured", not a
count nothing took.

Verified rather than assumed: `test_a_shared_forward_group_runs_once` still
fails on the merged tree when `_interned` is stubbed to return None ("8
forwards for a campaign whose plan has 5 distinct groups"), so the 8 -> 5
claim survives the rename intact.

Checks: `pytest -m "not gpu and not slow and not golden"` 2675 passed, 0
failed (the base's own suite is green now that #49 landed). pre-commit clean.
basedpyright: 7 errors across the touched modules against 9 on the base — all
pre-existing, none in the merged code. The golden tier is still a cluster job.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
goodatticus pushed a commit that referenced this pull request Sep 1, 2026
nnsight's LanguageModel refuses any config registered with
AutoModelForImageTextToText, so it cannot load qwen3_5_moe — the text tower of
Qwen3.6-35B-A3B and the target of the hookpoint-vocabulary work — even though
AutoModelForCausalLM resolves to that tower cleanly. ndif-team/nnterp#52 fixes
it and is open for review; this unpins us from that review landing.

45f386b is the upstream pin b4a3127 plus the two commits in #52, so this is a
strict superset of what we had — the packaging fix (#49) that motivated the git
source in the first place is still in there, and the comment now records both
reasons and how each retires.

Verified installed from the fork's public URL rather than a local path:
nnterp 1.3.1.dev16+g45f386b7c loads tiny-random/qwen3.5-moe as
Qwen3_5MoeForCausalLM with no vision tower attached, and traces to logits.

Note this branch still resolves transformers 4.57.1, which carries no
qwen3_5_moe, so the fix ships here but cannot be exercised until a transformers
>= 5.16 bump reaches this lineage. That bump landed downstream on
can/protocol-refactor (#46) and is a separate decision for the path to main.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant