Repository navigation
Commit 16fef19
authored
Do not hand the tokenizer loader an unusable special token list (pytorch#22047)
Part of pytorch#21805.
The Apple frameworks are built from the `extension/llm/tokenizers` submodule
pin. The tokenizers half of this bug is meta-pytorch/tokenizers#210, and it
reaches iOS users only once that pin moves forward. This PR does not move it,
and it does not depend on it either: the two changes fix the same crash from
opposite ends and can land in either order.
## Problem
`ExecuTorchLLMTextRunner`'s two argument initializer passes `@[]` for special
tokens, and the three argument one accepts whatever array the caller gives it,
including an empty one. Either way the runner holds an empty
`std::vector<std::string>` and hands that to `load_tokenizer`.
An empty vector is not the same thing as no vector. `load_tokenizer` takes it at
face value and builds a `Tiktoken` whose BOS index 0 and EOS index 1 point past
the end of an empty list. In the 1.4.0 prebuilts that reached an `abort()` in
the `Tiktoken` constructor, so the app died with signal 6 rather than returning
an error. It happens whenever the HuggingFace loader fails first and the chain
falls through to Tiktoken, which is the path a Qwen style `tokenizer.json` takes
today.
A list with exactly one entry reaches the same `abort()`, through the EOS index
alone. This API fixes the BOS index at 0 and the EOS index at 1, so one entry
cannot name both, and there is no reading of that input under which the load can
succeed.
There is a second, quieter bug in the same lines. The special tokens were moved
into `load_tokenizer`, which leaves the runner's own pointer null, not empty. A
caller that retried `load` after a failure therefore took the branch that means
"no list was given", and `Tiktoken` filled in its own 256 built in Llama 3
special tokens. The retry looks like it succeeded while quietly producing token
ids from the wrong list.
## Solution
Pass nothing when the list is empty, so the tokenizer falls back to its own
defaults, and copy the list instead of moving it so a retry still has it.
Reject a list of exactly one entry with an error before the loader runs.
Defaults cannot stand in for it the way they can for an empty list, because the
caller did state a list, and substituting a different one is the quiet wrong
token id bug above. It is caught here rather than left to the loader because
the loader reports a `Tiktoken` it could not set up the same way it reports a
file it could not parse, and then moves on to the next tokenizer in the chain,
so the message never names the real cause.
That check is stricter than the crash strictly requires. `load_tokenizer` only
reads the list on the Tiktoken path, so a one entry list passed alongside a
`tokenizer.json` that the HuggingFace loader accepts is ignored today rather
than fatal. Such a call is now an error. That input cannot be honoured by this
API in the case where it is read at all, so reporting it is preferred over
letting it decide the outcome by which loader happens to match first.
## Test plan
Copied `load_tokenizer` into a small C++ program and called it three ways
against a Tiktoken model file, on Linux x86_64, built against
meta-pytorch/tokenizers#210:
| what is passed | result |
|---|---|
| an empty list, as the runner did before | `bos index 0 and eos index 1 must both be below the special token count 0`, the chain returns null, no abort |
| nothing, as the runner does now | loads, vocab 128256 |
| a moved from pointer, as a retry saw | loads with the tokenizer's own defaults, BOS piece is the built in begin of text token, not the caller's |
The one entry case is covered on the tokenizers side by
`TiktokenTest.LoadWithInvalidEOSIndex`, which passes a single token with BOS 0
and EOS 1 and is the same constructor call `load_tokenizer` makes. It aborts on
the current submodule pin and returns `LoadFailure` with
meta-pytorch/tokenizers#210 applied. This change keeps that call from being made
at all.
CI compiles the changed file clean for iOS device, iOS simulator and macOS, in
the `apple (ios)`, `apple (ios-simulator)` and `apple (macos)` builds. The one
warning reported against it comes from a header it includes and shows up on the
unchanged files in the same target too.
Not tested: there is no run on a device, and no automated test. The repository
has no host runnable harness that exercises this binding, and the existing Swift
tests cover a successful load, not these paths.1 parent d042ed6 commit 16fef19
1 file changed
Lines changed: 23 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
56 | 56 | | |
57 | 57 | | |
58 | 58 | | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
59 | 78 | | |
60 | 79 | | |
61 | | - | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
62 | 84 | | |
63 | 85 | | |
64 | 86 | | |
| |||
0 commit comments