Skip to content

Handle surrogate pairs in encode_with_unstable - #581

Open
devYRPauli wants to merge 2 commits into
openai:mainfrom
devYRPauli:fix/encode-with-unstable-surrogate-pairs
Open

Handle surrogate pairs in encode_with_unstable#581
devYRPauli wants to merge 2 commits into
openai:mainfrom
devYRPauli:fix/encode-with-unstable-surrogate-pairs

Conversation

@devYRPauli

Copy link
Copy Markdown

encode and encode_ordinary already recover from surrogate pairs and lone surrogates by retrying after a UTF-16 surrogate repair, but encode_with_unstable raised UnicodeEncodeError on the same inputs.

This applies the identical retry (a utf-16 / surrogatepass round-trip) to encode_with_unstable, matching the existing pattern used in encode.

Repro before the fix:

import tiktoken
enc = tiktoken.get_encoding("cl100k_base")

# "\ud83d\udc4d" is a surrogate pair (thumbs-up); "\ud83d" is a lone surrogate
enc.encode("\ud83d\udc4d")                # works
enc.encode_with_unstable("\ud83d\udc4d")  # raised UnicodeEncodeError

enc.encode("\ud83d")                      # works
enc.encode_with_unstable("\ud83d")        # raised UnicodeEncodeError

Added a test asserting that both a surrogate pair and a lone surrogate encode without raising, and that the stable prefix matches encode.

Fixes #541.

Constructing an Encoding with duplicate ranks in mergeable_ranks made
CoreBPE::new_internal hit a raw assert!, which surfaces to Python as an
uncatchable pyo3_runtime.PanicException instead of a clean ValueError.

new_internal already returns Result and reports regex errors cleanly via
Regex::new(pattern)?; py.rs maps that Err to PyValueError. Return an Err
on the length mismatch so it flows through the same path.

Refs openai#87.

Signed-off-by: Yash Raj Pandey <yashpn62@gmail.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: af7e2e8464

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tiktoken/core.py
Comment on lines +247 to +248
text = text.encode("utf-16", "surrogatepass").decode("utf-16", "replace")
return self._core_bpe.encode_with_unstable(text, allowed_special)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Recheck disallowed specials after surrogate repair

For custom encodings with a non-BMP special token, a surrogate-pair spelling of that token skips the disallowed-special guard: the regex check above runs on the unrepaired string (e.g. "\ud83d\udc4d"), but this retry tokenizes the repaired scalar (e.g. "👍") without running the guard again. In that case encode_with_unstable no longer raises for a default-disallowed special token even though the normal scalar spelling still would; rerun the special-token check after repair before calling into Rust.

Useful? React with 👍 / 👎.

@devYRPauli

Copy link
Copy Markdown
Author

On the disallowed_special check in the surrogate-repair branch: the guard runs on the original text before the try/except, so any special-token substring (special tokens are plain ASCII, e.g. <|endoftext|>) is present unchanged in the original text and is already caught there, raising before the repair path is reached. The repair only runs on UnicodeEncodeError (i.e. lone surrogates) and only replaces those surrogates with U+FFFD, which cannot form a special token. This branch also mirrors the existing encode method, which handles disallowed_special the same way. So there is no additional bypass introduced here.

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.

encode_with_unstable does not handle surrogate pairs like encode

1 participant