feat(gooddata-eval): add KDA-skill agentic evaluator - #1706
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 79.15% 79.42% +0.27%
==========================================
Files 271 272 +1
Lines 18794 19001 +207
==========================================
+ Hits 14876 15092 +216
+ Misses 3918 3909 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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
f00088c to
19bfdfb
Compare
29c6d13 to
46a27e6
Compare
33cc1de to
f6cde10
Compare
8a7de88 to
9c98501
Compare
myhoai
left a comment
There was a problem hiding this comment.
Review notes — items I'd like resolved before merge. All are scoped to this PR; nothing here disputes the agreed scope (completion-only, whole-turn E2E latency). Comments inline.
92b1cd3 to
da3aa76
Compare
Adds kda_skill.py to gooddata-eval, evaluating the chatbot's
create_key_driver_analysis/execute_key_driver_analysis tool calls against the
agent_kda_skill Langfuse dataset.
Scope is strictly completion, not field correctness:
strict_pass = triggered AND executed AND success AND turn_completed
Per-field checks (Measure/Date Attribute/Periods/Filters/Summary matching
expected values) are deferred entirely to a follow-up ticket (QA-28699)
rather than half-computed here as scores nothing reads yet.
Two things can extend a run past a single turn, each bounded to
max_iterations: the agent asks a clarifying question (a simulated user reply,
gpt-4o-mini, nudges it forward), or it calls create but not execute in the
same turn (create/execute are tracked independently across turns, so a plain
continuation nudge gives it another turn instead of scoring it as if execute
never happened).
Latency is measured directly by the harness (ChatClient times each
send_message() call around the SSE stream), not re-derived from a Langfuse
trace after the fact. Only the turn that actually completes KDA counts
toward it -- not an earlier disambiguation/continuation turn, and not the
simulated-reply's own OpenAI call. Logged as the kda_turn_wall_clock_sec
Langfuse score; combo_report.py (gdc-nas) reads it directly.
Fixes from review:
- kda_ prefix on pass_at_k/pass_power_k Langfuse scores -- unprefixed,
"pass_at_2" at k=2 collides with visualization.py's own score name, which
gdc-nas's combo_report.py.verdict() checks first when classifying a trace.
- send_message errors no longer propagate out of run_agentic_kda_skill
uncaught -- they now surface as a normal failed run, so it still gets
scored to Langfuse instead of only showing up as a bare JUnit failure.
- ChatError/TransientChatError now carry partial_result, so tool calls that
already succeeded before a later, unrelated error (e.g. a failed final
summary) aren't discarded and misreported as "KDA never triggered".
- turn_completed resets to False in the exception branch, so a crash on a
later iteration can't leave a stale True from an earlier one.
- run_agentic_kda_skill rejects k < 1 -- a bad env-driven KDA_RUN_K value
(0, a typo, negative) previously ran silently once instead of surfacing
the bad config.
- stream_ended is now set the moment the response_ended event line is
parsed, not its data line -- an event with no data payload previously
left the flag unset.
- t0 is set before opening the SSE stream, not after -- it was missing the
connection/server-setup time a caller actually waits through.
- Disambiguated-turn latency no longer double-counts every turn's time plus
the simulated-reply's own OpenAI call.
- The 6 per-field informational correctness scores and their support code
are removed -- team confirmed this PR's scope is trigger+complete only.
- KdaEvaluation.kda_triggered renamed to triggered, matching the other
three core fields (none of which carry the kda_ prefix). The Langfuse
score name kda_triggered is unchanged.
- generate_simulated_kda_response's OpenAI call now has a 30s timeout.
- kda_skill wired into the CLI's agentic dispatcher (agentic_runner.py)
and AGENTIC_TEST_KINDS, matching every other agentic skill, so it can
be run/debugged standalone via gd-eval run instead of only through
gdc-nas's tavern-e2e harness.
- dataset_name default renamed from agent_kda_skill to kda_skill,
matching the no-prefix pattern every other skill uses; no functional
change since gdc-nas always passes it explicitly.
- A few comments/docstrings that had drifted from the multi-turn behavior
corrected: KdaRunResult's docstring ("one message" -> up to
max_iterations), the turn_wall_clock_sec field comment, and
_extract_kda_calls' docstring (pairing is only guaranteed within one
turn; merging across turns is the caller's job).
- expected_output.get("Measure") now guards for non-dict shapes --
DatasetItem.expected_output on the gdc-nas side allows str/list, not
just dict, and a list-shaped item previously raised AttributeError,
silently swallowed by the broad except and disabling disambiguation
with only a WARNING.
- _DEFAULT_MAX_ITERATIONS raised from 2 to 3 -- 2 was lower than every
other agentic skill (visualization=4, alert_skill=6, metric_skill=7)
and left no room for a case that needs both disambiguation and a
create/execute turn split.
- turn_wall_clock_sec's field comment corrected again -- it accumulates
from the first create call through every turn attempted after it,
whether or not execute ever completes, not only "through the turn
that completed execute".
- kda_pass_at_k/kda_pass_power_k no longer logged to Langfuse -- matches
metric_skill/alert_skill/guardrail/search_tool/general_question, which
all compute the pair but never log it at their default k=1; nothing
reads a kda_pass_at_1 score today, and the score name shifts if k ever
changes, silently splitting any Langfuse view built on the old name.
- sse_client.py's t0 comment corrected again -- being per-attempt
excludes not just the sleep backoff between retries, but the entire
duration of any earlier failed attempt too.
- Two missing test cases added: create succeeds but execute never
arrives even after the continuation nudge (previously only tested
running out of iterations via repeated clarification, not via nudge);
and a run combining both extension paths (disambiguation, then a
create-without-execute continuation) exhausting its budget safely.
- KdaRunResult.eval renamed to evaluation -- it shadowed the eval
builtin and is part of the published surface (exported in
core/agentic/__init__.py's __all__); fixed now, before gdc-nas starts
consuming this module.
- turn_wall_clock_sec reverted to counting only the turn that actually
completed KDA (both create and execute resolved), matching this PR's
original, already-reviewed latency definition. An earlier change in
this PR summed a create-only turn's time into it on the theory that a
continuation turn (create succeeds, execute lands a turn later) is
real gen-ai processing time -- that theory doesn't match the agreed
definition: create being tracked across turns is only what lets the
harness recognize completion when execute arrives late, it was never
meant to change what latency measures.
JIRA: QA-28800
da3aa76 to
216f21d
Compare
What
Adds
kda_skill.pytogooddata-eval, evaluating the chatbot'screate_key_driver_analysis/execute_key_driver_analysistool calls against thekda_skillLangfuse dataset.Related: QA-28800 — Build E2E LLM test for KDA skill.
Scope
Scope is strictly completion, not field correctness:
Per-field checks (
Measure/Date Attribute/Analyzed Period/Reference Period/Filters/Summarywithin tolerance) are out of scope for this PR — deferred to a follow-up ticket (QA-28699).Latency / performance reporting
Latency is measured directly by the harness, not derived from a Langfuse trace after the fact.
ChatClienttimes eachsend_message()call around the SSE stream (starting before the request opens, so it includes connection setup time; excludes retry backoff).KdaRunResult.turn_wall_clock_secsums every turn from the one that first callscreateonward — including a continuation turn that only finishesexecutea turn later, since that's real gen-ai processing time too — but excludes any earlier disambiguation turn and the simulated-reply's own OpenAI call.evaluate_agentic_kda_skilllogs it as thekda_turn_wall_clock_secLangfuse score;gdc-nas'scombo_report.pyreads it directly to classify pass/failed/error — no trace re-resolution needed.The latency logged is the whole conversational turn (LLM planning + every tool call), not isolated KDA-backend execution time — gen-ai doesn't trace tool execution as its own observation, so there's no narrower point to measure from on either side of this cross-repo pair.
Disambiguation & continuation safety nets
KDA cases are designed to resolve in one turn, but two things can extend a run, each bounded to
max_iterations:createbut notexecutewithin the same turn —create/executeare tracked independently across turns, and a plain continuation nudge ("Please proceed.") gives the agent another turn to finish, instead of the run being scored as ifexecutenever happened.Also fixed in review
pass_at_k/pass_power_kLangfuse scores arekda_-prefixed — unprefixed,pass_at_2atk=2collides withvisualization.py's own score name, whichgdc-nas'scombo_report.py.verdict()checks first.ChatError/TransientChatErrorraised mid-turn no longer propagates out ofrun_agentic_kda_skilluncaught — it now surfaces as a normal failed run, so it still gets scored to Langfuse.ChatError/TransientChatErrornow carrypartial_result, so tool calls that already succeeded before a later, unrelated error (e.g. the final summary failing) aren't discarded and misreported as "KDA never triggered".turn_completedresets toFalsein the exception branch, so a crash on a later iteration can't leave a staleTruefrom an earlier one.run_agentic_kda_skillrejectsk < 1— a bad env-drivenKDA_RUN_Kvalue previously ran silently once instead of surfacing the bad config.stream_endedis now set the moment theresponse_endedevent line is parsed, not its data line — an event with no data payload previously left the flag unset.t0now begins before opening the SSE stream, not after — it was missing the connection/server-setup time a caller actually waits through.create/executetracked independently across turns instead of required to pair from the same turn's tool calls (see "Disambiguation & continuation safety nets" above).KdaEvaluation.kda_triggeredrenamed totriggered, matching the other three core fields — the Langfuse score namekda_triggeredis unchanged.generate_simulated_kda_response's OpenAI call now has a 30s timeout.kda_skillwired into the CLI's agentic dispatcher (agentic_runner.py) andAGENTIC_TEST_KINDS, matching every other agentic skill, so it can be run/debugged standalone viagd-eval runinstead of only throughgdc-nas's tavern-e2e harness. (README'stest_kindtable isn't updated for this — it documents a different, older evaluator subsystem and has zeroagentic_*entries for any of the 6 existing agentic skills, so adding onlykda_skillthere would be misleading.)dataset_namedefault renamed fromagent_kda_skilltokda_skill, matching the no-prefix pattern every other skill uses; no functional change sincegdc-nasalways passes it explicitly.createsucceeds,executeonly completes a turn later) now has its latency summed across both turns instead of only the turn that completedexecute.expected_output.get("Measure")now guards for non-dict shapes —DatasetItem.expected_outputon thegdc-nasside allowsstr/list, not justdict, and a list-shaped item previously raisedAttributeError, silently swallowed and disabling disambiguation with only a WARNING._DEFAULT_MAX_ITERATIONSraised from 2 to 3 — 2 was lower than every other agentic skill (visualization=4,alert_skill=6,metric_skill=7) and left no room for a case needing both disambiguation and acreate/executeturn split.kda_pass_at_k/kda_pass_power_kno longer logged to Langfuse — matchesmetric_skill/alert_skill/guardrail/search_tool/general_question, which all compute the pair but never log it at their defaultk=1; nothing reads akda_pass_at_1score today, and the score name would shift ifkever changes.KdaRunResult's docstring, theturn_wall_clock_secfield comment, and_extract_kda_calls' docstring (pairing is only guaranteed within one turn; merging across turns is the caller's job).Known limitations (out of scope for this PR)
cost_usdon the KDA score is still read frompt.total_cost, whereptis the Langfuse trace picked by the default max-latency selector — not necessarily the KDA turn itself. Latency was fixed to not depend on this trace selection; cost wasn't, since this ticket's scope is latency/completion only. Candidate for a follow-up if cost accuracy on this score matters.KdaRunResult.evalshadows theevalbuiltin as an attribute name. Not renamed here sinceAlertRunResultalready ships the same field name — fixing only KDA would be inconsistent, not a real fix.Verification
No local Tiger instance available, so verified two ways:
ruff check,ruff format --checkall clean;_evaluate_runexercised directly with real SSE payloads captured from a 30-run manual stability test).from gooddata_eval.core.agentic import evaluate_agentic_kda_skill, ...) verified to resolve with no circular-import issues.Not included in this PR
gdc-nasside (shim, tavern test, fixtures, cron wiring, daily report's KDA table) — companion PR ingdc-nasunder the same QA-28800 ticket.🤖 Generated with Claude Code