fix(tokenizer): support SentencePiece artifacts in hash_tokenizer_config and add unit tests - #184
Open
rishiiicreates wants to merge 1 commit into
Conversation
…fig and add unit tests - Support both SentencePiece (spm.model, spm.vocab) and BPE (vocab.json, merges.txt) in hash_tokenizer_config - Auto-detect tokenizer type based on artifacts present on disk - Add unit tests for SentencePiece training, file existence, determinism, and artifact config hashing - Add unit tests for create_tokenizer factory covering BPE, SentencePiece, case-insensitivity, and error cases - Add unit tests for load_merkle_proof covering valid JSON, missing file, and malformed JSON Closes AOSSIE-Org#69 Closes AOSSIE-Org#77
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 43 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
closes #69
closes #77
hash_tokenizer_config() in Legacy/openverifiablellm/tokenizer/train.py previously assumed BPE tokenizer artifacts only (vocab.json and merges.txt). when training a SentencePiece tokenizer, it crashed with FileNotFoundError because SentencePiece writes spm.model and spm.vocab instead of json/txt files. furthermore, unit tests were missing for create_tokenizer and direct load_merkle_proof validation.
this PR updates hash_tokenizer_config() to support both SentencePiece and BPE tokenizers with automatic disk artifact detection and optional explicit type passing. for SentencePiece artifacts, it hashes spm.vocab and spm.model, computes the actual vocabulary size from the vocabulary lines, and sets tokenizer_merges_hash to None while maintaining full backward compatibility for BPE output keys.
also added comprehensive unit tests in Legacy/tests/test_tokenizer.py covering SentencePiece file generation, determinism, config hashing, and error paths for missing models or vocabs. added create_tokenizer factory tests covering BPE, SentencePiece, case-insensitivity, and unsupported types, as well as load_merkle_proof tests in Legacy/tests/test_util.py for valid JSON, missing files, and invalid formats. all 48 tests passing green.