Add custom `stopping_criteria` and `logits_processor` to `generate` #14779

lvwerra · 2021-12-15T11:28:36Z

What does this PR do?

This PR are continues the work and discussions from #12219 with a fresh start.

It integrates the custom stopping_criteria and logits_processor with the following logic:

the default stopping_criteria/logits_processor are created from the arguments and model's config
if additional, custom stopping_criteria and logits_processor are passed to generate, they are compared with the default list
- if there is an overlap between the two lists (e.g. a MaxLengthCriteria in both lists) an error is thrown
- if there is no overlap the two lists are merged

Narsil

Looks good and clean.

Only suggestion would be making it slightly more obvious in the docstring that those arguments are intended for power users.

Narsil · 2021-12-15T14:58:27Z

src/transformers/generation_utils.py

@@ -573,6 +573,7 @@ def _get_logits_processor(
        num_beam_groups: int,
        diversity_penalty: float,
        remove_invalid_values: bool,
+        logits_processor: Optional[StoppingCriteriaList],


I think you meant LogitsProcessorList

lvwerra · 2021-12-20T13:14:30Z

@Narsil thanks for your feedback and catching that copy-paste error! I added something to the docstring and fixed the error.

@patrickvonplaten any comments?

patrickvonplaten · 2021-12-20T21:23:51Z

src/transformers/generation_utils.py

+    def _merge_criteria_processor_list(
+        self, default_list: StoppingCriteriaList, custom_list: StoppingCriteriaList
+    ) -> StoppingCriteriaList:
+        if custom_list is None:


How can the custom_list be None? I think it can only be empty no? Wouldn't it be better to do:

if len(custom_list) == 0: return default_list

The default in generate is None but I can change it to an empty list if you think that is more consistent.

An empty StoppingCriteriaList and LogitsProcessorList to be more precise.

patrickvonplaten · 2021-12-20T21:24:35Z

src/transformers/models/rag/modeling_rag.py

@@ -1469,6 +1473,14 @@ def generate(
                conditioned on the previously generated tokens :obj:`inputs_ids` and the batch ID :obj:`batch_id`. This
                argument is useful for constrained generation conditioned on the prefix, as described in
                `Autoregressive Entity Retrieval <https://arxiv.org/abs/2010.00904>`__.
+            logits_processor (:obj:`LogitsProcessorList`, `optional`):
+                 Custom logits processors that complement the default logits processors built from arguments and a


I like the doc-string!

patrickvonplaten · 2021-12-20T21:25:10Z

src/transformers/generation_utils.py

+            for custom in custom_list:
+                if type(custom) is type(default):
+                    raise ValueError(
+                        f"A custom stopping criteria or logits processor of type {type(custom)} was passed to `generate` which is already created with an argument or model config."


Suggested change

f"A custom stopping criteria or logits processor of type {type(custom)} was passed to `generate` which is already created with an argument or model config."

f"A custom stopping criteria or logits processor of type {type(custom)} was passed "

"to `generate` which is already created with an argument or model config."

Also, since this error won't be easy to solve for the user can we try to give her/him as much guidance as possible? we could a) Figure out if it's a stopping criteria or a logits processor from the type. And then b) IMO we should display as much information as possible. E.g.

object_type = "stopping criteria" if isinstance(custom, StoppingCriteria) else "logits processor" f"A custom {object_type} of type {type(custom)} with values {custom} has been passed to `generate`, but it has already been created with the values {default}. {default} has been created by passing the corresponding arguments to generate or by the config's default values."

Something like this maybe, there is probably a better way to phrase it

Actually what we could do in a second step is to enforce that the init names of the processors have to correspond 1-to-1 to the inputs of generate() and then we can actually pass the arguments that need to set to None in generate to allow this. Maybe a bit overkill in a first step, but we could do this if we encounter issues about it

I refined the error message to include a little bit more detail. Let me know if you think it is clearer now.

I agree with your last point: if a lot of people have issues we can make it more strict.

patrickvonplaten · 2021-12-20T21:26:54Z

src/transformers/generation_utils.py

+        return criteria
+
+    def _merge_criteria_processor_list(
+        self, default_list: StoppingCriteriaList, custom_list: StoppingCriteriaList


Suggested change

self, default_list: StoppingCriteriaList, custom_list: StoppingCriteriaList

self, default_list: List[Any], custom_list: List[Any]

They are not of type List. Union[LogitsProcessorList, StoppingCriteriaList] is the correct thing (you can define a new type too to make it less verbose maybe)

GenerateList = Union[LogitsProcessorList, StoppingCriteriaList] def _merge(... default_list: GenerateList) -> GenerateList: ....

I have added Union[LogitsProcessorList, StoppingCriteriaList]. Should be more careful with type hints in the future. I chose the verbose option because it is quite readable that way.

patrickvonplaten · 2021-12-20T21:27:05Z

src/transformers/generation_utils.py

+
+    def _merge_criteria_processor_list(
+        self, default_list: StoppingCriteriaList, custom_list: StoppingCriteriaList
+    ) -> StoppingCriteriaList:


Suggested change

) -> StoppingCriteriaList:

) -> List[Any]:

See above discussion.

src/transformers/generation_utils.py

patrickvonplaten

Great work! Thanks a lot for the feature.

Feel free to merge whenever :-)

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

