Add TextGeneration Evaluator #350
Conversation
Uses word_count as default metric for the moment
Not relevant for this PR. Instead useful for perplexity, in a future PR.
|
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Thanks @NimaBoscarino for adding this - looks great to me! Left a few questions/remarks.
| @@ -347,7 +347,7 @@ def load_data(self, data: Union[str, Dataset], subset: str = None, split: str = | |||
| "Please specify a valid `data` object - either a `str` with a name or a `Dataset` object." | |||
| ) | |||
|
|
|||
| def prepare_data(self, data: Dataset, input_column: str, label_column: str): | |||
| def prepare_data(self, data: Dataset, input_column: str, label_column: str, *args, **kwargs): | |||
There was a problem hiding this comment.
The TextGeneration Evaluator's predictions_processor method has a different signature than the base Evaluator's one, which means that without adding this IDEs will complain about mismatched signatures. I did this following this pattern: https://stackoverflow.com/a/54155637
| @@ -49,6 +50,10 @@ | |||
| "implementation": TokenClassificationEvaluator, | |||
| "default_metric_name": "seqeval", | |||
| }, | |||
| "text-generation": { | |||
| "implementation": TextGenerationEvaluator, | |||
| "default_metric_name": "word_count", | |||
There was a problem hiding this comment.
To solve the perplexity issue we could just make gpt2 the default model so it's a kwarg instead of arg.
There was a problem hiding this comment.
Not sure I understand – gpt2 would be the default value for model_or_pipeline for the TextGenerationEvaluator's compute method?
There was a problem hiding this comment.
No, I meant we can update the perplexity metric to have a default value for the model (gpt2) so it works easily with the Evaluator. What do you think?
There was a problem hiding this comment.
Oh! Sure, that sounds good to me. I think that one of the basic requirements for perplexity is that it also needs the ability to receive the actual model itself as well, so I'll make sure that's possible and I'll also include that as an option in the TextGenerationEvaluator here. I'll open a separate PR for the perplexity change.
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
|
Merged for now and we can fix PPL later. |
This reverts commit 2d65438.
This PR adds a basic TextGeneration evaluator. For the moment it's set up to use
word_countas the default metric, but I think ideally it would useperplexity. The issue with perplexity is that the measurement requires amodel_id(model name or path) passed to it, which wouldn't work with the Evaluator API since it should support running evaluations on models that are already instantiated in memory. (The model being evaluated might not be saved locally, if it's been modified in-code.)I can open a second PR to modify
perplexityto optionally accept instantiated models? Then I can make perplexity the default metric here.The other metric that this is useful for is HONEST, although that requires some more small changes to get metric init_kwargs passed down from the Evaluators to the metrics. I'll be opening up another PR soon for that. (EDIT: Opened it here #351)
NOTE: I also changed the base
.compute()return signature to fix some IDE warnings.