fix(fast): training no longer mutates the global cluster cache#58
Merged
Conversation
Fixes #56. apply_merge_to_cluster_cache turned the cluster-graph memo into trained state: the second training run in a process started from mutated cluster graphs and produced different merges than the first (the reference has no such statefulness — its cluster building is a pure function). The mutation served nothing: no path can rely on merged cached graphs, since the reference would fail it too. Deleted, with its call sites and the now-empty on_merge hook. SuperBPE's full-scale digest changes 2a6baff51d -> 30929f0c51: phase 2 previously built graphs from clusters mutated by phase 1's merges. The new value is proven correct against the reference at 20k rows / 500 merges (both b308cdb768). All other digests unchanged; warm == cold at full scale. New shared test: train each tokenizer twice in one process and assert identical merges (all four failed before the fix under fast). Separately discovered while gating: 'cd fast && pytest ../tests/' never loaded fast/conftest.py under pytest 9, so the fast suite silently tested the REFERENCE. Running it properly (-p conftest, now documented in CLAUDE.md) exposed two unmirrored APIs, both added to the fast shim: GraphSettings.TRADE_MEMORY_FOR_SPEED and Tokenizer(cache_maxsize=...) with the word-graph lru_cache dedup in _build_graphs. Co-Authored-By: Claude Fable 5 <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.
Fixes #56 — and surfaces something bigger found while gating it.
The bug
apply_merge_to_cluster_cachemutated the global cluster-graph memo as training progressed, turning it into trained state: the second training run in a process produced different merges than the first. Repro (now a shared test,test_repeated_training_gives_identical_merges, all four tokenizers): corpuscafé déjà …, 4 merges — cold run mergesé's UTF-8 bytes, warm run finds them pre-merged in the cache and picks a different merge. The reference is stateless and matches fast's cold run.The mutation served nothing: the reference has no cluster cache at all (cluster building is a pure function), so no behavior can legitimately depend on merged cached graphs. Deleted, along with its call sites and the now-empty
on_mergehook. It was also the hidden cause of flakiness potential intest_node_count_with_chinese_ids(Han handler Trees cached and mutated between builds).One digest legitimately changes: SuperBPE full-scale
2a6baff51d→30929f0c51(phase 2 previously built graphs from clusters mutated by phase 1's 250 merges). Proven correct against the reference at 20k rows / 500 merges: pre-fix fastae6475b5ef≠ reference; post-fix fastb308cdb768= referenceb308cdb768. All other digests unchanged; warm == cold now holds at full scale.cd fast && pytest ../tests/never loadsfast/conftest.pyunder pytest 9 (conftests apply to their directory subtree; a cwd conftest is not an initial conftest for sibling paths) —complex_tokenizationresolved to the reference package, so every recent "fast suite passed" validated the reference twice. Digest/benchmark verifications were unaffected (they importcomplex_tokenization_fastdirectly); only the pytest gate was hollow.Running the suite properly (
pytest ../tests/ -p conftest, now documented as REQUIRED in CLAUDE.md) exposed two unmirrored APIs, both added to the fast shim:GraphSettings.TRADE_MEMORY_FOR_SPEED(every aliased test errored in a fixture without it)Tokenizer(cache_maxsize=...)incl. the word-graphlru_cachededup in_build_graphsand kwargs-forwarding subclassesVerification
2af4c7edca✓, BNE967201e51c✓, Boundless10318b5e1c✓, SuperBPE30929f0c51(reference-proven). 2000-row defaults: all four unchanged.🤖 Generated with Claude Code