-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add text2text evaluator #261
Conversation
The documentation is not available anymore as the PR was closed or merged. |
@@ -350,7 +350,7 @@ def prepare_pipeline( | |||
pipe = model_or_pipeline | |||
if tokenizer is not None and feature_extractor is not None: | |||
logger.warning("Ignoring the value of the preprocessor argument (`tokenizer` or `feature_extractor`).") | |||
if pipe.task != self.task: | |||
if (pipe.task != self.task) and not (self.task == "translation" and pipe.task.startswith("translation")): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because for translation the task in the pipeline is translation_xx_to_yy
where the placeholders are the languages.
That's great! We should include the new subclasses in https://github.com/huggingface/evaluate/blob/main/docs/source/package_reference/evaluator_classes.mdx I think, and probably mention them in the how-to guide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, some comments and questions inline!
Co-authored-by: helen <31600291+mathemakitten@users.noreply.github.com>
…d-text2text-evaluator
Thanks for your feedback, I think I integrated everything. Let me know if you think anything else needs to be changed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good to me, just some minor changes needed to the examples in the doc; see comments.
@NimaBoscarino have you had a chance to try this out yet? We can merge this for now but make updates to text2text-generation if it doesn't work with HONEST.
Co-authored-by: helen <31600291+mathemakitten@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a couple small comments! I don't think that this works with HONEST for the moment for a couple reasons, but I'm wrapping up a few tests and will post my thoughts + questions as soon as I'm done.
EDIT: After talking with Leandro, it sounds like HONEST would likely work better with a FillMaskEvaluator, so I can hack one together for that tomorrow!
Co-authored-by: Nima Boscarino <nima.boscarino@gmail.com>
* add text2text evaluator * add tests * Apply suggestions from code review Co-authored-by: helen <31600291+mathemakitten@users.noreply.github.com> * final tweaks to t2t evaluators * fix compute docstring in base class * add new split/subset logic * Apply suggestions from code review Co-authored-by: helen <31600291+mathemakitten@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Nima Boscarino <nima.boscarino@gmail.com> * update docs Co-authored-by: helen <31600291+mathemakitten@users.noreply.github.com> Co-authored-by: Nima Boscarino <nima.boscarino@gmail.com>
This PR adds three evaluators following the pipeline philosophy:
While they are functionality similar they use slightly different defaults. The parity tests for the Trainer are missing but we can add those in a separate PR I think.
(for some reason I can't add you as reviewer @fxmarty but I think you can comment anyway)
cc @philschmid