fix(cli): save the trained model, default to fast, make decode compose - #11
Open
Mirdula18 wants to merge 1 commit into
Open
fix(cli): save the trained model, default to fast, make decode compose#11Mirdula18 wants to merge 1 commit into
Mirdula18 wants to merge 1 commit into
Conversation
Three defects, all in the CLI. No trainer, encoder or pattern code touched; results/ and the README AUTOGEN block are byte-identical. 1. `granule train` trained a tokenizer and threw it away, so the very next Quickstart command died with FileNotFoundError. Adds -o/--output; without it, say so plainly rather than guessing a filename or exiting quietly. 2. The CLI defaulted to the naive trainer (`--fast` was store_true) while Tokenizer.train defaults to fast=True -- a 54x penalty for following the README. Flipped: fast is the default, `--naive` selects the oracle. Safe because the two are asserted to produce identical merge lists. `--fast` is kept as a hidden no-op so existing invocations keep working. 3. encode printed space-separated ids but --decode parsed only commas, so the two halves could not talk to each other. Decode now accepts commas, whitespace and newlines, and `text` takes nargs="+" so xargs can hand the ids back as separate argv words. Non-numeric input gets a one-line error instead of a traceback. Adds Tokenizer.merges (a copy, since _ranks is derived from it) and uses it instead of reaching into tok._merges. Also sets ruff's `src` explicitly. Local ruff 0.5 -- the floor in [dev] -- does not infer the src layout, classifies `granule` as third-party, and flags I001 on 11 files that CI's newer ruff accepts. Being explicit makes `ruff check src tests` agree across the supported range. Tests: 19 new CLI tests plus 5 in test_b7_quickstart, which previously proved only that flags parse -- it appended `--help`, and argparse exits 0 before any handler runs. Verified 22 of them fail against the pre-fix code.
There was a problem hiding this comment.
Pull request overview
This PR fixes three CLI behavior defects so the README Quickstart can run end-to-end: training now persists a model when requested, CLI defaults match the Python API’s fast trainer behavior, and encode/--decode now compose cleanly (including xargs pipelines). It also exposes Tokenizer.merges as a public API to avoid CLI reach-through into tokenizer internals, and strengthens Quickstart-related tests to execute the documented flow rather than only parsing --help.
Changes:
- Add
-o/--outputtogranule trainand print an explicit “not saved” message when omitted. - Make fast training the CLI default (
--naiveselects the reference trainer; legacy--fastremains accepted but hidden). - Make
encodeacceptnargs="+"and improve--decodeparsing to accept commas/whitespace/newlines; expand CLI/Quickstart tests accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_cli.py | New focused regression tests covering train save behavior, trainer default, encode/decode composition, and avoiding private tokenizer access. |
| tests/test_b7_quickstart.py | Strengthens Quickstart validation by parsing pipelines, checking coherence (train output vs encode input), and executing the sequence. |
| src/granule/tokenizer.py | Adds a public merges property returning a copy to prevent external mutation from desyncing derived state. |
| src/granule/cli.py | Implements -o/--output, defaults training to fast, and improves decode parsing + xargs-friendly encode argument handling. |
| README.md | Updates Quickstart commands/documentation to reflect required -o, default fast training, and encode/decode composability. |
| pyproject.toml | Pins ruff src layout explicitly to avoid version-dependent import classification differences. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
154
to
156
| if args.decode: | ||
| ids = [int(x) for x in args.text.split(",")] | ||
| result = tok.decode(ids) | ||
| result = tok.decode(_parse_token_ids(text)) | ||
| print(result) |
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.
Three CLI defects. No trainer, encoder or pre-tokenization code touched —
results/and the README AUTOGEN block are byte-identical.granule trainnever saved anything, so the next Quickstart line died withFileNotFoundError-o/--output; without it, print that nothing was savedTokenizer.traindefaults tofast=True(54x penalty for following the docs)--naivefor the oracle,--fastkept as a hidden no-opencodeprinted space-separated ids,--decodeparsed only commastextisnargs="+"soxargsworksPlus
Tokenizer.merges(returns a copy —_ranksis derived from_merges),replacing the
tok._mergesreach-through incli.py.The Quickstart now runs start to finish
Verified verbatim in a fresh clone:
Why the existing quickstart test didn't catch this
test_quickstart_granule_commands_are_real_subcommandsappends--helptoevery command, and argparse prints help and exits 0 before any handler
runs. It proved the flags parse and nothing else. Strengthened to check
Quickstart coherence (does the file
trainwrites match the oneencodereads? does the corpus path exist?) and to actually execute the sequence,
pipeline included.
22 of the new tests were verified to fail against the pre-fix code.
One thing beyond the brief
ruff check src testswas already failing onmainwith 11I001errors,before my change. Cause is toolchain skew, not the code:
pyproject.tomlnever set ruff's
src, so ruff 0.5 (the floor in[dev]) treatsgranuleasthird-party, while CI's newer ruff infers the src layout and accepts it. Set
src = ["src", "."]explicitly rather than reordering imports in 11 files tosatisfy the older version.
335 passed with
-k "not slow"; ruff and mypy clean.