adds basic ragas eval #193

RobotSail · 2024-12-06T16:33:51Z

This PR introduces Rubric-based evaluation through Ragas using the default with-reference rubric that they provide.

The current evaluation supports the two following modes:

Being given a dataset contains records which hold user_input (question), reference (golden answer), and response (model answer)
Being given a dataset with the user_input and reference, and additionally a ModelConfiguration of a model which will be used to generate the response for each question. This can be any model running on an OpenAI-compatible endpoint

Signed-off-by: Oleg S 97077423+RobotSail@users.noreply.github.com

Maxusmusti · 2024-12-06T20:57:56Z

requirements.txt

@@ -10,3 +10,5 @@ pandas
 pandas-stubs
 lm-eval>=0.4.4
 httpx
+
+ragas


missing newline at EOF

Maxusmusti · 2024-12-06T21:03:08Z

src/instructlab/eval/ragas.py

+    def __init__(self):
+        pass
+
+    def run(


So for this a user is expected to bring a list of Sample objects, which hold the input, prediction, and ground truth? Are we going to provide a way to build this list of Samples from given files or lists of each category, or is this moreso just for use with self-built scripts that import the Sample object and build?

Updated it such that dataset now is either a pathlib.Path object or a list of samples, and we read what we need to accordingly

Signed-off-by: Oleg S <97077423+RobotSail@users.noreply.github.com>

We want ragas to read from both a list as well as a list of samples Signed-off-by: Oleg S <97077423+RobotSail@users.noreply.github.com>

abhi1092 · 2024-12-09T21:46:09Z

@RobotSail let me know once you are done with testing the code. Other than that LGTM.

alimaredia · 2024-12-12T19:10:36Z

src/instructlab/eval/ragas.py

+
+    max_tokens: int = 768
+
+    # Random seed for reproducibility. This is not supported everywhere and therefore is unreliable.


We had discussed this earlier, I think you're going to want to remove this comment.

@alimaredia I'll update this comment because I believe you are confusing it's meaning with our earlier conversation. This comment relates to the seed not being supported by every model serving framework.

When a dataset is provided and is missing the `response` field, we will need to generate these responses. This commit ensures that when this case happens, we will error out when a student model is not configured. Otherwise, we will always generate these responses if the student model exists, regardless if `response` is in the dataframe or not. Signed-off-by: Oleg S <97077423+RobotSail@users.noreply.github.com>

RobotSail · 2024-12-13T18:07:11Z

@abhi1092 I've updated the code to have unit tests, please let me know if there was anything I missed.

src/instructlab/eval/ragas.py

abhi1092 · 2024-12-13T20:30:32Z

tests/test_ragas.py

+            api_key="test-api-key",
+        )
+        evaluator = RagasEvaluator()
+        result_df = evaluator._generate_answers_from_model(questions, student_model)


@RobotSail shouldn't we hit a mock client here too.

Instead of using gpt-3.5?

@abhi1092 We do test it with a mock-client, GPT-3.5 was just the first model that came to mind as a fill-in.

Updated this to use fake values to eliminate the possibility of it actually calling out to the real openai API.

abhi1092 · 2024-12-13T20:32:12Z

src/instructlab/eval/ragas.py

+        Given a DataFrame containing `user_input` columns, generates responses from the given model
+        and returns a new DataFrame containing its answers in the `response` column.
+        """
+        client = get_openai_client(


@RobotSail maybe this method and also the class can instead take the client as input?

@abhi1092 That's a good idea. Although the way I've done it here is how it happens in MT-Bench as well. Would it make sense to make a follow-up issue for this?

See: https://github.com/RobotSail/eval/blob/ad12276878dfa691599e37aa5e18d6c1bd7f4afd/src/instructlab/eval/mt_bench_answers.py#L120 for reference

@abhi1092 Actually I will just make this change in this PR since it's small.

Updated the PR to include this.

abhi1092 · 2024-12-13T20:35:26Z

tests/test_ragas.py

+        ########################################################################
+        # Test case: directly passing a dataset
+        ########################################################################
+        result = evaluator.run(


Here too maybe we can call mock gpt-4o with pre-defined evaluation result. This way we only test the evaluation given the student model response, reference and judge model output

@abhi1092 It is getting mocked, see: https://github.com/instructlab/eval/pull/193/files#diff-07ba3868aa4f9cd9324d6bfebe02ad4b4a3928285c0f01d29ef3d12d3f3b09b4R94

JamesKunstle

Overall just a could of questions re: function and testing simplification

JamesKunstle · 2024-12-16T20:43:18Z

src/instructlab/eval/ragas.py

+    system_prompt: str = _DEFAULT_SYSTEM_PROMPT
+
+    # "model randomness" aka likelihood of sampling something other than the likeliest token
+    temperature: float = 0.0


Very minor nit: Pydantic has an inline validation mechanism with Fields:

Suggested change

temperature: float = 0.0

temperature: float = Field(ge=0.0, le=1.0, default=0.0)

Thanks for pointing this out, this is way simpler.

JamesKunstle · 2024-12-16T20:44:18Z

src/instructlab/eval/ragas.py

+
+    def __init__(
+        self,
+        student_model: ModelConfig | None = None,


can any of these actually be None?

Yes, because the user isn't required to pass them in at initialization time.

It might streamline the code if these are required in the constructor so you don't have to do checks below. Although you might be implementing to the standard of the Evaluator class, in which case a refactor might make sense in a subsequent PR.

I see your point but it'd be fine to leave it as-is. This is also how a lot of other libraries including Ragas implement similar functionality.

JamesKunstle · 2024-12-16T20:47:01Z

src/instructlab/eval/ragas.py

+        run_config = run_config if run_config else self.run_config
+        openai_client = openai_client if openai_client else self.openai_client
+
+        if not dataset:


dataset is non-optional, so this check might be redundant.

Good point, I'll fix this.

JamesKunstle · 2024-12-16T20:49:34Z

src/instructlab/eval/ragas.py

+
+        # if the student model was provided then we always generate regardless
+        if student_model:
+            if not openai_client:


could move this assertion up to the start of the function to fail earlier.

No actually we can't do this. We have to first compute whether or not we'll need to generate questions, so there's nothing that can be skipped when computing this.

JamesKunstle · 2024-12-16T20:55:23Z

src/instructlab/eval/ragas.py

+                timeouts. This is because by default, OpenAI tier-1 usage accounts have very high
+                rate limits resulting in heavy throttling during evaluations.
+            openai_client (openai.Client | None, optional):
+                The client to use when generating questions from the student model, must be compatible with the OpenAI API.


Is this the client for the student model or the judge model?

This is for the student model. In this PR we are making the opinionated stance that the judge model needs to be GPT-4o for consistent results.

JamesKunstle · 2024-12-16T20:59:38Z

tests/test_ragas.py

+    @patch("instructlab.eval.ragas.evaluate")
+    @patch.object(RagasEvaluator, "_generate_answers_from_model")
+    @patch.object(RagasEvaluator, "_get_metrics")
+    def test_run(


Could this test that checks multiple things be broken down into separate tests?

cfchase · 2024-12-19T17:00:08Z

src/instructlab/eval/ragas.py

+        updated_df["response"] = ""
+
+        for i, qna in updated_df.iterrows():
+            messages = [


Should this be an array of dictionaries {"role": .., "content": ...}. ?
e.g.

messages = [ {"role": "system", "content": student_model.system_prompt}, {"role": "user", "content": qna["user_input"]}, ]

Good point, updating

Signed-off-by: Oleg S <97077423+RobotSail@users.noreply.github.com>

mergify bot added dependencies Pull requests that update a dependency file ci-failure labels Dec 6, 2024

RobotSail force-pushed the add-ragas branch from 04fe42a to dae91b6 Compare December 6, 2024 20:05

mergify bot added ci-failure and removed ci-failure labels Dec 6, 2024

RobotSail force-pushed the add-ragas branch from dae91b6 to 46ec02f Compare December 6, 2024 20:48

mergify bot removed the ci-failure label Dec 6, 2024

RobotSail requested review from danmcp, JamesKunstle and Maxusmusti December 6, 2024 20:55

Maxusmusti reviewed Dec 6, 2024

View reviewed changes

requirements.txt Outdated

@@ -10,3 +10,5 @@ pandas

pandas-stubs

lm-eval>=0.4.4

httpx

ragas

Copy link

Maxusmusti Dec 6, 2024

Choose a reason for hiding this comment

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

missing newline at EOF

Maxusmusti reviewed Dec 6, 2024

View reviewed changes

RobotSail added 2 commits December 6, 2024 16:55

adds basic ragas eval

5eb2310

Signed-off-by: Oleg S <97077423+RobotSail@users.noreply.github.com>

feat: add ability for ragas to read from a list

df441c1

We want ragas to read from both a list as well as a list of samples Signed-off-by: Oleg S <97077423+RobotSail@users.noreply.github.com>

RobotSail force-pushed the add-ragas branch from 46ec02f to df441c1 Compare December 6, 2024 22:07

mergify bot added ci-failure and removed ci-failure labels Dec 6, 2024

RobotSail force-pushed the add-ragas branch from f170a64 to 7538561 Compare December 7, 2024 03:23

mergify bot removed the ci-failure label Dec 7, 2024

RobotSail force-pushed the add-ragas branch 2 times, most recently from f12fbd7 to f0db4a4 Compare December 7, 2024 03:50

RobotSail requested a review from abhi1092 December 9, 2024 20:04

alimaredia reviewed Dec 12, 2024

View reviewed changes

RobotSail force-pushed the add-ragas branch from f0db4a4 to 3a9e3f2 Compare December 13, 2024 13:34

mergify bot added ci-failure testing Relates to testing and removed ci-failure labels Dec 13, 2024

RobotSail force-pushed the add-ragas branch from 8c5d66a to 0cd0d9d Compare December 13, 2024 18:06

abhi1092 reviewed Dec 13, 2024

View reviewed changes

RobotSail force-pushed the add-ragas branch from 0cd0d9d to e00d881 Compare December 13, 2024 20:53

JamesKunstle reviewed Dec 16, 2024

View reviewed changes

cfchase reviewed Dec 19, 2024

View reviewed changes

chore: add unit tests for ragas evaluator

d581e7d

Signed-off-by: Oleg S <97077423+RobotSail@users.noreply.github.com>

RobotSail force-pushed the add-ragas branch from a157203 to d581e7d Compare January 2, 2025 17:35

mergify bot added the ci-failure label Jan 2, 2025


		max_tokens: int = 768

		# Random seed for reproducibility. This is not supported everywhere and therefore is unreliable.

	temperature: float = 0.0
	temperature: float = Field(ge=0.0, le=1.0, default=0.0)

adds basic ragas eval #193

Are you sure you want to change the base?

adds basic ragas eval #193

Conversation

RobotSail commented Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhi1092 commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobotSail commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesKunstle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfchase Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobotSail commented Dec 6, 2024 •

edited

Loading

cfchase Dec 19, 2024 •

edited

Loading