added the max_matching_ngram_size to GenerationConfig

[mosheber]

What does this PR do?

• Added the max_matching_ngram_size parameter into the GenerationConfig, for the PromptLookupCandidateGenerator.
• Included the max_matching_ngram_size when calling the init of PromptLookupCandidateGenerator in _get_candidate_generator, in case it is specified.

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@gante , would appreciate it if you could give this PR a glance, and thank you in advance.

[gante]

Hi @mosheber 👋

I'd be happy to merge the PR, conditional on the answer to the following question being yes (preferably backed with data): have you found significant benefits of changing the flag you added?

On the original issue, the author's experiments showed that there were little benefits in changing this option. As such, we don't want to add new flags unless they result in clear benefits :)

[danielkorat]

Hi @gante @mosheber 👋

The results below show a 3ms speedup in latency with a 7B target model, when comparing the default max_matching_ngram_size=2 with max_matching_ngram_size=4.
The bottom graph shows n_matches vs max_matching_ngram_size.
As the target size increases further, the speedup will be greater.
Note that these results use an optimized routine for PLD subsequence matching (~70x faster on avg, uses numba):

get_candidates:       0.3467 ms
get_candidates_opt:   0.0047 ms

[gante]

@danielkorat convinced by your numbers 👍

Let's add this PR!

[gante]

Instead of creating a dict here, let's:
a) pass keyword arguments to PromptLookupCandidateGenerator (as before the PR)
b) default max_matching_ngram_size in PromptLookupCandidateGenerator to None, and set it to the original default value in __init__ if it is None.

This pushes complexity away from generate and into PromptLookupCandidateGenerator :)

[mosheber]

Thanks for the comment! I switched back to keyword arguments.

[gante]

@mosheber after applying the fix, please run make fixup from the transformers folder and commit the result. It will fix the CI issues you're seeing :)

[mosheber]

@mosheber after applying the fix, please run make fixup from the transformers folder and commit the result. It will fix the CI issues you're seeing :)

I ran the make fixup as well.

[gante]

The changes look good to me, thank you for iterating and making transformers better 💛

[gante]

@mosheber

Actually, there is a tiny missing thing: max_matching_ngram_size should have an entry in the docstring of GenerationConfig

our CI is still complaining about code formating, make sure you have the latest version installed when running make fixup again :)

[mosheber]

@mosheber

Actually, there is a tiny missing thing: max_matching_ngram_size should have an entry in the docstring of GenerationConfig

our CI is still complaining about code formating, make sure you have the latest version installed when running make fixup again :)

Great idea! I added the doc string to the GenerationConfig class.
Regarding the tests, I fixed the formatting issue, it seems that the CI fails due to timing reasons, which are not related to this PR.

[gante]

Regarding the tests, I fixed the formatting issue, it seems that the CI fails due to timing reasons, which are not related to this PR.

Yeah don't worry about it :)

[ArthurZucker]

LGTM! just one nit

[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.