-
Notifications
You must be signed in to change notification settings - Fork 759
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
[rag evals][1/n] refactor base scoring fn & data schema check #664
base: main
Are you sure you want to change the base?
Conversation
self.validate_row_schema(row_schema, self.get_expected_schema_for_eval()) | ||
|
||
def get_expected_schema_for_scoring(self): | ||
return [ |
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.
We will likely need to revisit expected data schema when evaluating for tools.
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.
its better for scoring/evals/post_training to all define their own schema for what they want the dataset to obey. The actual checking of the schema can be done in datasetio.
@@ -13,12 +13,51 @@ | |||
|
|||
class BaseScoringFn(ABC): |
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 still don't understand why we have these base classes because we have already declared what our impls need to do in terms of datatypes in our APIs. so the datatype for a scoring function already exists. Let's say I am implementing a new scoring function -- why do I need another base class and inherit from there? If there is some utilities I need for implementing the functions, they would be just utils / free functions or in the worst case, some mixins.
Can you explain the need for base classes please? I am very allergic to inheritance as you and @raghotham knows :)
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.
This class BaseScoringFn(ABC):
is mostly separated out based on feedback from Tejas for his use case without
registration (syncing with you offline).
For our llama-stack implementations, I agree we don't need this separate BaseScoringFn
, and could just use RegisteredBaseScoringFn
as mixins.
@@ -0,0 +1,93 @@ | |||
# Copyright (c) Meta Platforms, Inc. and affiliates. |
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 datasetio abstract schema validation instead of creating mixins to be used by scoring and evals?
What does this PR do?
Test Plan
Before submitting
Pull Request section?