model: fix tokenization issues with spm tokenizer #10081
Merged
Conversation
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
6eeaebd to
e139256
Compare
Mkhaled35
approved these changes
Apr 2, 2025
jmorganca
commented
Apr 2, 2025
| if id := spm.vocab.Encode(string(left.runes) + string(right.runes)); id < 0 { | ||
| continue | ||
| } | ||
| if string(left.runes) == "" || string(right.runes) == "" || len(string(left.runes))+len(string(right.runes)) != pair.size { |
There was a problem hiding this comment.
This tries to stay as close to the existing implementation as possible, but in a follow-up change .runes could be replaced with a string value instead.
I believe this line was the cause of the breakage since we were comparing len of a rune slice instead of the string length.
pdevine
approved these changes
Apr 2, 2025
| } | ||
| } | ||
|
|
||
| ids = append(ids, result...) |
There was a problem hiding this comment.
you might want to guard here if the result slice is empty because we didn't have the byte tokens in the vocab.
shethaadit
approved these changes
Apr 2, 2025
halfcrazy
pushed a commit
to halfcrazy/ollama
that referenced
this pull request
Jun 19, 2025
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.
This PR fixes inconsistencies in the SPM tokenizer for Gemma 3
Note: while this fixes tokenizing certain utf-8 characters (e.g. certain Korean characters) it doesn't fix de-tokenizing them yet