Skip to content

Fix/issue 474 byte encoding - #482

Merged
michalharakal merged 2 commits into
SKaiNET-developers:developfrom
Sehul07:fix/issue-474-byte-encoding
Oct 12, 2026
Merged

michalharakal merged 2 commits into
SKaiNET-developers:developfrom
Sehul07:fix/issue-474-byte-encoding

Conversation

@Sehul07

@Sehul07 Sehul07 commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Closes #474

@michalharakal michalharakal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Sehul07 — right file, right function. The byte mapping itself needs a different approach, though.

Why the current preprocess gives the wrong glyphs. byte.toInt().toChar() converts a signed Kotlin Byte: ü is UTF-8 C3 BC, 0xC3.toByte() is -61, and (-61).toChar() is U+FFC3, not Ã. So the token still misses the vocab and ends up as id 0. Two more gaps: bytes below 128 are passed through unchanged, so \n never becomes Ċ and \t never becomes ĉ; and the mapping is per character, whereas byte-level BPE is defined per UTF-8 byte of the whole string.

The table you need is already in this object. BYTE_TO_UNICODE sits a few lines below preprocess and is what postprocess inverts. Using it makes the forward path a few lines:

override fun preprocess(text: String): String {
    val bytes = text.encodeToByteArray()
    val sb = StringBuilder(bytes.size)
    for (b in bytes) sb.append(BYTE_TO_UNICODE.getValue(b))
    return sb.toString()
}

Space → Ġ falls out of the table, so the replace(" ", spaceMarker) line is no longer needed. Alternatively delegate to the engine's sk.ainet.io.tokenizer.ByteToUnicode.encode(...) — llm-core already depends on skainet-io-core.

Tests (step 4 of #474): preprocess("ü") == "ü", preprocess("\n") == "Ċ", and postprocess(preprocess(s)) == s for a string with umlauts, CJK and an emoji with a skin-tone modifier. The round trip is the one that proves the two tables agree.

Branch. This PR currently has the same three commits as #481. Once that branch is reset (see my note there), please rebase this one so it carries only the BPEStrategy commit, and drop the indentation-only change in GGUFTokenizer.detectFromVocab. Then CHANGELOG.md line + Closes #474, and it is ready for a re-review.

@michalharakal

Copy link
Copy Markdown
Contributor

CI finished on this branch and fails in :llm-core:compileCommonMainKotlinMetadata; the build treats Kotlin deprecation warnings as errors:

e: llm-core/src/commonMain/kotlin/sk/ainet/apps/llm/tokenizer/BPEStrategy.kt:43:21 'expect fun String(chars: CharArray): String' is deprecated. Use CharArray.concatToString() instead.

That line goes away entirely with the table-based preprocess suggested in the #482 review, so no need to patch it separately.

Sehul Vagadiya and others added 2 commits October 12, 2026 19:45
…T-developers#474)

BPEStrategy.preprocess now maps every UTF-8 byte of the input through
BYTE_TO_UNICODE, the table postprocess already inverts, so spaces, newlines,
tabs and all non-ASCII bytes come out of one mapping. The per-character
signed Byte -> Char conversion produced U+FFC3 for the first byte of "ü"
and left bytes below 128 untouched.

BPEStrategyByteLevelTest: space, newline, tab, umlauts per byte (ß's second
byte is a remapped control byte), printable ASCII unchanged, and a
postprocess(preprocess(s)) round trip for German, CJK and an emoji with a
skin-tone modifier. CHANGELOG line under Unreleased.
@michalharakal
michalharakal force-pushed the fix/issue-474-byte-encoding branch from af0872f to 49e3c2b Compare October 12, 2026 19:52
@michalharakal

Copy link
Copy Markdown
Contributor

Thanks @Sehul07, right file and right function. I rebuilt the branch so it carries only your BPEStrategy commit on top of develop and finished it:

  • preprocess now walks the UTF-8 bytes through BYTE_TO_UNICODE, the same table postprocess inverts, so " " → Ġ, "\n" → Ċ, "ü" → ü come out of one mapping (the signed Byte → Char conversion was the part that produced U+FFC3);
  • BPEStrategyByteLevelTest: space, newline, tab, umlauts per byte, printable ASCII unchanged, and postprocess(preprocess(s)) == s for German, CJK and an emoji with a skin-tone modifier;
  • CHANGELOG.md line.

Squash-merge with you as author once CI is green.

@michalharakal
michalharakal merged commit 1b86bb5 into SKaiNET-developers:develop Oct 12, 2026
1 check passed
michalharakal added a commit that referenced this pull request Oct 12, 2026
…ributor

The Unreleased section lists the three fixes from #480, #481 and #482 (issues
#472, #473, #474); this names the contributor who implemented them.

Co-authored-by: Sehul Vagadiya <179387438+Sehul07@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Lane 2 · kotlin-core] tokenizer.json parity — BPEStrategy.preprocess: byte-encode non-ASCII (ü currently becomes id 0)

2 participants