plumbing: full pipeline#2130
Merged
Merged
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
SBrandeis
commented
Jul 2, 2026
Comment on lines
+569
to
+590
| impl PipelinePatternMatcher for AddedVocabulary { | ||
| fn get_next_special_token( | ||
| &self, | ||
| input: &str, | ||
| normalized: bool, | ||
| ) -> Option<((usize, usize), u32)> { | ||
| if input.is_empty() { | ||
| return None; | ||
| } | ||
| let trie = if normalized { | ||
| self.split_normalized_trie.as_ref() | ||
| } else { | ||
| self.split_trie.as_ref() | ||
| }?; | ||
| let matched = trie.leftmost_find_iter(input).next()?; | ||
| let start = matched.start(); | ||
| let end = matched.end(); | ||
| let token_id = matched.value(); | ||
|
|
||
| Some(((start, end), token_id)) | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
cc @ArthurZucker this is where to plug the new added_vocab matching logic
Collaborator
There was a problem hiding this comment.
I need an offset on the string to be able to do lstrip / rstrip and single word falgs
SBrandeis
commented
Jul 2, 2026
Comment on lines
+37
to
+45
| impl PreTokenizer for PipelinePreTokenizer { | ||
| fn pre_tokenize(&self, text: &str, out: &mut Vec<Split>) -> Result<()> { | ||
| match self { | ||
| Self::None => Ok(()), | ||
| Self::Bert(pretok) => pretok.pre_tokenize(text, out), | ||
| Self::Whitespace(pretok) => pretok.pre_tokenize(text, out), | ||
| } | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
cc @McPatate we just add an entry to the enum per pre-tokenizer we implement
ArthurZucker
reviewed
Jul 2, 2026
| { | ||
| type Item = Segment<'a>; | ||
|
|
||
| fn next(&mut self) -> Option<Self::Item> { |
Collaborator
There was a problem hiding this comment.
can we document what we are ddoing?
Like why dod we store the special for example
Comment on lines
+569
to
+590
| impl PipelinePatternMatcher for AddedVocabulary { | ||
| fn get_next_special_token( | ||
| &self, | ||
| input: &str, | ||
| normalized: bool, | ||
| ) -> Option<((usize, usize), u32)> { | ||
| if input.is_empty() { | ||
| return None; | ||
| } | ||
| let trie = if normalized { | ||
| self.split_normalized_trie.as_ref() | ||
| } else { | ||
| self.split_trie.as_ref() | ||
| }?; | ||
| let matched = trie.leftmost_find_iter(input).next()?; | ||
| let start = matched.start(); | ||
| let end = matched.end(); | ||
| let token_id = matched.value(); | ||
|
|
||
| Some(((start, end), token_id)) | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
I need an offset on the string to be able to do lstrip / rstrip and single word falgs
SBrandeis
force-pushed
the
plumbing-pretokenizers
branch
from
6af5ebf to
1954de7
Compare
SBrandeis
commented
Jul 2, 2026
SBrandeis
commented
Jul 2, 2026
Rebased onto feat/train_encode_split, which parked the #2129 fast path (BucketVocabStore / Buckets / bucket_added_vocabulary) and restored the legacy Tokenizer as the A/B baseline. This collapses the three stale fixup commits ("rebase", "attempt to fix", "apply normalizer in legacy tokenizer") from the previous rebase — they targeted the pre-park base — into one coherent step: - bucket_added_vocabulary.rs: the final fast AddedVocabulary (Buckets-backed, extract_next / extract_and_normalize, impl PipelinePatternMatcher). - PipelineTokenizer uses the bucket AddedVocabulary; the base Tokenizer stays legacy. TryFrom<&Tokenizer> rebuilds the bucket AV from the tokenizer's added tokens in id order, so ids are preserved (model-present tokens reuse their model id) and the pipeline emits the same ids as the reference tokenizer. pipeline_oracle passes: identical ids on big.txt (English) and wagahai (Japanese) at 1kB/10kB chunks. Full tk-encode/tk-train suites green; fmt + clippy -D warnings clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The PipelineTokenizer drives special-token matching through `extract_next` (`PipelinePatternMatcher`) + `SpecialSegmentIterator`, so the bucket AV's `extract_and_normalize` / `split_on_matches` pair is dead code (only its own test used it). Drop them and the now-unused `Range`/`PreTokenizedString`/`Token` imports. The shared helpers (`is_ws`, `is_single_word`, `skip_whitespace_*`) stay — `extract_next` uses them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the test-only `extract_segments` / `extract_two_pass` / `owned` helpers and the 7 extraction tests built on them. `extract_two_pass` reimplemented `PipelineTokenizer::encode`'s two-pass loop inside the test module (a drift-prone duplicate); `extract_next`'s matching behavior is exercised end-to-end by pipeline_oracle. Kept the AV-level unit tests that don't go through the harness: can_add_tokens, can_add_special_tokens, normalized_tokens_are_stored_by_normalized_form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SBrandeis
force-pushed
the
plumbing-pretokenizers
branch
from
6371e9c to
89a1d19
Compare
Restore the extract_next coverage dropped with the extract_two_pass harness, but drive the real `SpecialSegmentIterator` instead of a parallel reimplementation. Five single-pass tests over a bucket `AddedVocabulary`: raw added-token carving, single_word, lstrip/rstrip span absorption, the encode_special_tokens toggle, and raw-vs-normalized matcher selection. `SpecialSegmentIterator::new` is now pub(crate) so the AV tests can construct it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ArthurZucker
approved these changes
Jul 3, 2026
Comment on lines
+404
to
+407
| if self.encode_special_tokens && metadata.special { | ||
| search = match_end; | ||
| continue; | ||
| } |
Comment on lines
+296
to
+309
| struct FixedMatcher(Vec<((usize, usize), u32)>); | ||
| impl PipelinePatternMatcher for FixedMatcher { | ||
| fn extract_next( | ||
| &self, | ||
| _bytes: &[u8], | ||
| search_offset: usize, | ||
| _normalized: bool, | ||
| ) -> Option<((usize, usize), u32)> { | ||
| self.0 | ||
| .iter() | ||
| .find(|((start, _), _)| *start >= search_offset) | ||
| .copied() | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
Fixed is naive? we could / should use daachorse as replacement
Contributor
Author
There was a problem hiding this comment.
Just a test fixture even
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
3 participants
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.
TL;DR
Implement a new encode loop using the optimized blocks from other PRs