Fix single letter stop strings

[Rocketknight1]

Our StopStringCriteria has a bug when all stop strings are one letter long. This results in a case where no stop strings have any valid_positions, as this variable only tracks internal positions, and this causes a max() to fail later.

This code fixes the issue by ensuring that max() takes a minimum value of 1. It also adds a test for single-letter stop strings!

Fixes #31435

cc @amyeroberts @zucchini-nlp @gante

[HuggingFaceDocBuilderDev]

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.

[amyeroberts]

Our StopStringCriteria has a bug when all stop strings are one letter long. This results in a case where no stop strings have any valid_positions, as this variable only tracks internal positions, and this causes a max() to fail later.

@Rocketknight1 Could you explain this a bit more? Why does this result in no stop strings having any valid_positions? i.e. what does "this variable only tracks internal positions" mean?

[gante]

LGTM 👍

[Rocketknight1]

@amyeroberts Sorry for the confusion! The way this works is that we precompute two things for every (token, stop_string) pair: The length of the token's overlap with the end of the stop string, which we use to begin matches, and the positions inside the stop string where the token can match that don't overlap with the end.

The max() in the code was implicitly assuming that at least one (token, stop_string) pair would have a valid non-end position (a position inside the string that the token matches that does not overlap with the end of the stop string), but there are cases when this isn't true, and the most common one is when all the stop strings are a single character, which can only have end-matches.

[lapp0]

Thanks so much for quickly addressing!

Code changes look good, but I cannot test due to related bug:

E               ValueError: There are one or more stop strings, either in the arguments to `generate` or in the model's generation config, but we could not locate a tokenizer. When generating with stop strings, you must pass the model's tokenizer to the `tokenizer` argument of `generate`.

Can we get rid of the second tokenizer = kwargs.pop("tokenizer", None) as well in this PR?

https://github.com/huggingface/transformers/blob/eed9ed67987/src/transformers/generation/utils.py#L1643

[gante]

@lapp0 that issue was fixed today (#31427), if you rebase on main and apply this PR on top of it it should work 🤗

[lapp0]

Great job @Rocketknight1 LGTM

Thanks so much for the tokenizer popping fix @gante

[amyeroberts]

Thanks for fixing!