feat(gooddata-eval): register knowledge_question evaluator - #1763
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe evaluator registry adds ChangesKnowledge question evaluator support
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change enables evaluation of knowledge questions instead of skipping them; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant get_evaluator
participant GeneralQuestionEvaluator
participant LLMJudge
get_evaluator->>GeneralQuestionEvaluator: resolve knowledge_question
GeneralQuestionEvaluator->>LLMJudge: evaluate answer
LLMJudge-->>GeneralQuestionEvaluator: return score
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1763 +/- ##
==========================================
+ Coverage 81.58% 81.60% +0.02%
==========================================
Files 275 275
Lines 19863 19863
==========================================
+ Hits 16205 16210 +5
+ Misses 3658 3653 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
hkad98
left a comment
hkad98
left a comment
There was a problem hiding this comment.
Approving.
Verified the two load-bearing claims in the new comment:
ItemReport.test_kindis populated fromitem.test_kind(runner.py:112), not from the evaluator class, so registering one class under two keys does not mislabel results.GeneralQuestionEvaluator's rubric (_EVALUATION_STEPS) is generic free-text scoring with no LDM- or workspace-specific wording, so it genuinely transfers to platform/product/policy facts.
The lazy-registration path keys off the explicit _LAZY_EVALUATOR_* dicts rather than ev.test_kind, so the double registration is safe as written.
Three non-blocking nits for whenever you next touch this file:
GeneralQuestionEvaluator.test_kind = "general_question"is now inaccurate for half its registrations. Harmless today, but it would mislabel if anything ever derives output labels fromevaluator.test_kindinstead ofitem.test_kind. Worth a pointer from the class to the comment you added here.- There is no
agentic_knowledge_questionbranch in_dispatch_agentic(cli/agentic_runner.py), so the kind is single-turn only. Fine as scope, just flagging it as the likely next request. - Pre-existing:
supported_test_kinds()'s docstring still lists only "(general_question, guardrail)" and is now two kinds out of date.
…ual test_kind use Addresses two of hkad98's non-blocking nits on #1763: - supported_test_kinds()'s docstring only listed (general_question, guardrail) as the LLM-judge kinds; dashboard_summary and knowledge_question were already excluded by the same check but never mentioned. - GeneralQuestionEvaluator.test_kind is now inaccurate for half its registrations (knowledge_question shares the class) -- added a pointer to the registry comment explaining why, so a future reader doesn't derive an output label from it without realizing. Third nit (no agentic_knowledge_question dispatch branch) left as scope, per the review comment's own "fine as scope, likely next request."
|
@hkad98 thanks for the approval and the nits — fixed the first two in 068c393:
Left the third ( |
knowledge_question had reclassified fixtures (commit 658b9ea6) but no registered evaluator, so get_evaluator() raised KeyError and every item was silently skipped -- data/test_kinds.yaml had to keep it disabled. Reuses GeneralQuestionEvaluator directly: both are free-text-rubric, LLM-judged prose answers, and ItemReport.test_kind is tagged from the dataset item's own field rather than the evaluator class, so sharing one class across both kinds doesn't mislabel results.
…ual test_kind use Addresses two of hkad98's non-blocking nits on #1763: - supported_test_kinds()'s docstring only listed (general_question, guardrail) as the LLM-judge kinds; dashboard_summary and knowledge_question were already excluded by the same check but never mentioned. - GeneralQuestionEvaluator.test_kind is now inaccurate for half its registrations (knowledge_question shares the class) -- added a pointer to the registry comment explaining why, so a future reader doesn't derive an output label from it without realizing. Third nit (no agentic_knowledge_question dispatch branch) left as scope, per the review comment's own "fine as scope, likely next request."
068c393 to
f213331
Compare
|
Heads-up: #1771 landing put this into conflict after your approval. Rebased onto The only conflict was an import collision in Re-verified after the rebase: 702 passed (suite grew from 476 with #1771), |
hkad98
left a comment
hkad98
left a comment
There was a problem hiding this comment.
Re-approving — my earlier approval was dismissed by the rebase, not by anything wrong with the PR.
Both nits from 068c393b are in and are the right shape. The comment on GeneralQuestionEvaluator.test_kind says exactly the useful thing (harmless today, but don't derive a label from it without accounting for the second kind), and supported_test_kinds()'s docstring now lists all four LLM-judge kinds. Agreed on leaving agentic_knowledge_question out — that was my note as future scope, not a request.
Your read on the rebase conflict matches mine: test_text_evaluators.py import collision only (#1771's JudgeResponseError vs this PR's get_evaluator), both kept, nothing semantic. The registry change itself was untouched.
Verified against current master (fb6852d6, which now has #1757, #1762, #1772 and #1775 on top of the 45892f79 you rebased onto):
| Check | Result |
|---|---|
| Merges into current master | clean |
| Suite, merged | 713 passed |
ruff check / format --check |
clean, 85 files |
ty check |
All checks passed |
Nothing further from me.
2 participants
Summary
knowledge_questionfixtures were reclassified out ofgeneral_question(platform/product/policy Q&A that isn't LDM-grounded) but no evaluator was ever registered for the kind, soget_evaluator("knowledge_question")raisedKeyErrorand every item was silentlyskipped— the downstream repo has keptknowledge_question: falsein its test-kind config as a result.knowledge_questionin the lazy-evaluator registry, reusingGeneralQuestionEvaluatordirectly rather than adding a new class: both are free-text-rubric, LLM-judged prose answers, differing only in subject matter (LDM facts vs. platform/policy facts).ItemReport.test_kindis tagged from the dataset item's owntest_kindfield (runner.py), not from the evaluator class'stest_kindattribute, so sharing one evaluator class across both kinds does not mislabel results in reports/dashboards.Review follow-ups
Two of @hkad98's three nits are addressed:
GeneralQuestionEvaluator.test_kindis now inaccurate for half its registrations — added a pointer to the registry comment explaining why it's harmless, so nothing derives an output label from it unaware.supported_test_kinds()'s docstring listed only(general_question, guardrail); now lists all four LLM-judge kinds.The third (no
agentic_knowledge_questiondispatch branch, so the kind is single-turn only) is left as scope per the review — likely its own follow-up.Test plan
uv run pytest packages/gooddata-eval/tests/— 702 passed (rebased onto perf: take Langfuse trace linking off the eval item critical path #1771, which grew the suite)get_evaluator("knowledge_question")resolves toGeneralQuestionEvaluator; pass/fail dispatch through the reused evaluator (test_text_evaluators.py)test_run_items_routes_all_supported_kinds's exact-set assertion to includeknowledge_questionruff check/ruff format --checkcleanRebase note
Rebased onto
45892f79after #1771 landed and put this into conflict. The only clash was an import collision intest_text_evaluators.py(#1771 addedJudgeResponseError, this PR addedget_evaluator) — both kept, nothing semantic.