fix: lossless merge strings via UTF-8 with byte escapes#63
Merged
Conversation
Fixes #62. Merges crossed the string boundary through decode(errors='replace'): any non-UTF-8-aligned token (byte-level BPE constantly learns these, e.g. the first two bytes of an en-dash) collapsed to U+FFFD, so get_merges -> add_merges silently dropped it — SuperBPE's phase 2 systematically started from a weaker state than phase 1 reached. bytes_to_str/str_to_bytes make the textual form bijective: valid UTF-8 reads as itself (Hebrew, CJK, IDS composition all unchanged), literal backslashes are doubled, and undecodable bytes become \xNN escapes. GraphVertex.__str__ uses it, so display, dot labels, and get_merges agree; make_trainer inverts it exactly. Blast radius is exactly the bug: all 145 existing tests pass unchanged and 2000-row digests are identical (no invalid-byte merges at that scale); deeper training produces new (correct) digests. New shared tests: codec round-trip, and get_merges -> add_merges reproducing the exact trainer graph on a corpus with a non-UTF8-aligned merge (fails on main). Approaches rejected: latin-1 (bijective in 2 lines but cannot live in __str__ without breaking IDS lookup, and renders all non-ASCII as mojibake) and bytes-typed get_merges/add_merges (public API break, churns every string-based test). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bytes_to_str in graph.rs is a hand-rolled UTF-8 validator walk mirroring CPython's decoder byte-for-byte (maximal-valid-subpart error ranges, per-lead-byte continuation windows, no surrogates/overlongs), with literal backslashes doubled and each invalid byte escaped as lowercase \xNN; str_to_bytes inverts with the reference's alternation order. to_str_repr uses it (IDS lookup order unchanged); the shim re-exports both from graph.py so the shared tests resolve under aliasing, and both make_trainer variants decode incoming merge strings with str_to_bytes. Fuzzed against the Python codec on 7k+ cases (random bytes, escape-heavy soup, truncated/overlong/surrogate sequences): zero mismatches, zero round-trip failures. Cross-implementation digests match under the new semantics at 2000-row/100 (unchanged values) and 20k/500 (new values, all four cases). Full-scale digests re-canonized in the PR. No perf regression (boundary-only); SuperBPE at 5000 merges is 1.3s faster — with no merges silently dropped, phase 2 starts from the true, more-merged phase-1 state and does less work. 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
1 participant
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 #62. Both implementations — the reference commit fixes
decode(errors='replace')in Python; the second commit fixes the identical lossiness in fast (from_utf8_lossyinto_str_repr+ plain-UTF-8 re-encode in the shim'smake_trainer).The bug
Merges crossed the string boundary lossily: any non-UTF-8-aligned token (byte-level BPE constantly learns these — e.g.
(0xE2, 0x80), the first two bytes of an en-dash) collapsed to U+FFFD, soget_merges → add_mergessilently dropped it. SuperBPE's phase 2 systematically started from a weaker state than phase 1 reached; persisted merges lost the same information.The fix: bijective text form
bytes_to_str/str_to_bytes: valid UTF-8 reads as itself (Hebrew staysשלום,éstaysé, IDS composition untouched); literal backslashes doubled; only undecodable bytes become\xNNescapes. Python: ~20 lines ingraph.py, wired into__str__/make_trainer. Rust: a hand-rolled UTF-8 validator walk mirroring CPython's decoder byte-for-byte (maximal-valid-subpart error ranges, no surrogates/overlongs), fuzzed against the Python codec on 7k+ adversarial cases: zero mismatches, zero round-trip failures (independently re-fuzzed with a different seed at review: same).Rejected alternatives (tried per the directive): latin-1 (2 lines but can't live in
__str__without breaking IDS lookup; mojibakes all non-ASCII) and bytes-typed API (public break, churns every string test).Blast radius = exactly the bug
get_merges → add_mergesreproducing the exact trainer graph on an en-dash corpus — fails on main, passes here, both implementations.4db42268f7/ed61f906c7/6606ff5e40/0c38860691). New full-scale fast canon: 500-mergef8daaf401b/bd30dd6919/22d80e8f61/15916b03c7; 5000-merge89f35c96c8/3abc2f54e4/41135ef599/3d82241012.Performance
Boundary-only: no regression anywhere — and SuperBPE at 5000 merges got 1.3s faster (8.25→6.95s): with no merges silently dropped, phase 2 starts from the true, more-merged phase-1 state and does less work. The bug was costing time as well as correctness.
Status
🤖 Generated with Claude Code