Fix audit findings B1-B9 - #1
Merged
Merged
Conversation
split_corpus called Path(corpus).is_file() unguarded. os.stat() raises OSError ENAMETOOLONG on Linux/macOS for any string over 255 bytes instead of returning False, so training on a literal string -- the main documented entry point -- crashed everywhere except Windows, which swallows the errno. Route the check through _looks_like_existing_file(), which answers False on OSError and ValueError (embedded null bytes) rather than propagating. Also unblock the type checker: `mypy src` was aborting on "source file found twice" before checking anything, which hid two real errors (untyped re.Pattern generics, str passed where PatternName was expected).
run_all() defaulted to pattern="none", which collapses the entire corpus into a single pre-token. The fast trainer's deduplication step then has nothing to collapse, so it pays heap overhead for no benefit and measures slower than naive -- an artifact that was written into the README as a permanent limitation. - default run_all(pattern=) to "gpt4"; add --pattern to `granule bench` - add a derived Speedup column (naive/fast) to the table and results.json - drop the "Fast trainer overhead" bullet from README Limitations - add scaling tests asserting fast beats naive above 10 KB and that the advantage widens with corpus size
The repo advertises "benchmarked against tiktoken on compression and speed", but nothing in metrics.py ever encoded a corpus with cl100k_base or o200k_base. The only tiktoken contact was _vocab_overlap, which compares vocabulary set membership -- a different measurement entirely. - baseline_metrics() computes chars/token, bytes/token and median encode throughput for cl100k_base and o200k_base per corpus - carried on Metrics, written to results.json, rendered as table columns - encode_slowdown_vs_cl100k derives the throughput ratio from measurements - encodings are loaded once and cached; any failure (missing package, no network for the first vocab fetch) yields partial results, never a crash - write_report() takes an explicit readme= target so tests writing to a tmpdir cannot splice into the repo's README
.gitignore hid results/, so the README's claim that its table was "regenerated with granule bench --report" could not be checked against anything -- and SPEC.md M5 requires the directory to be committed. - un-ignore results/ and commit results.json, results.md and the plot - normalise the AUTOGEN splice so re-running --report with an unchanged table produces no diff, with tests covering both the splice and the full write_report() path
Limitations asserted the encoder was "~50x slower than tiktoken's Rust-backed implementation". No tiktoken throughput was measured anywhere in the codebase, which violates AGENTS.md rule 3. Now generated: an "Encode throughput vs tiktoken" table in the AUTOGEN block reports granule, cl100k_base and o200k_base MB/s per corpus plus the derived slowdown, and the prose points at that column instead of quoting a figure. The real ratio is corpus-dependent and nowhere near the guess. A test rejects any "<n>x slower/faster" claim in README prose outside the AUTOGEN block.
…[B6] corpora.py has held Tamil, Hindi, Arabic, Python and JSON lines all along, but the published table showed three English rows. The tell was visible in the README: c/t equalled b/t in every row, an identity that only holds for pure ASCII. Adds a "Compression by language" table covering all corpora, with UTF-8 inflation (bytes/char) and a derived "cl100k cost vs English" multiplier -- English chars/token over this corpus's, i.e. how many times more tokens the same text costs in this script. Tamil currently measures 7.1x.
…e [B7] Quickstart opened with `pip install granule`, which fails -- the package is not published under that name. Replaced with the editable install from a clone, and added tests that reject a PyPI install line and check every `granule <cmd>` shown in the quickstart is a real subcommand.
…rscores
Prerequisite for B8. SPEC.md "Encoding" specifies: split with the pattern,
encode each pre-token independently, lru_cache on pre-token -> ids. None of
that was implemented -- encode() applied all V merges linearly over the whole
byte stream, ignoring pre-tokenization entirely. That is O(vocab x bytes):
one pass over a 1.5 MB corpus at vocab 8192 took ~7 minutes, which made
benchmarking at realistic corpus and vocab sizes impossible.
Honouring the split surfaced two latent defects, both fixed here:
- `[^\s\p{L}\p{N}]` was translated to `[^\w\s]`. `\w` includes `_`, and `_`
is not a letter, so underscores matched no branch of either pattern and
were silently deleted from every corpus -- catastrophic for source code.
Corrected to `(?:[^\w\s]|_)`; upstream pattern strings re-checked against
the installed tiktoken_ext/openai_public.py rather than recalled.
- split() now emits unmatched spans as their own pre-tokens, so the split is
lossless by construction for any pattern and any input. A gap in a
hand-translated regex must never become a hole in decode(encode(s)) == s.
Also replaces the O(V) linear scan in _token_id_for_merge with a rank dict.
Measured on data/python.txt at vocab 4096: encode 0.07 -> 3.25 MB/s (~46x),
round-trip verified over the full corpus.
Corpora were 500 B - 1.7 KB inline string literals benchmarked at vocab 306/456/756. Production tokenizers use 8k-100k vocabularies, so compression at 306 tokens measured almost nothing. - data/ now holds ~1.5 MB per corpus with source, license, SHA256, byte and char counts, and full attribution in MANIFEST.json - scripts/build_corpora.py is the reproducible builder and provenance record - corpora.py loads lazily and verifies the hash on every read, so a silently rewritten corpus fails loudly instead of changing the numbers - default vocab sizes are now 4096/8192/16384 - the tiny literals live on as fast test fixtures in tests/fixtures/ Licenses verified before use, not assumed: - Wikipedia (english, tamil, hindi, arabic): CC BY-SA 4.0, article URLs recorded per corpus to satisfy attribution - python: CPython v3.12.0 stdlib, PSF-2.0, per-file URLs recorded - jsonlines: generated from a fixed seed, original work, MIT The naive trainer is O(merges x symbols), so timing it on 1.5 MB at vocab 16384 would take hours. Both trainers are now timed on the same 32 KB prefix and the table says so; compression and throughput still use the full corpus. Since the fast trainer's advantage grows with corpus size, this understates the speedup rather than flattering it.
- Tokenizer.train(text, vocab_size=100) silently returned a 256-token vocabulary. _reserve_special now raises ValueError when vocab_size is at or below 256 + len(special_tokens), i.e. when no merge can be learned. - _cpu_model() used platform.processor(), which is empty on most Linux systems, so results recorded anywhere but a Windows/macOS machine had no CPU attribution -- and AGENTS.md calls throughput without a recorded CPU noise. Falls back to /proc/cpuinfo, then to platform.machine(). - _vocab_overlap called decode_single_token_bytes across the full 100k cl100k_base vocabulary on every benchmark row. The result never changes, so it is built once at module scope via lru_cache.
…table Rewrites the README so the multilingual compression comparison is the first thing after the title -- it is the finding this project exists to produce. Results regenerated with `granule bench --report` over the ~1.5 MB corpora at vocab 4096/8192/16384, now that B1-B9 make that possible. Also corrects the throughput measurement: _measure_throughput warmed the pre-token cache and then timed it, so it measured dictionary lookups (508k hits vs 28k misses on english.txt) against a baseline that does the work every time. SPEC.md asks for warm-cache runs to be excluded, so each timed run now starts from a cleared cache. English: 2.63 -> 1.96 MB/s. Stale Limitations bullets replaced with what is actually true now: the bounded naive-trainer timing window, the corpus scale, and why fertility reads in the tens for compact JSON.
Verifying B9 on Linux showed the fallback never firing: platform.processor() returned "x86_64" -- non-empty, so it short-circuited, but an architecture string is not CPU attribution any more than an empty one is. The audit's premise (empty on most Linux) holds widely but not everywhere. Check /proc/cpuinfo first, and treat processor() == machine() as uninformative. Linux now records "AMD Ryzen 5 4600H with Radeon Graphics" instead of "x86_64"; Windows is unchanged.
CI installs only ".[dev]", so tiktoken and matplotlib are absent when `mypy src` runs and it failed with import-not-found on both 3.11 and 3.12. That absence is the supported configuration -- they are benchmark-only deps in the [bench] extra, and the code already degrades to "no baseline" and "no plots" when the import fails at runtime. Locally both were installed, which is why this only showed up in CI.
Mirdula18
commented
Jul 31, 2026
Mirdula18
left a comment
Mirdula18
left a comment
Owner
Author
There was a problem hiding this comment.
Reviewed the PR thoroughly. The changes are clean, and everything is functioning as expected. No further comments. Approving.
Mirdula18
commented
Jul 31, 2026
Mirdula18
left a comment
Mirdula18
left a comment
Owner
Author
There was a problem hiding this comment.
Reviewed all 34 files. The changes look good, and everything is working as expected. No issues found from my side. Approved.
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
2 participants
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 all nine findings from the external audit, one commit per bug.
split_corpusno longerstat()s literal corpus text —OSError/ValueErroranswer False instead of propagating. Reproduced on Linux (Errno 36) before and after.run_alldefaults topattern="gpt4";--patternflag added; derived Speedup column; "Fast trainer overhead" bullet deleted.baseline_metrics()measures chars/token, bytes/token and encode throughput forcl100k_baseando200k_base, degrading to partial results if tiktoken is unavailable.results/un-ignored and committed; the AUTOGEN splice is idempotent.cl100k cost vs Englishmultiplier.data/with SHA256 + attribution; vocab 4096/8192/16384.ValueError;/proc/cpuinfoCPU attribution; reference vocab built once.Two things worth reviewing closely
B8 was blocked by the encoder.
Tokenizer.encodenever implemented theSPEC.md "Encoding" contract — it applied all V merges over the whole byte
stream, O(vocab x bytes), making a production-scale benchmark a ~45-hour job.
Implementing the spec'd pre-token split + per-instance cache took encode from
0.07 to ~3 MB/s.
That surfaced silent data loss.
[^\s\p{L}\p{N}]was translated to[^\w\s];\wincludes_and_is not a letter, so underscores matchedno branch of either pattern and were being deleted from every corpus. Fixed,
and
split()is now lossless by construction so a regex gap can never becomea hole in
decode(encode(s)) == s.Also corrected: throughput was timed with a warm pre-token cache (508k hits vs
28k misses), measuring dict lookups against a baseline that does the work every
time. SPEC.md asks for warm-cache runs to be excluded, so timed runs now start
cold.
Verification
ruff check src testsandmypy src(strict) cleantraining works, underscore round-trip holds, vocab floor raises, CPU model
reads
AMD Ryzen 5 4600Hinstead ofx86_64