Clean up MinGram repro: lint, dead code, attribution, no silent errors#6
Merged
Conversation
Post-merge follow-up to #5. No behavior change to the trained tokenizers. - Fix 4 ruff errors that slipped past the PR's validation (unused imports/locals and an f-string without placeholders in the pathpiece / convextok trainers). - Remove the dead token-bias/hybrid-experiment framework from paper_utils/hybrid/train_hybrid.py (TRAIN_CORPUS_SETS, TOKEN_BIAS_VALUES, get_hybrid_experiment_configs, load_model_if_cached, train_hybrid_model, run_experiment); keep the externally-used path/BPE-init helpers and OVERSHOOT_FACTORS. train_model.py is the live training entrypoint. - Remove internal references: /fsx/sander cluster paths in downstream/pretokenize.py, and a commented-out alternate-pretokenizer config in paper_utils/unigram/utils.py. - Attribute the skip_substring batch rule and the ConvexTok length-limited regex to "Craig Schmidt, personal communication" (the rule is explicitly beyond PathPiece's published behaviour) instead of informal "Craig's rule". - No silent errors (per repo convention): let the encode-pool fork failure propagate, raise when a base_eval metric is missing instead of returning None, and stop swallowing ValueError when parsing the per-task CSV. - Point the README MinGram repro section at the paper link instead of "(Soon)". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The length-limited pre-tokenizer regex is published in the ConvexTok paper (Tempus et al., 2026), not a personal communication. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per Craig Schmidt (pers. comm.), skip_substring=1/true is what PathPiece actually does (not 'beyond' it) and has limited effect in practice, so the prior 'default off = paper behaviour' note was wrong. Also clarify that MinGram's 'mi' prune_criterion is the PathPiece-style MI pruning where the flag applies. Co-Authored-By: Claude Opus 4.8 (1M context) <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.
Post-merge follow-up to #5 (review cleanups). No behavior change to the trained tokenizers; verified
ruffclean andpytest tests/tokenizers tests/pretokenize(217 passed).Lint (slipped past #5's validation)
The PR's
ruffinvocation didn't cover thepathpiece/convextoktrainers. Fixed 4 real errors: unuseddefaultdictimport, deadlast_ctc/n_atomiclocals, and anf""with no placeholders.Dead code
Removed the unused token-bias / hybrid-experiment framework from
paper_utils/hybrid/train_hybrid.py(TRAIN_CORPUS_SETS,TOKEN_BIAS_VALUES,get_hybrid_experiment_configs,load_model_if_cached,train_hybrid_model,run_experiment— all referenced only within the file itself). Kept the externally-used helpers (get_model_path,get_bpe_model_path,bpe_to_unigram_tokens,get_or_train_bpe,OVERSHOOT_FACTORS);train_model.pyis the live entrypoint. (~195 lines removed.)Internal references removed
/fsx/sander/...cluster paths indownstream/pretokenize.pyusage docstring → generic/path/to/....paper_utils/unigram/utils.py.Attribution
skip_substringbatch rule is explicitly beyond PathPiece's published behaviour (its own comment notes "default off = paper behaviour"), so informal "Craig's rule" is now "Craig Schmidt, personal communication" (in_mi_prune.py,mingram/trainer.py,pathpiece/trainer.py, plus the CLI help/sweep scripts).No silent errors (repo convention)
tokenizer_adapter.init_encode_pool: droppedexcept Exception: _POOL = None— a fork-pool failure now propagates instead of silently degrading ~90-worker encode to serial.runner._search_float: raises when abase_evalmetric isn't found instead of returningNone.runner._read_core_csv: stopped swallowingValueErroron per-task row parsing.Docs
README MinGram repro section points at the paper link instead of "(Soon)" (consistent with the top-of-README link).
🤖 Generated with Claude Code