Handle surrogate pairs in encode_with_unstable - #581
Conversation
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>
There was a problem hiding this comment.
💡 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".
| text = text.encode("utf-16", "surrogatepass").decode("utf-16", "replace") | ||
| return self._core_bpe.encode_with_unstable(text, allowed_special) |
There was a problem hiding this comment.
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 👍 / 👎.
|
On the |
encodeandencode_ordinaryalready recover from surrogate pairs and lone surrogates by retrying after a UTF-16 surrogate repair, butencode_with_unstableraisedUnicodeEncodeErroron the same inputs.This applies the identical retry (a
utf-16/surrogatepassround-trip) toencode_with_unstable, matching the existing pattern used inencode.Repro before the fix:
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.