Repository navigation
Fix/issue 472 array merges - #480
michalharakal merged 4 commits into
Conversation
michalharakal
left a comment
There was a problem hiding this comment.
Thanks for picking this up so quickly, @Sehul07 — and for the walk-the-array approach: branching on " vs [ inside the merges block is exactly the right shape, and it keeps the string form working.
A few things before this can go in:
1. It does not compile yet. findStringEnd and parseStringArray are called at lines 178 and 195, but neither is defined in GGUFTokenizer.kt. They exist only as private members of TokenizerJsonParser in HuggingFaceBPETokenizer.kt, so they are not visible here. Two easy options:
- copy both small functions into the
GGUFTokenizercompanion next tofindMatchingBracket/unescapeJsonString, or - make
TokenizerJsonParserand those two functionsinternaland call them from here.
CI is approved on your branch now, so the next push will show the compiler result directly. Locally, ./gradlew :llm-core:jvmTest catches it in under a minute.
2. Please revert gradle/wrapper/gradle-wrapper.properties. The timeout/retry change is unrelated to the issue and also drops the file's trailing newline. If the wrapper download was flaky for you, that is worth its own small issue.
3. Test. Step 3 of #472 has it ready: a 4-token vocab {"a":0,"b":1,"c":2,"ab":3} once with merges [["a","b"]] and once with ["a b"]; both must encode "ab" to [3]. One test class, two cases — that is the proof that the two forms are now identical.
4. Housekeeping. A line under Unreleased in CHANGELOG.md, and a short PR description with Closes #472 so the lane closes automatically on merge.
Nice to have, not required: the removed debug println for the vocab size could stay, just to keep the diff focused on the merges.
Ping me when it is pushed and I will re-review the same day.
|
CI finished on this branch (run 38085857435) and shows the compile errors from the review, nothing else: The third one disappears once |
) Adds the two JSON helpers the merges loop calls, findStringEnd and parseStringArray, next to the existing bracket matcher in the GGUFTokenizer companion; restores gradle-wrapper.properties from develop; keeps the vocab-size debug line so the diff stays on the merges block. GGUFTokenizerMergesFormatTest proves the string form and the array form rank the same merge, with a vocabulary where an unparsed merge would be visible ("abc" -> [a, bc], not [ab, c]), plus one escaped-string case. CHANGELOG line under Unreleased.
|
Thanks @Sehul07, the walk-the-array shape was the right call, so I finished the branch rather than send it around once more:
Your commits stay as they are, mine sits on top; I will squash-merge with you as author once CI is green. One request for next time: your commits are authored as Heads-up on the bigger picture: the |
Closes #472