fix(dedup): map exact-substring ranges to original text - #516
Open
Excelius-Wang wants to merge 1 commit into
Open
Excelius-Wang wants to merge 1 commit into
Excelius-Wang wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Problem
ESRangeRemovercurrently decodes duplicate token IDs and removes the decoded string withstr.replace. This assumes encoding and decoding round-trip to the same Unicode code points. Withtokenizer normalization, decomposed text such as
u\u0303can encode to the same token as NFCũbut decode to different underlying text, causing the Stage 3 sync check or duplicate removal to fail.
str.replacecan also remove identical occurrences that were not part of the detected token range.Some normalizers also omit a trailing combining mark from the aligned token offset, so the mapped
text span must include unaligned marks attached to its final character.
This follows the retokenization-and-offset direction suggested in #121 and leaves all three stage
file formats unchanged.
Validation
pytest -q tests/pipeline/dedup— 27 passed, 3 existing lighteval/NumPy 2.0 skipsmake qualitygit diff --checkSynthetic Stage 3 benchmark: 10,000 documents, 50 tokens per document, five-run median. Stage 1 and
Stage 2 are excluded from timing.
mainmain→ PR)The common no-range path keeps the existing decode check. Re-tokenization is limited to documents
with duplicate ranges, or to the non-round-trippable fallback needed for token-ID synchronization.
This is related to #121 but does not change the Stage 1
uint16token format; tokenizers with IDsabove 65,535 remain outside this PR's scope.
Note
Medium Risk
Changes core dedup text mutation logic for exact substrings; incorrect offset mapping could drop wrong spans, though behavior is guarded by asserts and new regression tests.
Overview
Stage 3 exact-substring dedup no longer decodes duplicate token spans and strips them with
str.replace. Duplicate byte ranges are converted to token indices, mapped to character spans in the originaldoc.textvia the tokenizer’s encode offsets, with trailing combining marks included when normalization left them outside the offset window. Overlapping spans are merged, then removed by slicing the original string (newest ranges first).Documents without duplicate ranges keep a lightweight sync path (decode comparison, with encode-ID assert only when decode doesn’t round-trip). The per-document decode equality assert in
run()is removed in favor of checks insideremove_duplicatewhen ranges apply.Tests add a normalizing tokenizer harness and cases for NFC/decomposed Unicode, single-occurrence removal, emoji offsets, and overlapping ranges.
Reviewed by Cursor Bugbot for commit 1a40e8c. Bugbot is set up for automated code reviews on this repo. Configure here.