Merge pull request #1685 from gooddata/QA-28379-eval-metric-cleanup

fix(gooddata-eval): delete metrics created during agentic eval runs

Author: myhoai

packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py

@@ -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
@@ -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}")
 
@@ -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
@@ -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()

packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py

@@ -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 (
@@ -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:
@@ -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,
@@ -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}

packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py

@@ -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
 
@@ -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
+
+
+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:
+        print(f"[CLEANUP] Failed to delete metric {metric_id}: {exc}")
+
+
 def _is_asking_clarification(text: str) -> bool:
     if not text:
         return False
@@ -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.
+    """
     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
+            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(
@@ -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
@@ -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)

packages/gooddata-eval/tests/test_agentic_conversation.py

@@ -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,
@@ -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",
@@ -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")