…uggingface#14779) * add custom `stopping_criteria` and `logits_processor` to `generate` * add tests for custom `stopping_criteria` and `logits_processor` * fix typo in RAG * address reviewer comments * improve custom logits processor/stopping criteria error message * fix types in merge function signature * change default for custom list from `None` to empty list * fix rag generate * add string split suggestion Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

lvwerra added 2 commits December 15, 2021 12:14

add custom stopping_criteria and logits_processor to generate

6dfbcff

add tests for custom stopping_criteria and logits_processor

fa4ed80

lvwerra requested review from Narsil and patrickvonplaten December 15, 2021 11:28

fix typo in RAG

865c7a1

Narsil approved these changes Dec 15, 2021

View reviewed changes

address reviewer comments

1ccaffa

Merge branch 'master' into custom-criteria-logits-processor-generate

a4e875b

patrickvonplaten reviewed Dec 20, 2021

View reviewed changes

lvwerra added 5 commits December 21, 2021 14:15

improve custom logits processor/stopping criteria error message

f2de651

fix types in merge function signature

4a85415

change default for custom list from None to empty list

c9f653e

Merge branch 'master' into custom-criteria-logits-processor-generate

50b9210

fix rag generate

2092d81

patrickvonplaten reviewed Dec 21, 2021

View reviewed changes

src/transformers/generation_utils.py Outdated Show resolved Hide resolved

patrickvonplaten approved these changes Dec 21, 2021

View reviewed changes

add string split suggestion

836d435

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

lvwerra changed the title ~~Custom criteria logits processor generate~~ Add custom stopping_criteria and logits_processor to generate Dec 21, 2021

lvwerra merged commit 5722d05 into master Dec 21, 2021

lvwerra deleted the custom-criteria-logits-processor-generate branch December 21, 2021 15:47

lvwerra mentioned this pull request Dec 23, 2021

add custom stopping criteria to human eval script #14897

Merged

Narsil mentioned this pull request Dec 27, 2021

Enabling users to provide their own stopping_criteria + logits_processor to generate. #12219

Closed

5 tasks

patrickvonplaten mentioned this pull request Dec 27, 2021

Logits warper for batch generation #14530

Closed

patil-suraj mentioned this pull request Jan 12, 2022

Support custom StoppingCriteria in model.generate #15108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom `stopping_criteria` and `logits_processor` to `generate` #14779

Add custom `stopping_criteria` and `logits_processor` to `generate` #14779

lvwerra commented Dec 15, 2021

Narsil left a comment

Narsil Dec 15, 2021

lvwerra commented Dec 20, 2021

patrickvonplaten Dec 20, 2021

lvwerra Dec 21, 2021

lvwerra Dec 21, 2021

patrickvonplaten Dec 20, 2021

patrickvonplaten Dec 20, 2021

patrickvonplaten Dec 20, 2021 •

edited

patrickvonplaten Dec 20, 2021

patrickvonplaten Dec 20, 2021

lvwerra Dec 21, 2021 •

edited

patrickvonplaten Dec 20, 2021

Narsil Dec 21, 2021

lvwerra Dec 21, 2021

patrickvonplaten Dec 20, 2021

lvwerra Dec 21, 2021

patrickvonplaten left a comment

	f"A custom stopping criteria or logits processor of type {type(custom)} was passed to `generate` which is already created with an argument or model config."
	f"A custom stopping criteria or logits processor of type {type(custom)} was passed "
	"to `generate` which is already created with an argument or model config."

	self, default_list: StoppingCriteriaList, custom_list: StoppingCriteriaList
	self, default_list: List[Any], custom_list: List[Any]

Add custom stopping_criteria and logits_processor to generate #14779

Add custom stopping_criteria and logits_processor to generate #14779

Conversation

lvwerra commented Dec 15, 2021

What does this PR do?

Narsil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvwerra commented Dec 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickvonplaten Dec 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvwerra Dec 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickvonplaten left a comment

Choose a reason for hiding this comment

Add custom `stopping_criteria` and `logits_processor` to `generate` #14779

Add custom `stopping_criteria` and `logits_processor` to `generate` #14779

patrickvonplaten Dec 20, 2021 •

edited

lvwerra Dec 21, 2021 •

edited