DAGE-23: Add llm explainability feature#191
Conversation
There was a problem hiding this comment.
The test updates and cleanup look good. However, the LLM reasoning implementation seems too complex for what it needs. Open to discussing the reasoning behind this design.
Data persistence
This PR should be approved soon. It includes new persistence functions in data_store, which align 100% with your task but go a bit further.
- I don’t think a new writer is needed. The reasoning dump could be handled by slightly extending
data_storeandLLM_service(e.g., adding areasoningparam). See linked PR for context.
Use of dataclasses
Not sure about the choice to use a dataclass for Rating, especially after pushing to remove the Pydantic version. It’s a simple dict[int, Optional[str]].
- Why use a dataclass here?
- What advantages does it bring over Pydantic?
- Why not apply the same logic to more complex models like
Query_Rating_Context, as I proposed since the beginning, which would benefit more from structured classes?
I love dataclasses, but mixing patterns without strong reasoning, adds inconsistency. Open to hearing you.
|
Note: I keep the previous feedback, but just realized maybe you were not aware on the PR, which now implements saving / loading functions for a common json file, since it's recent (past Friday) |
| query_id: str = data_store.add_query(query_text, doc.id) | ||
| data_store.add_rating_score(query_id, doc.id, max(config.relevance_label_set)) | ||
| data_store.add_rating_score(query_id, doc.id, max(config.relevance_label_set), | ||
| "Human Reasoning: max rating score is assigned when generated query") |
There was a problem hiding this comment.
Default max rating is assigned because the query is generated by the document
| try: | ||
| score = json.loads(raw)['score'] | ||
| score_reasoning = None | ||
| if reasoning: |
There was a problem hiding this comment.
I would rename readinging to exiplaination or something like that
| ("graded", 1, "Camry is a car, so it is relevant."), | ||
| ("graded", 2, "This exactly matches the definition of 'car of the year'."), | ||
| ]) | ||
| def test_generate_score_with_reasoning_returns_explanation(scale, valid_score, explanation, example_doc): |
There was a problem hiding this comment.
<what you test>__expect__<what you expect>
| ('graded', '{"score": null}', 'Score must be 0, 1, or 2 for graded scale, got None'), | ||
| ]) | ||
| def test_generate_score_with_invalid_llm_responses_EXPECTED_value_error(scale, response_json, expected_error, example_doc): | ||
| def test_generate_score_with_invalid_llm_responses_EXPECTED_value_error(scale, response_json, expected_error, |
| for doc_id, doc_data in documents.items(): | ||
| self.add_document(doc_id, Document.model_validate(doc_data)) | ||
|
|
||
| def export_all_records_with_reasoning(self, output_path: str | Path) -> None: |
There was a problem hiding this comment.
Can we have a test for this functionality?
4 participants
Jira: https://sease.atlassian.net/browse/DAGE-23
save_llm_explanation,llm_explanation_destinationin configuration