Skip to content
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

Merged
merged 30 commits into from
Mar 18, 2024
Merged

Refactoring of the local metrics #94

merged 30 commits into from
Mar 18, 2024

Conversation

liwii
Copy link
Contributor

@liwii liwii commented Mar 2, 2024

Refactored (mainly) local metrics code. The changes include:

  • For reference_free metrics with Detoxify / AutoModelForSequenceClassification, created base classes and made logics such as tqdm progress bar common. For these metrics I also added an argument local_overflow_strategy, with which you can handle the input longer than the model input window in 3 different ways (truncate, nullify, raise)
  • For semantic similarity (both local and openai), made a base class and put most of the logic into that class.
  • For non-english factual consisntency, long inputs always caused error, so avoided it by adding 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)
  • Moved loading logics of many models into model_manager. check_model_availability became flaky as a result, so I disabled these checks for now.

@liwii liwii changed the title [WIP] Overflow handling of the local metrics Overflow handling of the local metrics Mar 6, 2024
@liwii liwii changed the title Overflow handling of the local metrics Refactoring of the local metrics Mar 6, 2024
@liwii liwii marked this pull request as ready for review March 6, 2024 14:27
@liwii liwii requested a review from yosukehigashi March 6, 2024 14:28
@liwii
Copy link
Contributor Author

liwii commented Mar 6, 2024

@yosukehigashi
The PR became much bigger than I thought, but I would be happy if you could take a look when you have time!!

Copy link
Contributor

@yosukehigashi yosukehigashi left a 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,
Copy link
Contributor

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

Copy link
Contributor Author

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

@liwii
Copy link
Contributor Author

liwii commented Mar 9, 2024

Thanks for the review! Let me resolve the comments first, then we can do the sanity-check together as you say!

@liwii
Copy link
Contributor Author

liwii commented Mar 18, 2024

@yosukehigashi
Does the change look good to you? If so, we can start testing the new version.

@yosukehigashi
Copy link
Contributor

@yosukehigashi Does the change look good to you? If so, we can start testing the new version.

yeah looks fine to me! can you start testing English and Japanese then? I need to make some code changes to test German / Chinese

@liwii
Copy link
Contributor Author

liwii commented Mar 18, 2024

Tried some examples of Japanese & English with references, there were no errors at least.

Copy link
Contributor

@yosukehigashi yosukehigashi left a 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!

@liwii liwii merged commit 7b8d73f into main Mar 18, 2024
14 checks passed
@liwii liwii deleted the cleanup-local-metrics branch March 18, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants