Add knap, a byte level BPE tokenizer in pure Mojo - #352
Conversation
Knap 1.0.0. All seven tiktoken encodings, byte identical over a 110 MB corpus and 20 million fuzzed inputs, with no Python interpreter at run time. The recipe installs a precompiled Mojo package, a command line tool and shell completions. Built and its test section run locally from this path. The run requirement pins the compiler exactly rather than using pin_compatible, because a precompiled Mojo artefact is tied to the compiler that produced it and a range would be a promise it cannot keep. The reasoning is in the recipe.
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate recipe validation issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Knap, a pure Mojo byte-level BPE tokenizer, as a reproducible conda package with its library, CLI, completions, and smoke tests.
Changes:
- Pins Knap source and Mojo compiler compatibility.
- Builds and installs the library, CLI, and shell completions.
- Adds installation and decoding validation.
File summaries
| File | Summary | Findings |
|---|---|---|
recipes/knap/smoke.mojo |
Verifies installed-library import and decoding. | None. |
recipes/knap/recipe.yaml |
Defines the build, dependencies, metadata, and package tests. | Two moderate findings: validate knap version against ${{ version }} (2 votes); remove -I so default library discovery is tested (1 vote). |
Review details
Suppressed comments (1)
recipes/knap/recipe.yaml:115
- Because this invocation supplies
-I, the smoke test can pass even if the installed.mojocis not discoverable through the package's promised default${PREFIX}/lib/mojopath. The build comment immediately above says that path needs no include flag, and other precompiled-package smoke tests in this repository compile without one; remove the override so this acceptance test covers the advertised install behavior.
mojo run -I "${PREFIX}/lib/mojo" smoke.mojo
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two findings from the Copilot review, both correct. The test ran knap version and discarded the output, so a package built from the wrong revision would have passed it. It now compares that output against the version the recipe declares. The smoke test passed -I PREFIX/lib/mojo while the build script comment claims that path needs no include flag. The flag made the assertion vacuous. Removed, and verified: mojo run smoke.mojo resolves the package with no include path, so the promise was true and is now proven rather than stated.
|
Both findings were right, and both are now fixed in b4df4a8.
test "$("${PREFIX}/bin/knap" version)" = "${{ version }}"The I checked rather than assumed that default discovery actually works, since if it did not, the right fix would have been to correct the claim instead. Building the recipe from So the promise held and is now proven rather than stated. The remaining deviation from the house style is the exact compiler pin in |
Adds
recipes/knap/, a recipe and its test file for Knap, a byte level BPE tokenizer written in pure Mojo.What the package contains
$PREFIX/lib/mojo/knap.mojoc$PREFIX/bin/knapknap countrather than a library you have to write a program against$PREFIX/share/{bash-completion,zsh/site-functions,fish/vendor_completions.d}/No vocabulary is bundled. Vocabulary files are third party data under their own terms, and bundling them would attach a licence question to this package that fetching them does not.
What it does
All seven
tiktokenencodings:cl100k_base,o200k_base,o200k_harmony,p50k_base,p50k_edit,r50k_baseandgpt2. Byte identical withtiktokenover a 110 MB corpus, 191762320 tokens, plus 20 million differentially fuzzed inputs with zero divergences. No Python interpreter at run time.Verification
Built locally with
rattler-buildfrom this recipe at this path, and the test section run against the installed artefact:knap-1.0.0-hb0f4dca_0.conda, 341 KiB, completions present,knap versionprinting1.0.0, andmojo run -I $PREFIX/lib/mojo smoke.mojoimporting the package and decoding through it.One deliberate deviation, and why
The run requirement is
mojo-compiler ==1.0.0rather thanpin_compatible('mojo-compiler').pin_compatiblederives a range from whatever resolved at build time, and a range is the one thing a precompiled Mojo artefact cannot honour: the compiler's documentation says a.mojocfile may not be compatible with another compiler version, and the ABI is explicitly not stable. An exact pin turns that into a solver error at install time instead of a link error deep in a consumer's build. The reasoning is written into the recipe rather than left in this thread, so it survives.Happy to change it if the channel prefers consistency here.
Maintenance
The recipe is maintained in the Knap repository at
conda.recipe/recipe.yamland copied here. A gate there checks on every push that the declared version agrees withCITATION.cff, that the compiler pin agrees withpixi.tomlandpyproject.toml, thatsource.revis a commit in that repository, and that every path the build script reads exists at that commit. That last check was added after the recipe pinned a commit older than its own build script and the package build died on a file that had been added months later.