Added run_tokenizer method in caikit_nlp/modules/text_generation/text_generation_local.py

[m-misiura]

There is a method with text_generation_local.py which does not appear to be implemented:

caikit-nlp/caikit_nlp/modules/text_generation/text_generation_local.py

Line 593 in 56b7e18

raise NotImplementedError("Tokenization not implemented for local")

I have written a method, which takes a string as an input and returns the token count (based on the model tokenizer). The output should follow the data_model: TokenizationResults

This method was tested with tox; the tests include checking outputs in the case of:

1. an empty string
2. a short sentence
3. a relatively long input

The implemented tests seem to pass

[evaline-ju]

Thanks for the contribution, @m-misiura! The DCO check is currently failing - could you resolve that with the steps here?

[m-misiura]

Many thanks for highlighting the lack of the sign-off @evaline-ju; I've rebased and added the sign-off accordingly

[evaline-ju]

A couple questions

[evaline-ju]

Wondering if we may want to specifically include return_attention_mask instead of leaving to the model default such as in other modules https://github.com/caikit/caikit-nlp/blob/main/caikit_nlp/modules/text_embedding/embedding.py#L1062 ?

[m-misiura]

Very interesting question! HF in their documentation mention that If left to the default, will return the attention mask according to the specific tokenizer’s default, defined by the return_outputs attribute.

For consistency and clarity with the other modules, it could be prudent to include return_attention_mask explicitly. In such a case, the only potential concern would be whether setting return_attention_mask explicitly could result in messing with the default behaviour of the tokeniser, but I think the likelihood of this is low.

Thus, I can change the method to:

def run_tokenizer(
        self,
        text: str,
    ) -> TokenizationResults:
        """Run tokenization task against the model

        Args:
           text: str
                Text to tokenize
        Returns:
            TokenizationResults
                The token count
        """
        error.type_check("<NLP48137045E>", str, text=text)
        tokenized_output = self.model.tokenizer(text, return_attention_mask=True)
        return TokenizationResults(
            token_count=len(tokenized_output["input_ids"]),
        )

Let me know how you would like me to proceed

[evaline-ju]

I slightly prefer the consistency with other modules but also don’t hold my opinion particularly strongly. The use of this local text generation module has been fairly limited, thus the lack of particular tokenization implementation previously. If the “default” would be preferred for your/your users’ usage, we can also just keep it as is.

[m-misiura]

I've added return_attention_mask=True based on our conversation to ensure consistency with the other modules

[evaline-ju]

Wondering if we could check a particular number here to make sure an expected "token count" is compared [even for this dummy model], rather than just non-zero

[m-misiura]

Checking for an expected number instead of or in addition to a non-zero condition seems like a good idea to increase the robustness of the test.

What are your thoughts on re-writing such a test like this:

assert short_result.token_count == len(model.model.tokenizer.encode(short_text))

[evaline-ju]

I think that works

[m-misiura]

Great -- changes to tests have been implemented accordingly and they seem to pass

[evaline-ju]

LGTM - thanks for the updates!