Uh oh!
There was an error while loading. Please reload this page.
[bot] Merge master/8ad7eb3f into rel/dev - #1732
Merged
Merged
Conversation
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.
- The "continuation" mechanism itself (create/execute tracked
independently across turns, with a "Please proceed." nudge if create
succeeded without execute in the same turn) is removed entirely, not
just its latency accounting. Read gdc-nas's actual KDA skill (system
prompt + orchestration loop): create and execute are always called
together in one turn, no confirmation step -- this scenario doesn't
happen. The only real no-execute case is the org having data-sharing
with the LLM off, which removes execute_key_driver_analysis from the
tool list entirely (permanent gap, not a delayed call); no chat nudge
can work around that, so there was nothing for this mechanism to
correctly handle in the first place.
- _DEFAULT_MAX_ITERATIONS raised from 2 to 3 for a different reason than
the removed continuation mechanism: metric and analyzed-period
ambiguity can each need their own clarifying question, so a case
ambiguous on both can legitimately take 2 rounds before create is
ever called (confirmed against real test runs). Still asking after
that is the model failing to resolve the object, not KDA itself.
JIRA: QA-28800feat(gooddata-eval): add KDA-skill agentic evaluator
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## rel/dev #1732 +/- ##
===========================================
+ Coverage 79.15% 79.42% +0.26%
===========================================
Files 271 272 +1 Lines 18796 18997 +201 ===========================================
+ Hits 14878 15088 +210 + Misses 3918 3909 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
🚀 Automated PR to perform merge from master into rel/dev with changes up to 8ad7eb3 (created by https://github.com/gooddata/gooddata-python-sdk/actions/runs/31552432783).