[mishig25]

Closes #873 by implementing preprocessor::Sequence

as suggested by @SaulLu,

Let me just re-share here a design idea that we had - I think - also discussed offline a long time ago: add a new method to all processors - for example named process_chain - which will have another composable signature (a list) and which will be the method called by processors.Sequence. 😊

Please let me know if proposed changes look good, if so, I will add tests. Otherwise, I'm happy to make any changes as necessary. Also, please see the comments as I had some questions

[thomasw21]

Not sure I understand why we use process_chain ... I think all fn process should take encodings: Vec<Encoding> instead, and Sequence is just a for loop that calls process instead of adding a new endpoint?

[mishig25]

to emulate

tokenizers/tokenizers/src/tokenizer/mod.rs

Lines 123 to 125 in bb6fea0

	encoding.set_sequence_id(0);
	pair.set_sequence_id(1);
	encoding.merge_with(pair, false);

[mishig25]

@Narsil please feel free to do another round of review.
The things I've changed based on our huddle on friday are:

1. process_chain should not use/call process
2. process_chain should not be optional (i.e. every preprocessor must implement process_chain method)
3. template processing should err if the input (encodnings vector) length is anything other than 1 or 2
4. added tests preprocessor (for bytelevel, template, sequence)

• add bindings implementation for composable processors

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

[Narsil]

Seems a fine direction but we need to remove the code within the process function in each implementor of the trait no ?

[Narsil]

We probably want to check some type_ids and other vec being filled here.

(Maybe you added the tests directly within the merge function which would be better, will have to check)

[mishig25]

we need to remove the code within the process

do you mean, that process implementation should use/call process_chain
if so, commit 78cb806 does so

except processors::Sequence::process, process implementations for other processors (Bert, Roberta, ByteLevel, Template) are identical, which is:

fn process(
    &self,
    encoding: Encoding,
    pair_encoding: Option<Encoding>,
    add_special_tokens: bool,
) -> Result<Encoding> {
    let mut encodings = vec![encoding];
    if let Some(encoding) = pair_encoding {
        encodings.push(encoding);
    }

    let encodings = self.process_chain(encodings, add_special_tokens)?;

    <dyn PostProcessor>::merge_encodings(encodings)
}

therefore, made process of PostPorcoessor trait here have a default implementation, wdyt?

[thomasw21]

Does it make it available to python? Is there a way not to do that? One of the discuss we vaguely discussed with @SaulLu is that we could keep this feature only in rust so that the day we want to break it and change the API it doesn't count as a API breaking change. What do you think?

cc @Narsil

[mishig25]

Because of process_chain should not be optional (i.e. every preprocessor must implement process_chain method) & the line below

tokenizers/bindings/python/src/processors.rs

Line 59 in 34bf453

impl PostProcessor for PyPostProcessor {

rust will not compile with error: PyPostProcessor does not implement process_chain

the alternative is to make implementing process_chain option, which we don't want based on process_chain should not be optional (i.e. every preprocessor must implement process_chain method)

[thomasw21]

Ah so I see, thanks! Would be down to know if it would make sense to build such a feature where we can construct private abstract methods

[SaulLu]

Hello to all!

Thanks a lot again @mishig25 for taking the lead on this feature! 🙌

I was wondering where we are on this feature! What is currently the sticking point? Is there anything we need to discuss?

[mishig25]

Superceded by #1047