feat(gooddata-eval): add KDA-skill agentic evaluator - #1706
Open
FrankHuynh wants to merge 4 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds an agentic KDA skill evaluator. It validates KDA results, supports clarification turns and repeated runs, integrates optional Langfuse tracing and scoring, and exposes the new API through the agentic package. ChangesAgentic KDA evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Evaluator
participant GoodDataAPI
participant OpenAI
participant Langfuse
Evaluator->>GoodDataAPI: Create conversation and send question
GoodDataAPI-->>Evaluator: Return agent messages and tool-call events
Evaluator->>OpenAI: Generate simulated clarification reply
OpenAI-->>Evaluator: Return clarification response
Evaluator->>GoodDataAPI: Send reply and collect execution result
Evaluator->>Langfuse: Discover traces and log evaluation scores
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1706 +/- ##
==========================================
+ Coverage 78.40% 78.53% +0.13%
==========================================
Files 271 272 +1
Lines 18741 18995 +254
==========================================
+ Hits 14693 14918 +225
- Misses 4048 4077 +29 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (4)
packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py (4)
55-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNormalize
expectedthe same way asactual.Line 201 passes
expected.get("Filters", []). If a dataset item contains"Filters": null,expectedisNone.json.dumps(None)produces"null", andactualis normalized to[], sofilters_correctbecomes False for a semantically empty expectation. A follow-up ticket plans to promote this field intostrict_pass, so fix the baseline now.♻️ Proposed normalization
-def _filters_match(actual: object, expected: list) -> bool: +def _filters_match(actual: object, expected: list | None) -> bool: actual = actual or [] + expected = expected or [] try: return json.dumps(actual, sort_keys=True) == json.dumps(expected, sort_keys=True) except TypeError: return False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py` around lines 55 - 60, Update _filters_match to normalize expected the same way as actual before comparing serialized values, so None is treated as an empty filter list and expected.get("Filters", []) remains semantically consistent with missing filters. Preserve the existing TypeError handling and comparison behavior for non-null values.
96-100: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFilter non-dict candidates before calling
.get.
measure_candidatescomes from the datasetexpected_output. If the list contains a non-dict element, line 98 raisesAttributeError._measure_matchesalready guards this shape at line 52 withisinstance(c, dict). Apply the same guard here.🛡️ Proposed guard
- candidates = measure_candidates if isinstance(measure_candidates, list) else [measure_candidates or {}] + raw = measure_candidates if isinstance(measure_candidates, list) else [measure_candidates or {}] + candidates = [c for c in raw if isinstance(c, dict)] candidate_desc = "; or ".join(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py` around lines 96 - 100, Update the candidate construction used by candidate_desc to filter list elements through isinstance(c, dict), matching the shape guard in _measure_matches. Ensure only dictionary candidates reach the generator expression and its .get calls, while preserving the existing fallback behavior for non-list measure_candidates.
107-111: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet
timeout=30.0on the OpenAI call.This prevents the evaluation path from waiting for the client's long default timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py` around lines 107 - 111, Update the OpenAI request in the client.chat.completions.create call to pass timeout=30.0, ensuring the evaluation path uses the explicit 30-second timeout while preserving the existing model, messages, and max_tokens arguments.
189-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the optionals directly instead of through intermediate boolean guards.
Runtime behavior is correct because
andshort-circuits. Type checkers, however, do not narrowdict | Nonethrough intermediate boolean variables. Type narrowing occurs only through direct conditions inandandifstatements. Direct narrowing improves clarity and prevents type-checker warnings when static analysis is enabled.♻️ Proposed narrowing
- success = executed and execute_result.get("success") is True + success = execute_result is not None and execute_result.get("success") is True # Informational only (see KdaEvaluation docstring) -- still computed so a follow-up # ticket can promote these to strict_pass without redoing the extraction logic. - measure_correct = kda_triggered and _measure_matches(create_args.get("measure"), expected.get("Measure")) - date_attribute_correct = kda_triggered and create_args.get("date_attribute_id") == expected.get("Date Attribute") - analyzed_period_correct = kda_triggered and create_args.get("analyzed_period") == expected.get("Analyzed Period") - reference_period_correct = kda_triggered and create_args.get("reference_period") == expected.get("Reference Period") - filters_correct = kda_triggered and _filters_match(create_args.get("filters"), expected.get("Filters", [])) + args = create_args or {} + measure_correct = kda_triggered and _measure_matches(args.get("measure"), expected.get("Measure")) + date_attribute_correct = kda_triggered and args.get("date_attribute_id") == expected.get("Date Attribute") + analyzed_period_correct = kda_triggered and args.get("analyzed_period") == expected.get("Analyzed Period") + reference_period_correct = kda_triggered and args.get("reference_period") == expected.get("Reference Period") + filters_correct = kda_triggered and _filters_match(args.get("filters"), expected.get("Filters", []))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py` around lines 189 - 201, Update the correctness calculations in the evaluation flow around kda_triggered and create_args so each optional create_args access is guarded by a direct create_args is not None condition in the same and expression. Remove reliance on the intermediate kda_triggered boolean for type narrowing, while preserving kda_triggered for reporting and the existing matching logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py`:
- Around line 266-269: Contain failures from generate_simulated_kda_response
within the clarification loop in _run_once: catch its dependency, configuration,
and API exceptions, log them through a module-level _log logger, and terminate
only the current run while preserving already-completed runs and allowing
evaluate_agentic_kda_skill to continue to Langfuse logging and final assertion.
Add the requested logging import and module-level logger.
---
Nitpick comments:
In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py`:
- Around line 55-60: Update _filters_match to normalize expected the same way as
actual before comparing serialized values, so None is treated as an empty filter
list and expected.get("Filters", []) remains semantically consistent with
missing filters. Preserve the existing TypeError handling and comparison
behavior for non-null values.
- Around line 96-100: Update the candidate construction used by candidate_desc
to filter list elements through isinstance(c, dict), matching the shape guard in
_measure_matches. Ensure only dictionary candidates reach the generator
expression and its .get calls, while preserving the existing fallback behavior
for non-list measure_candidates.
- Around line 107-111: Update the OpenAI request in the
client.chat.completions.create call to pass timeout=30.0, ensuring the
evaluation path uses the explicit 30-second timeout while preserving the
existing model, messages, and max_tokens arguments.
- Around line 189-201: Update the correctness calculations in the evaluation
flow around kda_triggered and create_args so each optional create_args access is
guarded by a direct create_args is not None condition in the same and
expression. Remove reliance on the intermediate kda_triggered boolean for type
narrowing, while preserving kda_triggered for reporting and the existing
matching logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4c9e8b3-9e51-47bc-a0f9-5d1205de4325
📒 Files selected for processing (2)
packages/gooddata-eval/src/gooddata_eval/core/agentic/__init__.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/_langfuse.py`:
- Around line 75-78: Update _ObservationListResult.list to paginate through all
observation pages instead of limiting retrieval to the first 100, using
/api/public/v2/observations with cursor pagination, io fields, and string I/O
decoding for Cloud and self-hosted v4 deployments. Preserve equivalent
pagination through the legacy /api/public/observations endpoint for self-hosted
v3, or explicitly enforce a supported-deployment constraint, so
_select_kda_trace() receives the complete observation set.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73b38461-b03f-45d1-8fc6-36e4eae5603c
📒 Files selected for processing (2)
packages/gooddata-eval/src/gooddata_eval/core/agentic/_langfuse.pypackages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py
FrankHuynh
force-pushed
the
QA-28800-kda-skill
branch
from
August 5, 2026 06:12
ed73865 to
02ac594
Compare
Add kda_skill.py: the evaluator for the KDA (Key Driver Analysis) chatbot skill. Mirrors metric_skill/alert_skill's shape (run_agentic_*/ evaluate_agentic_*/*AssertionError), with a few KDA-specific pieces: - Scope (QA-28800): strict_pass gates only completion (kda_triggered, executed, success, turn_completed), not per-field correctness or latency -- those are logged as informational-only for a follow-up ticket. - A bounded (max_iterations=2) disambiguation safety net: if the agent asks a clarifying question instead of triggering KDA, a simulated user reply (gpt-4o-mini) nudges it forward. A failure in that helper ends only the current run (contained), not the whole evaluation. - KDA-turn latency for the daily report is read from Langfuse, but the trace to read it from is selected by which one actually made the create_key_driver_analysis/execute_key_driver_analysis tool call (paginated observation lookup) -- not by picking the largest-latency trace in the session, which can pick the wrong turn when a case spans more than one (a disambiguation exchange, or a transient-retry the chat SDK does internally). - classify_kda_report_bucket() classifies each run into pass (completed, <=60s) / failed (completed, slower) / error (didn't complete) for a separate daily-report reducer to read back via three Langfuse boolean scores -- distinct from strict_pass, which continues to gate the CI assertion on completion only. _langfuse.py: add an additive Observations API (paginated) and an optional select= override on find_traces_per_conversation (default unchanged: max latency) so KDA's trace selection doesn't touch the default other skills use. JIRA: QA-28800 risk: nonprod
FrankHuynh
force-pushed
the
QA-28800-kda-skill
branch
from
August 5, 2026 09:43
02ac594 to
73f0c22
Compare
kda_skill.py shipped with zero test coverage, unlike its metric_skill/ alert_skill siblings which each have a dedicated test file -- this is what tripped codecov/patch (27.91% vs 78.30% target) on PR #1706. Covers the pure helpers, the KDA-trace selection/pagination logic, classify_kda_report_bucket, and run_agentic_kda_skill/ evaluate_agentic_kda_skill via a mocked ChatClient, mirroring the existing test_agentic_metric_skill.py/test_agentic_alert_skill.py patterns. 94% coverage on kda_skill.py. JIRA: QA-28800 risk: nonprod
metric_skill.py and alert_skill.py already thread an optional reasoning_effort through to ChatClient and the Langfuse run name/metadata (#1709); kda_skill.py predates that change and was missing it. Mirrors the same plumbing so KDA runs can request/compare reasoning effort like the other skills. JIRA: QA-28800 risk: nonprod
Verified empirically (real staging traces, light and full-KDA cases): the trace find_traces_per_conversation returns right after the chat response lands can still be mid-ingestion in Langfuse, so its .latency reads well under the true value -- e.g. 12.5s vs a settled 28.7s on the same trace. Both cases stabilized within ~20s and never moved again afterwards. Adds _settle_trace_latency (wait 25s, re-fetch the trace by id via the new _TraceAPI.get, fall back to the original on error) and calls it only when kda_triggered, right before the value is used for the report bucket and value_score. JIRA: QA-28800 risk: nonprod
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
kda_skill.pytogooddata-eval, evaluating the chatbot'screate_key_driver_analysis/execute_key_driver_analysistool calls against theagent_kda_skillLangfuse dataset.Related: QA-28800 — Build E2E LLM test for KDA skill.
Scope
Current scope is completion, not field correctness — decided with the team mid-implementation (originally the design asserted per-field correctness; narrowed to a performance/completion focus for this first pass):
Per-field checks (
Measure/Date Attribute/Analyzed Period/Reference Period/Filters/Summarywithin tolerance) are still computed and logged to Langfuse as informational scores — so a follow-up correctness ticket can promote them tostrict_passwithout redoing the extraction logic — but they do not gate pass/fail here.Disambiguation safety net
KDA cases are designed to resolve in one turn, but if the agent asks a clarifying question instead of triggering KDA directly (a metric-title collision, or a choice between the metric-id and the mathematically equivalent ad-hoc fact+SUM form of the same measure), a simulated-user reply — mirroring
alert_skill.py/metric_skill.py's existing pattern (gpt-4o-mini) — picks any acceptable candidate and continues, bounded to 2 turns. This keeps a disambiguation turn from blocking the actual thing being measured: whether KDA itself triggers and completes.Verification
No local Tiger instance available, so verified two ways:
ruff check,ruff format --check,ty check,py_compileall clean;_evaluate_runexercised directly with real SSE payloads captured from a 30-run manual stability test against the target workspace — including the exact "KDA computed correctly but the chat turn died silently" case, which correctly failsstrict_passviaturn_completed)._to_number,isinstancechecks before treating a value as a dict) added to matchalert_skill.py's existing risk tolerance for malformed tool-call payloads — not a new risk, just consistent handling.from gooddata_eval.core.agentic import evaluate_agentic_kda_skill, ...) verified to resolve with no circular-import issues after registering the new module in__init__.py.Not included in this PR
gdc-nasside (shim, tavern test, fixtures pulled from Langfuse, cron wiring) — tracked under QA-28800, to follow once this package version is released.🤖 Generated with Claude Code
Summary by CodeRabbit