Skip to content

Fix GPT-4o punctuation slash-suffix pre-tokenization - #1147

Open
kunal-vaishnavi wants to merge 3 commits into
mainfrom
kvaishnavi/fix-crlf-gpt-4o
Open

kunal-vaishnavi wants to merge 3 commits into
mainfrom
kvaishnavi/fix-crlf-gpt-4o

Conversation

@kunal-vaishnavi

Copy link
Copy Markdown
Contributor

Description

This PR keeps trailing slashes with punctuation across CR/LF boundaries for the GPT-4o regex, while preserving the CR/LF-only matcher. It also adds native regressions for LF, CRLF, mixed suffixes, and the non-slash control.

Motivation and Context

This PR fixes an issue with the GPT-4o tokenizer family.

Keep trailing slashes with punctuation across CR/LF boundaries for the GPT-4o regex, while preserving the CR/LF-only matcher. Add native regressions for LF, CRLF, mixed suffixes, and the non-slash control.
@kunal-vaishnavi
kunal-vaishnavi requested a review from a team as a code owner November 7, 2026 21:08
Copilot AI balanced review requested due to automatic review settings November 7, 2026 21:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues remain for the GPT-4o punctuation-suffix change.

No open issues 🎉

What changed in this PR

Fixes GPT-4o pre-tokenization so punctuation retains trailing slashes across carriage-return and newline boundaries, while preserving the existing CR/LF-only matcher.

Changes:

  • Shares punctuation-matching logic with an option to include slashes in the suffix.
  • Adds native regressions for LF, CRLF, mixed suffixes, and the CR/LF-only control.
File Description
test/​pp_api_test/​test_tokenizer_impl.cc Adds regression tests for slash-inclusive and CR/LF-only suffix matching.
operators/​tokenizer/​bpe_utils.hpp Routes the GPT-4o punctuation pattern to slash-aware suffix matching.

Review effort: Balanced

@sayanshaw24 Sayan Shaw (sayanshaw24) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the matcher split and regressions. The slash-aware path now implements the exact [\r\n/]* suffix while the existing CR/LF-only path preserves [\r\n]*; the tests cover LF, CRLF, mixed slash/newline suffixes, and the non-slash control. I found no correctness issues in the change.

@sayanshaw24

Copy link
Copy Markdown
Collaborator

I also checked the failing matrix jobs. The representative Linux, macOS, and Windows failures are all the unrelated test_fast_tokenizer.py::TestAutoTokenizer::test_mistral dependency error: the installed sentencepiece_model_pb2.py imports google.protobuf.runtime_version, but the environment has an older protobuf package. The new native punctuation-suffix tests are not implicated.

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.

3 participants