Skip to content

fix(embedding): price non-CJK non-ASCII at the UTF-8 byte ceiling - #2

Closed
kuangmi-bit wants to merge 2 commits into
akushonkamen:fix/5895-cjk-embedding-estimate-boundfrom
kuangmi-bit:fix/5895-non-ascii-byte-ceiling
Closed

kuangmi-bit wants to merge 2 commits into
akushonkamen:fix/5895-cjk-embedding-estimate-boundfrom
kuangmi-bit:fix/5895-non-ascii-byte-ceiling

Conversation

@kuangmi-bit

Copy link
Copy Markdown

Follow-up to the review in volcengine#5898 (#issuecomment-6430153829): the two gaps it names, with tests.

What changes

  1. Non-CJK non-ASCII is priced at its UTF-8 byte length, not 1 token/char. For a byte-level tokenizer the byte count is a hard ceiling — an unseen sequence can split into one token per byte — so pricing a 4-byte emoji at 1 was the one place the "upper bound" claim did not hold:

    text chars bytes before after
    👍 1 4 1 4
    🇨🇳 2 8 2 8
    👨‍👩‍👧 5 18 5 18
    नमस्ते 6 18 6 18
    สวัสดี 6 18 6 18

    CJK keeps DEFAULT_CJK_TOKENS_PER_CHAR; the docstring now says which of the two is being claimed — a measured ratio with headroom for CJK, a ceiling for everything else.

  2. 0x1F200–0x1F2FF (Enclosed Ideographic Supplement) added to _CJK_RANGES. 🈀🉐🈶 were 4 bytes each in the 1-token bucket.

Verification

python -m pytest tests/unit/test_embedding_input.py -q   # 11 passed

The truncation invariant still holds on emoji-heavy input: for "👍 中文 hello " * 500 and budgets 10/50/500/5000, estimate(truncate(t, n)) <= n.

Test changes. test_non_ascii_non_cjk_costs_at_least_one_token_per_char becomes test_non_ascii_non_cjk_priced_at_byte_ceiling (equality, not >=), plus test_emoji_keeps_more_than_one_token_per_char and 🈀 added to CJK_PROBE_CHARS. The three ranges the original issue named, and the suffix-inside-budget behaviour you added, are unchanged.

It is a behaviour change for European text too — café estimates 3 instead of 2 — which is the cost of an actual upper bound. If you would rather keep the tighter estimate for 2-byte scripts and only raise the ceiling above that, say so and I will narrow it to a per-byte-length table.

Not a maintainer; this is the issue author's patch, offered as a PR against your branch rather than a comment so you can take, edit or drop it.

A byte-level tokenizer cannot spend more tokens than a string has bytes, but
the estimator charged 1 token per non-CJK non-ASCII character: a 4-byte emoji
cost 1, Devanagari and Thai cost 1 per 6-byte word-start. For chat text that is
the class the guard exists for, the estimate sits below the provider's count and
the request is rejected or silently truncated provider-side, which is what
issue volcengine#5895 reports.

Also adds the Enclosed Ideographic Supplement block (U+1F200-1F2FF) to the CJK
ranges; it was falling into the 1-token bucket at 4 bytes per character.

Signed-off-by: Mi Kuang <kuangmi@gmail.com>
…d ideographs

Signed-off-by: Mi Kuang <kuangmi@gmail.com>
@github-actions

Copy link
Copy Markdown

Failed to generate code suggestions for PR

@kuangmi-bit

Copy link
Copy Markdown
Author

@akushonkamen — this is the two gaps from the review on volcengine#5898 (#issuecomment-6430153829), as commits on your branch fix/5895-cjk-embedding-estimate-bound rather than as more prose in that thread. Nothing here asks you to adopt my reading of the fix.

  • openviking/utils/embedding_input.py — non-CJK non-ASCII is now priced at its UTF-8 byte length (👍 1→4, 🇨🇳 2→8, 👨‍👩‍👧 5→18), and 0x1F200–0x1F2FF is in _CJK_RANGES so 🈀🉐🈶 leave the 1-token bucket.
  • tests/unit/test_embedding_input.py — the old >= assertion becomes an equality (test_non_ascii_non_cjk_priced_at_byte_ceiling), plus test_emoji_keeps_more_than_one_token_per_char.

python -m pytest tests/unit/test_embedding_input.py -q → 11 passed. The truncation invariant is re-checked on "👍 中文 hello " * 500 at budgets 10/50/500/5000: estimate(truncate(t, n)) <= n still holds.

The one open choice is yours, not a detail I should decide: café now estimates 3, not 2. That is what a real upper bound costs for 2-byte scripts too. If you would rather keep the tighter number and raise the ceiling only above 2 bytes, say so and I will narrow it to a byte-length table — it is a five-line change.

@kuangmi-bit

Copy link
Copy Markdown
Author

Closing this one: both commits are already on your branch (bdf5e425e, 53c91dc11), so keeping the PR open leaves a second copy that will drift the moment either side moves. Nothing here is unresolved.

If the byte-length table for 2-byte scripts turns out to be the shape you want (café back to 2), say so on volcengine#5898 and I will send that as a patch against the branch instead of reviving this one.

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.

1 participant