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

Jump to bottom

Support updating template processors #1652

Open

ArthurZucker wants to merge 15 commits into main from sequential-post-processor

+509 −130

Collaborator

ArthurZucker commented Oct 14, 2024 •

edited

Loading

Goal:

from tokenizers import Tokenizer
from tokenizers.processors import TemplateProcessing
tokenizer = Tokenizer.from_pretrained("meta-llama/Llama-3.3-70B-Instruct")
tokenizer.post_processor 

tokenizer.post_processor[1] = TemplateProcessing(
    single="[CLS] $0 [SEP]",
    pair="[CLS] $A [SEP] $B:1 [SEP]:1",
    special_tokens=[("[CLS]", 1), ("[SEP]", 0)],
)

HuggingFaceDocBuilderDev commented Oct 14, 2024

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.

ArthurZucker and others added 9 commits

January 14, 2025 03:06


          current updates

394eff4


          simplify

8b55e53


          set_item works, but `tokenizer._tokenizer.post_processor[1].single = …

59f6c60

…["$0", "</s>"]` does not !


          fix: normalizers deserialization and other refactoring

71d68ec


          fix: pre_tokenizer deserialization

7c58995


          feat: add __len__ implementation for normalizer::PySequence

d69cba8


          feat: add __setitem__ impl for normalizers::PySequence

dfbbc52


          feat: add __setitem__ impl to pre_tokenizer::PySequence

1a31fc9


          feat: add __setitem__ impl to post_processor::PySequence

4bb595b

McPatate force-pushed the sequential-post-processor branch from 11533c5 to 4bb595b Compare

January 14, 2025 02:37

McPatate added 5 commits

January 15, 2025 20:07


          test: add normalizer sequence setter check

9e22b9e


          refactor: allow unused processors::setter macro

adfaace


          test: add __setitem__ test for processors & pretok

519b009


          refactor: unwrap -> PyException::new_err()?


          refactor: fmt

2c6f83f

McPatate marked this pull request as ready for review

January 16, 2025 02:33


          refactor: remove unnecessary pub

4aeee0b

McPatate approved these changes

View reviewed changes

Member

McPatate left a comment

Self-approving here as it is your PR @ArthurZucker, waiting for your review before merging

ArthurZucker commented

View reviewed changes

Collaborator Author

ArthurZucker left a comment

LGTM! WOuld just add python tests! 😉

let's check set_item and also that get_item_ is mutable

bindings/python/Cargo.toml

@@ @@ -14,7 +14,7 @@ serde = { version = "1.0", features = ["rc", "derive"] } @@
               serde_json = "1.0"
               libc = "0.2"
               env_logger = "0.11"
-              pyo3 = { version = "0.23", features = ["abi3", "abi3-py39"] }
+              pyo3 = { version = "0.23", features = ["abi3", "abi3-py39", "py-clone"] }

Collaborator Author

ArthurZucker Jan 16, 2025

for this we probably need to rebase!

Member

McPatate Jan 17, 2025

Don't think so, @Narsil removed it in f4c9fd7

bindings/python/pyproject.toml

Collaborator Author

ArthurZucker Jan 16, 2025

let's revert these as well!

bindings/python/src/normalizers.rs

+                          PyNormalizerTypeWrapper::Single(ref inner) => match &*inner
+                              .as_ref()
+                              .read()
+                              .map_err(|_| PyException::new_err("rwlock is poisoned"))?

Collaborator Author

ArthurZucker Jan 16, 2025

Suggested change

      
                            .map_err(|_| PyException::new_err("rwlock is poisoned"))?
          
                            .map_err(|_| PyException::new_err("rwlock was poisoned when trying get subtype of PyNormalizer"))?

bindings/python/src/normalizers.rs

@@ @@ -218,7 +223,7 @@ macro_rules! getter { @@
                   ($self: ident, $variant: ident, $name: ident) => {{
                       let super_ = $self.as_ref();
                       if let PyNormalizerTypeWrapper::Single(ref norm) = super_.normalizer {
-                          let wrapper = norm.read().unwrap();
+                          let wrapper = norm.read().expect("rwlock is poisoned");

Collaborator Author

ArthurZucker Jan 16, 2025

same here let's be a little bit more helpful

bindings/python/src/normalizers.rs

Comment on lines +418 to +422

+                  fn __len__(self_: PyRef<'_, Self>) -> usize {
+                      match &self_.as_ref().normalizer {
+                          PyNormalizerTypeWrapper::Sequence(inner) => inner.len(),
+                          PyNormalizerTypeWrapper::Single(_) => 1,
+                      }

Collaborator Author

ArthurZucker Jan 16, 2025

nice, thanks!
I think the issue you mentioned about length comes from here

Member

McPatate Jan 17, 2025

Because it's a Single? Not sure, it should deser to empty Sequence, but the type is Sequence with length 1 containing an NKFC

bindings/python/src/normalizers.rs

Comment on lines +451 to +453

+                                      .read()
+                                      .map_err(|_| PyException::new_err("rwlock is poisoned"))?
+                                      .clone();

Collaborator Author

ArthurZucker Jan 16, 2025

ah so we get a clone of the rust object with wrtie lock nice

bindings/python/src/normalizers.rs

               pub(crate) enum PyNormalizerTypeWrapper {
                   Sequence(Vec<Arc<RwLock<PyNormalizerWrapper>>>),
                   Single(Arc<RwLock<PyNormalizerWrapper>>),
               }
+              impl<'de> Deserialize<'de> for PyNormalizerTypeWrapper {

Collaborator Author

ArthurZucker Jan 16, 2025

can you add a comment as to why we need this?

Collaborator Author

ArthurZucker Jan 16, 2025

(We have to implement this to make sure the it's a Sequence(Sequence) and not Single(Sequence) wirite?

bindings/python/src/normalizers.rs

Comment on lines +750 to +754

+                              .map_err(|_| PyException::new_err("rwlock is poisoned"))?
+                              .normalize(normalized),
+                          PyNormalizerTypeWrapper::Sequence(inner) => inner.iter().try_for_each(|n| {
+                              n.read()
+                                  .map_err(|_| PyException::new_err("rwlock is poisoned"))?

Collaborator Author

ArthurZucker Jan 16, 2025

same let's have a more helpful error. I think in totenizers we create a type for the PyException, can be "IndexingError" or something

tokenizers/src/processors/template.rs

+                  // Setter for `single`
+                  pub fn set_single(&mut self, single: Template) {
+                      println!("Setting single to: {:?}", single); // Debugging output

Collaborator Author

ArthurZucker Jan 16, 2025

to remove!

bindings/python/tests/bindings/test_normalizers.py

Comment on lines +78 to 80

		normalizers[1] = Strip()
		assert normalizers[1].__class__ == Strip

Collaborator Author

ArthurZucker Jan 16, 2025

let's check set_item and also that get_item_ is mutabkle

Member

McPatate Jan 17, 2025

It isn't atm, need to impl getters and setters for each variant of the Normalizer/Pretok/Processor. Will do in a subsequent PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet