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

ragas: pass rubric in via RubricScore.SingleTurnPrompt.instruction #211

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alimaredia
Copy link
Contributor

In ragas, RubricScores.rubrics isn't being used anywhere except in repr thus it was not being passed into the prompt for the judge to evaluate responses against reference answers.

Passing a string rubric into SingleTurnPrompt.instruction allows us to pass the rubric into the prompt sent
to the judge.

@alimaredia
Copy link
Contributor Author

@RobotSail Here is what the prompts and internal responses look like at without this PR and with this PR:
https://paste.sh/KB9OjHDP#29Fd_qd5QQqCoaV7zMtm6NSG

What should the final prompt look like? There's a lot baked by ragas that's easy to see in the output when this PR is not applied.

@mergify mergify bot added the ci-failure label Jan 14, 2025
@mergify mergify bot added dependencies Pull requests that update a dependency file ci-failure and removed ci-failure labels Jan 16, 2025
Copy link
Member

@RobotSail RobotSail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @alimaredia.

I'm a little confused by this PR, because it seems like there are a lot of things being changed which may not be intended to be.

Could you please update this PR to only be changing the intended behavior (making sure that the old ragas rubric template can still be used)?

Additionally, we should somehow make the old rubric usage be a toggle-able behavior, since there is reason for which the users may want to use the new rubric.

@@ -88,29 +96,25 @@ def __init__(
self.judge_openai_api_key = judge_openai_api_key

@staticmethod
def _validate_dataset(df: DataFrame):
def validate_dataset(df: DataFrame):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we exposing this as a public dataset? What is the use-case for this?

required_keys = {"user_input", "reference"}
missing_keys = required_keys - set(df.columns)
if missing_keys:
required_keys = {"user_input", "reference", "response"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you changing response to be required? This method's only job is to ensure that the given dataset is not missing any of the values which the (InstructLab) evaluation method cannot proceed without.

required_keys = {"user_input", "reference", "response"}

columns_list = set(df.columns)
if not columns_list.issubset(required_keys):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. We use a set difference to ensure that the list of required keys are contained within the list of columns provided in the dataset.

In your code, you could have a dataset that passes in {"user_input"} and this method would allow it to be passed because ["user_input"] is mathematically a subset of ["user_input", "reference"], which this method would allow.

)

def run(
self,
dataset: List[Sample] | Path,
student_model: ModelConfig | None = None,
dataset: List[Sample] | DataFrame,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come we are getting rid of the Path type here?

run_config = run_config if run_config else self.run_config
student_openai_client = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we deleting this??

Before ragas v0.2.11 RubricScores.rubrics wasn't
being applied properly. This commit sets that
as the minimum version for this library.

A change in v0.2.11 from previous versions was a
change in the prompt for domain specific knowledge
evaluation with reference. The prompt from previous
versions is now explicitly passed in.

Signed-off-by: Ali Maredia <[email protected]>
Refactor RagasEvaluator Class for use for
`ilab` interface.

Signed-off-by: Ali Maredia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-failure dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants