Feat: evaluate retrieved context against golden context [cog-1481] #619
Conversation
WalkthroughThis pull request makes several updates across the evaluation framework. The answer generation now conditionally includes a Changes
Sequence Diagram(s)sequenceDiagram
participant RB as run_corpus_builder
participant CBE as CorpusBuilderExecutor
participant DA as DummyAdapter
RB->>CBE: build_corpus(limit, chunk_size, chunker, load_golden_context)
CBE->>DA: load_corpus(limit, seed, load_golden_context)
DA-->>CBE: (corpus_list, qa_pair)
CBE-->>RB: (corpus_list, qa_pair)
sequenceDiagram
participant EE as EvaluationExecutor
participant DEA as DeepEvalAdapter
participant CCM as ContextCoverageMetric
EE->>DEA: evaluate_answers(answers)
DEA->>DEA: For each answer, extract golden_context if exists
DEA->>LLM: Instantiate LLMTestCase(with context)
DEA->>CCM: Invoke measure/a_measure(test_case)
CCM-->>DEA: Return coverage score
DEA-->>EE: Return evaluated results
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cognee/tests/unit/eval_framework/deepeval_adapter_test.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (30)
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: run_simple_example_test / test
- GitHub Check: Test on macos-15
- GitHub Check: run_multimedia_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: Test on macos-15
- GitHub Check: run_notebook_test / test
- GitHub Check: run_networkx_metrics_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: Test on macos-15
- GitHub Check: test
- GitHub Check: Test on macos-13
- GitHub Check: run_eval_framework_test / test
- GitHub Check: test
- GitHub Check: Test on macos-13
- GitHub Check: Test on macos-13
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: windows-latest
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Build Cognee Backend Docker App Image
- GitHub Check: run_simple_example_test
- GitHub Check: docker-compose-test
alekszievr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
cognee/eval_framework/evaluation/metrics/context_match.py (4)
9-50: Add docstrings to improve code documentation.The class and its methods lack docstrings explaining their purpose, parameters, expected behavior, and return values. This makes it harder for other developers to understand and use this code correctly.
Consider adding docstrings to the class and methods:
class ContextMatchMetric(SummarizationMetric): + """ + Metric to evaluate how well retrieved context matches the golden context. + + This metric extends SummarizationMetric from deepeval and compares the + retrieval context against the original context using coverage-based evaluation. + """ def measure( self, test_case, _show_indicator: bool = True, ) -> float: + """ + Measure how well the retrieved context matches the golden context. + + Args: + test_case: The test case containing context and retrieval_context + _show_indicator: Whether to show progress indicator + + Returns: + float: Score representing context match quality + """
1-6: Add a module docstring.The file lacks a module-level docstring explaining its purpose and how it fits into the broader evaluation framework.
Consider adding a module docstring:
+""" +Context matching metric for evaluating retrieval quality. + +This module provides a metric that compares retrieved contexts against golden contexts +using summarization metrics from deepeval, helping evaluate retrieval performance. +""" from deepeval.metrics import SummarizationMetric
10-14: Add type hints for the test_case parameter.The
test_caseparameter lacks a type hint, making it unclear what type of object it should be.Consider adding appropriate type hints:
def measure( self, - test_case, + test_case: LLMTestCase, _show_indicator: bool = True, ) -> float:Similarly, update the parameter type in the
a_measuremethod as well.
28-30: Consider adding logging for better observability.When calculating scores and making pass/fail determinations, it would be helpful to have logging for debugging and monitoring purposes.
Consider adding logging:
+import logging + +logger = logging.getLogger(__name__) + class ContextMatchMetric(SummarizationMetric): # ... existing code ... self.coverage_verdicts = self._generate_coverage_verdicts(mapped_test_case) self.score = self._calculate_score(ScoreType.COVERAGE) + logger.debug(f"Context match score: {self.score}, threshold: {self.threshold}") self.success = self.score >= self.threshold
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cognee/eval_framework/evaluation/metrics/context_match.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- GitHub Check: run_notebook_test / test
- GitHub Check: Test on macos-15
- GitHub Check: run_networkx_metrics_test / test
- GitHub Check: run_eval_framework_test / test
- GitHub Check: Test on macos-15
- GitHub Check: run_notebook_test / test
- GitHub Check: run_simple_example_test / test
- GitHub Check: Test on macos-13
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: Test on macos-15
- GitHub Check: run_notebook_test / test
- GitHub Check: run_multimedia_example_test / test
- GitHub Check: Test on macos-13
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: run_notebook_test / test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: windows-latest
- GitHub Check: test
- GitHub Check: test
- GitHub Check: docker-compose-test
- GitHub Check: run_simple_example_test
- GitHub Check: Build Cognee Backend Docker App Image
🔇 Additional comments (1)
cognee/eval_framework/evaluation/metrics/context_match.py (1)
16-17: Clarify the mapping of test case fields.The code maps
test_case.context[0]toinputandtest_case.retrieval_context[0]toactual_output, but it's not immediately clear why these specific mappings were chosen. This could confuse future developers.Consider adding a comment to explain the mapping logic:
mapped_test_case = LLMTestCase( + # Map golden context to input and retrieved context to actual_output + # for comparison via SummarizationMetric input=test_case.context[0], actual_output=test_case.retrieval_context[0], )
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 9573981 | Triggered | Generic Password | bc18b4d | helm/values.yaml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cognee/eval_framework/evaluation/deep_eval_adapter.py(3 hunks)cognee/eval_framework/evaluation/evaluation_executor.py(1 hunks)cognee/eval_framework/evaluation/metrics/context_coverage.py(1 hunks)cognee/eval_framework/metrics_dashboard.py(2 hunks)cognee/tests/unit/eval_framework/deepeval_adapter_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cognee/eval_framework/metrics_dashboard.py
- cognee/eval_framework/evaluation/evaluation_executor.py
- cognee/tests/unit/eval_framework/deepeval_adapter_test.py
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: run_eval_framework_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: Test on macos-15
- GitHub Check: run_notebook_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: test
- GitHub Check: Test on macos-15
- GitHub Check: Test on macos-13
- GitHub Check: Test on macos-15
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: windows-latest
- GitHub Check: test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: Test on macos-13
- GitHub Check: Test on macos-13
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: run_simple_example_test
- GitHub Check: docker-compose-test
🔇 Additional comments (4)
cognee/eval_framework/evaluation/deep_eval_adapter.py (2)
7-7: Nice addition of theContextCoverageMetricimport.No immediate issues found with this import. The path appears correct and consistent with the newly introduced metric file.
19-19: Metric integration looks good.Including
"context_coverage": ContextCoverageMetric()in theg_eval_metricsdictionary follows the established pattern for other metrics (e.g.,"correctness","EM", etc.). This should integrate cleanly into the existing evaluation framework.cognee/eval_framework/evaluation/metrics/context_coverage.py (2)
1-9: All imports appear valid.No immediate issues or conflicts detected in these import statements. This setup for the metric definitions aligns well with the
deepevalarchitecture.
55-56: Straightforward success check.Returning
self.successhere is clear and consistent, assuming it’s always set by eithermeasureora_measure. No issues found.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cognee/eval_framework/evaluation/metrics/context_coverage.py (2)
9-13: Consider adding docstrings or type hints.Adding docstrings and type hints for
measure(and other methods below) can enhance clarity and maintainability by documenting the expected types and behavior.
27-32: Reduce duplicate logic inmeasureanda_measure.The code for generating coverage verdicts, alignment verdicts, calculating the score, and deriving the
successstatus is duplicated in both methods. Consider refactoring these steps into a shared helper function to streamline maintenance and ensure consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cognee/eval_framework/evaluation/metrics/context_coverage.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (30)
- GitHub Check: Test on macos-15
- GitHub Check: run_networkx_metrics_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_multimedia_example_test / test
- GitHub Check: Test on macos-13
- GitHub Check: run_eval_framework_test / test
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: run_simple_example_test / test
- GitHub Check: Test on macos-15
- GitHub Check: run_notebook_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: Test on macos-15
- GitHub Check: test
- GitHub Check: windows-latest
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Test on macos-13
- GitHub Check: Test on macos-13
- GitHub Check: test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: test
- GitHub Check: run_notebook_test / test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: run_simple_example_test
- GitHub Check: test
- GitHub Check: Build Cognee Backend Docker App Image
- GitHub Check: docker-compose-test
🔇 Additional comments (2)
cognee/eval_framework/evaluation/metrics/context_coverage.py (2)
15-16: Potential IndexError for empty contexts.This issue was previously noted in a past review. If
test_case.contextortest_case.retrieval_contextisNoneor empty, indexing[0]can raise an error. Consider validating inputs or returning early to avoid runtime exceptions.
34-50: Overall implementation aligns with PR objectives.Your asynchronous method correctly captures the same logic flow for coverage verdict generation. Aside from the duplication concern noted above, everything looks good, and the solution meets the intent of comparing retrieved context.
|
Nice work! Tested it and I the outputs seem meaningful and useful. |
Description
Example output:
DCO Affirmation
I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin
Summary by CodeRabbit
New Features
Bug Fixes