Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ability to inspect a 'Sequence' pre-tokenizer. #1341

Merged
merged 6 commits into from
Sep 21, 2023
Merged

Added ability to inspect a 'Sequence' pre-tokenizer. #1341

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
07b6ab3
Added ability to inspect a 'Sequence' pre-tokenizer.
eaplatanios Sep 19, 2023
f4ea1ed
Added ability to inspect a 'Sequence' pre-tokenizer.
eaplatanios Sep 19, 2023
e492f9d
Added ability to inspect a 'Sequence' pre-tokenizer.
eaplatanios Sep 19, 2023
a9ce58f
Linting error.
eaplatanios Sep 19, 2023
28c18dc
Fix.
eaplatanios Sep 20, 2023
7b3f7e2
Revert rename,
eaplatanios Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions tokenizers/src/pre_tokenizers/sequence.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,27 @@ use serde::{Deserialize, Serialize};
#[derive(Clone, Debug, PartialEq)]
#[macro_rules_attribute(impl_serde_type!)]
pub struct Sequence {
pretokenizers: Vec<PreTokenizerWrapper>,
pre_tokenizers: Vec<PreTokenizerWrapper>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like a breaking change to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a good point. I did it as a fly-by to match PreTokenizer but you're right. I'll revert that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though given this field is private I don't think it's a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted it in either case.

}

impl Sequence {
pub fn new(pretokenizers: Vec<PreTokenizerWrapper>) -> Self {
Self { pretokenizers }
pub fn new(pre_tokenizers: Vec<PreTokenizerWrapper>) -> Self {
Self { pre_tokenizers }
}

pub fn get_pre_tokenizers(&self) -> &[PreTokenizerWrapper] {
&self.pre_tokenizers
}

pub fn get_pre_tokenizers_mut(&mut self) -> &mut [PreTokenizerWrapper] {
Copy link
Contributor Author

@eaplatanios eaplatanios Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These two new functions are just mimicking the interface you already have for the normalizer Sequence.

&mut self.pre_tokenizers
}
}

impl PreTokenizer for Sequence {
fn pre_tokenize(&self, pretokenized: &mut PreTokenizedString) -> Result<()> {
for pretokenizer in &self.pretokenizers {
pretokenizer.pre_tokenize(pretokenized)?;
for pre_tokenizer in &self.pre_tokenizers {
pre_tokenizer.pre_tokenize(pretokenized)?;
}
Ok(())
}
Expand Down