Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
from dataclasses import dataclass
from typing import Any

from gooddata_sdk import GoodDataSdk

from gooddata_eval.core.agentic._catalog import CatalogMetricAlert
from gooddata_eval.core.chat.sse_client import ChatClient
from gooddata_eval.core.models import ToolCallEvent
Expand Down Expand Up @@ -183,11 +185,14 @@ def generate_simulated_alert_response(
return response.choices[0].message.content or ""


def _delete_alert(client: ChatClient, workspace_id: str, alert_id: str) -> None:
host = str(client._base).split("/api/")[0]
url = f"{host}/api/v1/entities/workspaces/{workspace_id}/automations/{alert_id}"
def _delete_alert(sdk: GoodDataSdk, workspace_id: str, alert_id: str) -> None:
"""Best-effort delete of an alert (automation) created during evaluation.

Uses the GoodData SDK entities API rather than reimplementing the REST call.
Failures are logged, not raised.
"""
try:
client._client.delete(url, headers=client._auth)
sdk._client.entities_api.delete_entity_automations(workspace_id, alert_id)
except Exception as exc:
print(f"[CLEANUP] Failed to delete alert {alert_id}: {exc}")

Expand Down Expand Up @@ -327,6 +332,7 @@ def run_agentic_alert_skill(
expected = _normalize_expected_output(expected_output)
run_results: list[AlertRunResult] = []
client = ChatClient(host=host, token=token, workspace_id=workspace_id)
sdk = GoodDataSdk.create(host, token)

def _run_once(conv_id: str) -> AlertRunResult:
alert_id_to_delete: str | None = None
Expand Down Expand Up @@ -375,7 +381,7 @@ def _run_once(conv_id: str) -> AlertRunResult:
)
finally:
if alert_id_to_delete:
_delete_alert(client, workspace_id, alert_id_to_delete)
_delete_alert(sdk, workspace_id, alert_id_to_delete)

try:
conv_id_0 = initial_conversation_id if initial_conversation_id is not None else client.create_conversation()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@
from dataclasses import dataclass
from typing import Literal

from gooddata_sdk import GoodDataSdk
from pydantic import BaseModel

from gooddata_eval.core.agentic.metric_skill import _delete_metric, _extract_created_metric_ids
from gooddata_eval.core.chat.sse_client import ChatClient
from gooddata_eval.core.models import ChatResult, ToolCallEvent
from gooddata_eval.core.scoring import (
Expand Down Expand Up @@ -283,11 +285,16 @@ def run_agentic_conversation(
replies before the agent produces the expected output.
"""
client = ChatClient(host=host, token=token, workspace_id=workspace_id)
sdk = GoodDataSdk.create(host, token)
turn_results: list[TurnResult] = []
turn_outputs: dict[str, dict] = {}
total_clarification_turns = 0
conversation_id: str = ""
owns_conversation = False
# Metrics created during this conversation, deleted after it completes so they do
# not persist in the (shared) workspace and get reused by a later test. Deferred to
# the end — a later turn may $ref a metric an earlier turn created.
created_metric_ids: list[str] = []

try:
if initial_conversation_id is not None:
Expand Down Expand Up @@ -335,6 +342,11 @@ def run_agentic_conversation(
if metric_data:
turn_outputs[turn.turn_id] = metric_data

# Track every metric created this turn (any turn may create one) for cleanup.
for metric_id in _extract_created_metric_ids(all_tool_calls):
if metric_id not in created_metric_ids:
created_metric_ids.append(metric_id)

turn_results.append(
TurnResult(
turn_id=turn.turn_id,
Expand All @@ -351,6 +363,8 @@ def run_agentic_conversation(
finally:
if owns_conversation and conversation_id:
client.delete_conversation(conversation_id)
for metric_id in created_metric_ids:
_delete_metric(sdk, workspace_id, metric_id)
client.close()

activated_all = {skill for tr in turn_results for skill in tr.activated_skills}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
from dataclasses import dataclass
from typing import Any

from gooddata_sdk import GoodDataSdk

from gooddata_eval.core.chat.sse_client import ChatClient
from gooddata_eval.core.models import ToolCallEvent

Expand Down Expand Up @@ -127,6 +129,41 @@ def _extract_metric_result(tool_call_events: list[ToolCallEvent]) -> dict | None
return None


def _extract_created_metric_ids(tool_call_events: list[ToolCallEvent]) -> list[str]:
"""Ids of every metric created by ``create_metric`` calls (a turn may create more than one).

Used for cleanup so no created metric leaks — unlike ``_extract_metric_result``, which
returns only the first result for MAQL evaluation. Shared with conversation evaluation.
"""
metric_ids: list[str] = []
for tc in tool_call_events:
if tc.function_name != "create_metric" or not tc.result:
continue
result_data = tc.parsed_result()
if not result_data:
continue
data = result_data.get("data", result_data)
metric_id = data.get("metric_id") if isinstance(data, dict) else None
if metric_id and metric_id not in metric_ids:
metric_ids.append(metric_id)
return metric_ids
Comment thread
coderabbitai[bot] marked this conversation as resolved.


def _delete_metric(sdk: GoodDataSdk, workspace_id: str, metric_id: str) -> None:
"""Delete a metric created during evaluation.

Eval runs share a persistent workspace, so a metric left behind is picked up by
a later test — the agent reuses it (returning ``SELECT {id}`` instead of full
MAQL) and the assertion fails. Deleting the created metric on the way out keeps
the workspace clean for the next run. Best-effort: failures are logged, not raised.
Mirrors ``alert_skill._delete_alert``.
"""
try:
sdk._client.entities_api.delete_entity_metrics(workspace_id, metric_id)
except Exception as exc:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Silence the Ruff BLE001 warning on the intentional best-effort catch.

The blind except Exception is deliberate (cleanup must never propagate), but Ruff still flags it. Add a # noqa: BLE001 with a short rationale to keep the lint clean.

🧹 Proposed fix
-    except Exception as exc:
+    except Exception as exc:  # noqa: BLE001 - best-effort cleanup, log and continue
         print(f"[CLEANUP] Failed to delete metric {metric_id}: {exc}")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
except Exception as exc:
except Exception as exc: # noqa: BLE001 - best-effort cleanup, log and continue
🧰 Tools
🪛 Ruff (0.15.20)

[warning] 143-143: Do not catch blind exception: Exception

(BLE001)

🤖 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/metric_skill.py` at
line 143, Add an inline <code># noqa: BLE001</code> comment with a brief
rationale to the intentional broad exception handler in the cleanup function
containing <code>except Exception as exc</code>, documenting that cleanup is
best-effort and must not propagate errors.

Source: Linters/SAST tools

print(f"[CLEANUP] Failed to delete metric {metric_id}: {exc}")


def _is_asking_clarification(text: str) -> bool:
if not text:
return False
Expand All @@ -136,41 +173,54 @@ def _is_asking_clarification(text: str) -> bool:

def _execute_single_metric_run(
client: ChatClient,
sdk: GoodDataSdk,
workspace_id: str,
conversation_id: str,
question: str,
expected_outputs: list[dict],
max_iterations: int,
) -> MetricRunResult:
"""Drive one full multi-turn metric-skill conversation and evaluate the result."""
"""Drive one full multi-turn metric-skill conversation and evaluate the result.

Any metric the agent creates during this run is deleted on the way out (see
``_delete_metric``) so it cannot leak into — and be reused by — a later test
sharing the workspace.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
"""
primary_expected = expected_outputs[0] if expected_outputs else {}
metric_result: dict | None = None
metric_id_to_delete: str | None = None
turns = 0
current_question = question

for _iteration in range(max_iterations):
turns += 1
chat_result = client.send_message(conversation_id, current_question)
candidate = _extract_metric_result(chat_result.tool_call_events or [])
if candidate is not None:
metric_result = candidate
break
response_text = (chat_result.text_response or "").strip()
if _is_asking_clarification(response_text):
current_question = generate_simulated_response(response_text, primary_expected)
else:
break

actual_maql = (metric_result or {}).get("maql", "")
metric_created = metric_result is not None
maql_correct, _ = _best_maql_match(actual_maql, expected_outputs) if metric_created else (False, "")
return MetricRunResult(
conversation_id=conversation_id,
metric_result=metric_result,
metric_created=metric_created,
actual_maql=actual_maql,
maql_correct=maql_correct,
total_turns=float(turns),
)
try:
for _iteration in range(max_iterations):
turns += 1
chat_result = client.send_message(conversation_id, current_question)
candidate = _extract_metric_result(chat_result.tool_call_events or [])
if candidate is not None:
metric_result = candidate
metric_id_to_delete = candidate.get("metric_id")
break
Comment thread
coderabbitai[bot] marked this conversation as resolved.
response_text = (chat_result.text_response or "").strip()
if _is_asking_clarification(response_text):
current_question = generate_simulated_response(response_text, primary_expected)
else:
break

actual_maql = (metric_result or {}).get("maql", "")
metric_created = metric_result is not None
maql_correct, _ = _best_maql_match(actual_maql, expected_outputs) if metric_created else (False, "")
return MetricRunResult(
conversation_id=conversation_id,
metric_result=metric_result,
metric_created=metric_created,
actual_maql=actual_maql,
maql_correct=maql_correct,
total_turns=float(turns),
)
finally:
if metric_id_to_delete:
_delete_metric(sdk, workspace_id, metric_id_to_delete)


def run_agentic_metric_skill(
Expand All @@ -191,12 +241,15 @@ def run_agentic_metric_skill(
expected_outputs: list[dict] = expected_output if isinstance(expected_output, list) else [expected_output]
run_results: list[MetricRunResult] = []
client = ChatClient(host=host, token=token, workspace_id=workspace_id)
sdk = GoodDataSdk.create(host, token)

try:
conv_id_0 = initial_conversation_id if initial_conversation_id is not None else client.create_conversation()
try:
run_results.append(
_execute_single_metric_run(client, conv_id_0, question, expected_outputs, max_iterations)
_execute_single_metric_run(
client, sdk, workspace_id, conv_id_0, question, expected_outputs, max_iterations
)
)
finally:
if initial_conversation_id is None: # only delete conversations we created
Expand All @@ -206,7 +259,9 @@ def run_agentic_metric_skill(
conv_id = client.create_conversation()
try:
run_results.append(
_execute_single_metric_run(client, conv_id, question, expected_outputs, max_iterations)
_execute_single_metric_run(
client, sdk, workspace_id, conv_id, question, expected_outputs, max_iterations
)
)
finally:
client.delete_conversation(conv_id)
Expand Down
122 changes: 122 additions & 0 deletions packages/gooddata-eval/tests/test_agentic_conversation.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# SPDX-License-Identifier: LicenseRef-GoodData-Enterprise
from unittest.mock import MagicMock, patch

import pytest
from gooddata_eval.core.agentic.conversation import (
ConversationFixture,
TurnDefinition,
Expand All @@ -12,6 +13,29 @@
from gooddata_eval.core.models import ToolCallEvent


def _skills_tc(*skills):
tc = MagicMock(spec=ToolCallEvent)
tc.function_name = "set_skills"
tc.parsed_arguments = lambda: {"skills": list(skills)}
return tc


def _create_metric_tc(metric_id):
tc = MagicMock(spec=ToolCallEvent)
tc.function_name = "create_metric"
tc.result = "{}" # truthy so cleanup collection processes it; content comes from parsed_result
tc.parsed_result = lambda mid=metric_id: {"data": {"metric_id": mid, "maql": "SELECT 1"}}
return tc


def _metric_turn_result(tool_calls):
r = MagicMock()
r.text_response = "done"
r.created_visualizations = None
r.tool_call_events = tool_calls
return r


def test_turn_definition_model():
t = TurnDefinition(
turn_id="t1",
Expand Down Expand Up @@ -169,3 +193,101 @@ def test_run_agentic_conversation_creates_and_deletes_conversation():
assert result.conversation_id == "new-conv"
mock_client.create_conversation.assert_called_once()
mock_client.delete_conversation.assert_called_once_with("new-conv")


def test_run_agentic_conversation_deletes_created_metrics():
mock_client = MagicMock()
mock_client.create_conversation.return_value = "conv-1"
mock_client.send_message.return_value = _metric_turn_result([_skills_tc("metric"), _create_metric_tc("foo_metric")])

fixture = ConversationFixture(
id="test-metric",
expected_skills=["metric"],
turns=[
TurnDefinition(
turn_id="t1",
message="Create a metric counting x",
expected_skill="metric",
expected_output_type="metric",
)
],
)
with (
patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=mock_client),
patch("gooddata_eval.core.agentic.conversation.GoodDataSdk") as mock_sdk_cls,
):
mock_sdk = mock_sdk_cls.create.return_value
run_agentic_conversation(
host="http://host/api/v1/actions/workspaces/ws1/ai",
token="tok",
workspace_id="ws1",
fixture=fixture,
)
# The metric created during the conversation is deleted after it completes, via the SDK.
mock_sdk._client.entities_api.delete_entity_metrics.assert_called_once_with("ws1", "foo_metric")


def _two_metric_turn_fixture():
return ConversationFixture(
id="test-multi",
expected_skills=["metric"],
turns=[
TurnDefinition(
turn_id="t1", message="Create shared", expected_skill="metric", expected_output_type="metric"
),
TurnDefinition(
turn_id="t2", message="Create extra", expected_skill="metric", expected_output_type="metric"
),
],
)


def test_run_agentic_conversation_deletes_every_unique_metric_across_turns():
mock_client = MagicMock()
mock_client.create_conversation.return_value = "conv-1"
# Turn 1 creates "shared"; turn 2 re-creates "shared" (duplicate) and adds "extra".
mock_client.send_message.side_effect = [
_metric_turn_result([_skills_tc("metric"), _create_metric_tc("shared")]),
_metric_turn_result([_skills_tc("metric"), _create_metric_tc("shared"), _create_metric_tc("extra")]),
]

with (
patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=mock_client),
patch("gooddata_eval.core.agentic.conversation.GoodDataSdk") as mock_sdk_cls,
):
mock_sdk = mock_sdk_cls.create.return_value
run_agentic_conversation(
host="http://host/api/v1/actions/workspaces/ws1/ai",
token="tok",
workspace_id="ws1",
fixture=_two_metric_turn_fixture(),
)

# Metrics from all turns are cleaned up, and each unique id is deleted exactly once.
deleted = sorted(c.args for c in mock_sdk._client.entities_api.delete_entity_metrics.call_args_list)
assert deleted == [("ws1", "extra"), ("ws1", "shared")]


def test_run_agentic_conversation_deletes_metrics_even_when_a_later_turn_raises():
mock_client = MagicMock()
mock_client.create_conversation.return_value = "conv-1"
# Turn 1 creates "m1"; turn 2 blows up mid-run — the finally must still clean up "m1".
mock_client.send_message.side_effect = [
_metric_turn_result([_skills_tc("metric"), _create_metric_tc("m1")]),
RuntimeError("boom"),
]

with (
patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=mock_client),
patch("gooddata_eval.core.agentic.conversation.GoodDataSdk") as mock_sdk_cls,
pytest.raises(RuntimeError),
):
mock_sdk = mock_sdk_cls.create.return_value
run_agentic_conversation(
host="http://host/api/v1/actions/workspaces/ws1/ai",
token="tok",
workspace_id="ws1",
fixture=_two_metric_turn_fixture(),
)

mock_sdk._client.entities_api.delete_entity_metrics.assert_called_once_with("ws1", "m1")
Loading
Loading