-
Notifications
You must be signed in to change notification settings - Fork 20
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
Refactoring of the local metrics #94
Conversation
@yosukehigashi |
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.
Wow what a beautiful refactor 🤩 Left a bunch of comments but generally LGTM!
Let's do a sanity check with LangCheckChat as well since this is a pretty big change. Could you test out English and Japanese, and I'll do Chinese and German?
'''Scorer using Hugging Face's AutoModelForSequenceClassification. | ||
''' | ||
|
||
def __init__(self, |
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.
Can you add a docstring here? E.g. we should explain the allowed overflow strategies and what they do
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 simple docstring there
Thanks for the review! Let me resolve the comments first, then we can do the sanity-check together as you say! |
@yosukehigashi |
yeah looks fine to me! can you start testing English and Japanese then? I need to make some code changes to test German / Chinese |
Tried some examples of Japanese & English with references, there were no errors at least. |
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.
Chinese and German work fine too. LGTM!
Refactored (mainly) local metrics code. The changes include:
Detoxify
/AutoModelForSequenceClassification
, created base classes and made logics such astqdm
progress bar common. For these metrics I also added an argumentlocal_overflow_strategy
, with which you can handle the input longer than the model input window in 3 different ways (truncate
,nullify
,raise
)truncation=True
argument. (I would like to add the overflow handling to the factual consietency metrics too, but it should be huge enough to develop in another PR)model_manager
.check_model_availability
became flaky as a result, so I disabled these checks for now.