lablup / lablup/mlxcel

fix(tokenizer): non-special added tokens do not encode as single ids on the SentencePiece path

Open
#1,758 0 comments 0 reactions 0 assignees View on GitHub
area:core priority:medium status:ready type:bug
Dominant language
Rust
Stars
467
Forks
54
Avg merge
4h 25m
Merged PRs (30d)
310

Description

## Problem

On the SentencePiece load path, `parse_special_tokens` (`src/tokenizer/mod.rs`) splits `added_tokens_decoder` by the `special` flag. Entries with `"special": true` go into `special_tokens` and are registered for encoding; entries with `"special": false` go into `added_token_contents`, which is consulted on decode only. So a non-special added token decodes back to its own spelling but does not fold back into its reserved id on encode.

HuggingFace does not make that distinction on the encode side. Every `added_tokens_decoder` entry is an `AddedToken` in the added vocabulary and is matched during encoding; `special` governs `skip_special_tokens` on decode and the `add_special_tokens` handling, not whether the token is recognized.

Measured on `models/mlx/iquest-coder-v1-40b-loop-instruct-4bit`, whose `added_tokens_decoder` has 30 entries (27 added tokens plus SentencePiece's own ``, ``, ``). The 19 `"special": true` entries round-trip in both directions. The 8 `"special": false` entries do not:

| token | id | `token_to_id` | `encode` |
| --- | --- | --- | --- |
| `` | 75872 | `None` | `[66580, 30272, 66604]` |
| `` | 75873 | `None` | `[469, 30272, 66604]` |
| `` | 75874 | `None` | `[66580, 7069, 66604]` |
| `` | 75875 | `None` | `[469, 7069, 66604]` |
| `` | 75876 | `None` | `[66580, 7005, 66561, 2998, 66604]` |
| `` | 75877 | `None` | `[469, 7005, 66561, 2998, 66604]` |
| `` | 75878 | `None` | `[66580, 7005, 66561, 3243, 66604]` |
| `` | 75879 | `None` | `[469, 7005, 66561, 3243, 66604]` |

All 8 decode correctly, so this is encode-side only.

## Impact

Observable on a tool-calling prompt. `chat_template.jinja` for this family emits ``, ``, ``, ``, `` and `` literally, so a rendered tool prompt reaches the model as 3 to 5 ordinary content tokens per tag instead of the single reserved id the model was trained on. Plain chat is unaffected: the template's non-tool path uses only `special: true` tokens, and it never emits `` or ``. The `iquestcoder` sibling has the same `added_tokens_decoder` layout and is affected identically. Any other SentencePiece checkpoint with non-special added tokens is too.

Current behavior is pinned by `the_iquest_loop_coder_tokenizer_round_trips_its_added_tokens` in `src/tokenizer/mod.rs`, which asserts the gap rather than glossing over it, so a fix will show up as that test needing to be inverted.

## Proposed direction

Register non-special added tokens on the encode side as well, keeping the `special` flag only for the decode-side `skip_special_tokens` behavior it actually governs, which is what the `tokenizers`-backed path already does through `read_added_tokens_sorted`.

The reason this was not folded into #1360 is blast radius: it changes tokenization for every SentencePiece checkpoint in the repository that carries a non-special added token, and a non-special added token that happens to be a common substring could shift ids in prompts that never intended to name it. The change needs its own survey of which local checkpoints have such entries, and a greedy-id diff on each before and after.

## Acceptance Criteria

- [ ] Survey which local SentencePiece checkpoints carry `"special": false` entries in `added_tokens_decoder` or `added_tokens.json`, and record the list
- [ ] Non-special added tokens resolve through `token_to_id` and encode as a single id on the SentencePiece path
- [ ] `skip_special_tokens` on decode still distinguishes special from non-special
- [ ] A tool-calling prompt rendered through `chat_template.jinja` for `iquestloopcoder` and `iquestcoder` tokenizes the six tool tags as one id each
- [ ] Greedy-id diff before and after on every checkpoint the survey turned up, with any change explained
- [ ] `the_iquest_loop_coder_tokenizer_round_trips_its_added_tokens` updated to assert the round trip instead of the gap

Contributor guide

Open the contributing guide

Research direction

Start in src/tokenizer/mod.rs by reading parse_special_tokens and running the_iquest_loop_coder_tokenizer_round_trips_its_added_tokens. Survey local SentencePiece checkpoints and their added_tokens_decoder or added_tokens.json entries, then compare greedy-id results before and after the change. Verify single-id encoding, decode-time skip_special_tokens behavior, and the six tool tags in chat_template.jinja for iquestloopcoder and iquestcoder.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
machine-learning, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.