mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fc4680b37a
## Description Two defects in `get_encoding_for_model`, both reachable through the normal `get_tokenizer()` path. **1. `gpt-5` had no prefix entry.** It fell through to `DEFAULT_ENCODING` (`cl100k_base`) instead of `o200k_base`. Same class as the `o4` gap already patched in that tuple. cl100k emits ~33% more tokens than o200k on CJK, so every gpt-5 count was inflated there: ```text CJK sample (30x repeated sentence) o200k_base (correct) 450 tokens cl100k_base (actual) 600 tokens +33.3% ``` Note #2758 taught the *registry* that `gpt-5` → the tiktoken backend; this is the next hop, where that backend picks its *encoding*. So gpt-5 got the right tokenizer family and the wrong encoding inside it. **2. Resolution was case-sensitive.** `TokenizerRegistry.get` lowercases only its **cache key**, then constructs the counter from the caller's original string (`_create_tokenizer(model, backend)`). An uppercase deployment name — routine on Azure, where the deployment name is user-chosen — arrived verbatim, matched no prefix, and took the default encoding. The cache makes this one genuinely unpleasant: key lowercased, construction not, so **the encoding a model receives depends on the casing of whichever request warmed the cache first**, and can differ across restarts. ```text cold cache, uppercase resolved first: GPT-4o -> cl100k_base CJK=600 WRONG GPT-4.1 -> cl100k_base CJK=600 WRONG Gpt-4O-Mini -> cl100k_base CJK=600 WRONG gpt-4o -> o200k_base CJK=450 ok ``` I nearly filed this as "not reachable" — my first check ran the lowercase spelling first, which populated the shared lowercased cache key and masked it completely. The tests call `clear_cache()` so the uppercase spelling resolves cold, which is the failing order. ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) ## Changes Made - Added `("gpt-5", "o200k_base")` to the ordered prefix tuple. - `get_encoding_for_model` now lowercases its input. The lowercasing is deliberately scoped to this function rather than the registry: every `MODEL_TO_ENCODING` key is already lowercase (asserted), so it is safe here — whereas lowercasing in `TokenizerRegistry` would break HuggingFace repo ids, which *are* case-sensitive (`Qwen/Qwen3-Coder`). ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check` + `ruff format`) - [x] Type checking passes (`mypy`) - [x] New tests added for new functionality ### Test Output ```text $ pytest tests/test_tokenizer_encoding_resolution.py -q 21 passed $ git stash push headroom/ && pytest tests/test_tokenizer_encoding_resolution.py -q # gpt-5 family, every uppercase case, and both cache-order tests fail ``` Targeted run across the tokenizer/pricing/provider suites, including `test_evals_cjk_tokenization.py` since CJK is the affected content type: ```text $ pytest tests/test_utils.py tests/test_reporting.py tests/test_cost_pricing_warning_dedup.py \ tests/test_pricing.py tests/test_pricing_litellm.py tests/test_provider_model_fallback.py \ tests/test_models.py tests/test_savings_ledger.py tests/test_tokenizers.py \ tests/test_tokenizer.py tests/test_tokenizer_selection_coverage.py \ tests/test_provider_tokenizer_one_ruler.py tests/test_openai_model_table_resolution.py \ tests/test_evals_cjk_tokenization.py -q 233 passed, 16 skipped ``` ```text $ ruff check <changed files> All checks passed! $ mypy headroom/tokenizers/tiktoken_counter.py # only pre-existing release_version.py tomllib redef, present on main ``` Deferring the full suite to CI — this environment has no maturin/Rust core, so the native-dependent shards can't run locally. ## Real Behavior Proof - **Environment:** macOS, Python 3.13.7, isolated worktree at `upstream/main` (`0cb72f45`). - **Exact command / steps:** `TokenizerRegistry.clear_cache()`, then resolve each spelling cold and count a CJK sample. - **Observed result:** ```text before after gpt-5 cl100k_base CJK=600 o200k_base CJK=450 gpt-5-mini cl100k_base CJK=600 o200k_base CJK=450 GPT-4o cl100k_base CJK=600 o200k_base CJK=450 GPT-4.1 cl100k_base CJK=600 o200k_base CJK=450 Gpt-4O-Mini cl100k_base CJK=600 o200k_base CJK=450 GPT-4 cl100k_base CJK=600 cl100k_base CJK=600 (unchanged, correct) gpt-4o o200k_base CJK=450 o200k_base CJK=450 (unchanged) gpt-3.5-turbo cl100k_base CJK=600 cl100k_base CJK=600 (unchanged) ``` `GPT-4` was previously "correct" only by accident — it missed every prefix and landed on `DEFAULT_ENCODING`, which happens to be `cl100k_base`. It is now correct by resolution.
84 lines
3.2 KiB
Python
84 lines
3.2 KiB
Python
"""``get_encoding_for_model`` must not depend on casing, and must know gpt-5.
|
|
|
|
Two defects, both reachable through the normal ``get_tokenizer()`` path:
|
|
|
|
1. **gpt-5 had no prefix entry**, so it fell through to ``DEFAULT_ENCODING``
|
|
(``cl100k_base``) instead of ``o200k_base``. On CJK text cl100k emits ~33%
|
|
more tokens than o200k, so every gpt-5 count was inflated.
|
|
|
|
2. **Resolution was case-sensitive.** ``TokenizerRegistry.get`` lowercases only
|
|
its *cache key*, then builds the counter from the caller's original string
|
|
(``_create_tokenizer(model, ...)``). An uppercase deployment name -- routine
|
|
on Azure, where the deployment name is user-chosen -- reached the resolver
|
|
verbatim, matched nothing, and took the default encoding.
|
|
|
|
The cache made (2) genuinely nasty: because the key is lowercased but
|
|
construction is not, the encoding a model ends up with depended on the
|
|
casing of whichever request warmed the cache first, and could differ across
|
|
restarts. The tests below call ``clear_cache()`` so the uppercase spelling is
|
|
resolved cold, which is the failing order.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import pytest
|
|
|
|
from headroom.tokenizers import get_tokenizer
|
|
from headroom.tokenizers.registry import TokenizerRegistry
|
|
from headroom.tokenizers.tiktoken_counter import get_encoding_for_model
|
|
|
|
CJK = "这是一个测试文档，用于验证分词器的差异。" * 30
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
("model", "expected"),
|
|
[
|
|
# gpt-5 family: the missing entry.
|
|
("gpt-5", "o200k_base"),
|
|
("gpt-5-mini", "o200k_base"),
|
|
("gpt-5-nano", "o200k_base"),
|
|
("gpt-5-2025-08-07", "o200k_base"),
|
|
# Casing must not change the answer.
|
|
("GPT-4o", "o200k_base"),
|
|
("GPT-4.1", "o200k_base"),
|
|
("Gpt-4O-Mini", "o200k_base"),
|
|
("GPT-5", "o200k_base"),
|
|
("O4-Mini", "o200k_base"),
|
|
("GPT-4", "cl100k_base"),
|
|
("GPT-4-Turbo", "cl100k_base"),
|
|
# Must not regress.
|
|
("gpt-4o", "o200k_base"),
|
|
("gpt-4.1", "o200k_base"),
|
|
("gpt-4", "cl100k_base"),
|
|
("gpt-4-turbo", "cl100k_base"),
|
|
("gpt-3.5-turbo", "cl100k_base"),
|
|
("o4-mini", "o200k_base"),
|
|
],
|
|
)
|
|
def test_encoding_resolution(model: str, expected: str) -> None:
|
|
assert get_encoding_for_model(model) == expected
|
|
|
|
|
|
@pytest.mark.parametrize("model", ["gpt-5", "GPT-4o", "GPT-4.1"])
|
|
def test_cold_cache_uppercase_still_gets_the_right_encoding(model: str) -> None:
|
|
"""End-to-end through the registry, with the uppercase spelling resolved first.
|
|
|
|
Without clear_cache() a preceding lowercase lookup would populate the shared
|
|
(lowercased) cache key and mask the defect entirely.
|
|
"""
|
|
tiktoken = pytest.importorskip("tiktoken")
|
|
o200k = len(tiktoken.get_encoding("o200k_base").encode(CJK))
|
|
|
|
TokenizerRegistry.clear_cache()
|
|
assert get_tokenizer(model).count_text(CJK) == o200k
|
|
|
|
|
|
def test_casing_is_not_load_order_dependent() -> None:
|
|
"""The same model must count identically whichever spelling arrives first."""
|
|
TokenizerRegistry.clear_cache()
|
|
upper_first = get_tokenizer("GPT-4o").count_text(CJK)
|
|
|
|
TokenizerRegistry.clear_cache()
|
|
lower_first = get_tokenizer("gpt-4o").count_text(CJK)
|
|
|
|
assert upper_first == lower_first
|