From 16b8722d26021112b7e1187e676c4b1d855ce8b4 Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Fri, 10 Jul 2026 17:18:34 +0700 Subject: [PATCH] fix(gooddata-eval): delete metrics created during agentic eval runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Metric-skill and conversation evaluators create metrics in a shared, persistent eval workspace but never removed them. A leftover metric is detected and reused by a later test (the agent returns `SELECT {existing_id}` instead of full MAQL), so the assertion fails — and it makes some conversation metric-turns produce no create_metric call at all. Delete each metric a run creates on the way out, keyed on the exact metric_id from the create_metric tool result, in a finally block: - metric_skill: per-run cleanup in _execute_single_metric_run. - conversation: collect metric_ids across turns, delete after the conversation completes (deferred so a later turn's $ref still resolves). Mirrors the existing alert_skill._delete_alert pattern. Parallelism-safe: each run deletes only the id it created, so concurrent tests sharing the workspace never touch each other's objects. No version bump. JIRA: QA-28379 risk: nonprod --- .../gooddata_eval/core/agentic/alert_skill.py | 16 ++- .../core/agentic/conversation.py | 14 ++ .../core/agentic/metric_skill.py | 109 ++++++++++++---- .../tests/test_agentic_conversation.py | 122 ++++++++++++++++++ .../tests/test_agentic_metric_skill.py | 80 ++++++++++++ 5 files changed, 309 insertions(+), 32 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py index c3a9df936..54a024269 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py +++ b/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() diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py index 5ae5668c5..ecbec12a7 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/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} diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py index 2fca3eb5b..9e8eb97cb 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py +++ b/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) diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index c886a6ba9..119146d3c 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/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") diff --git a/packages/gooddata-eval/tests/test_agentic_metric_skill.py b/packages/gooddata-eval/tests/test_agentic_metric_skill.py index 90829b06d..67a163e92 100644 --- a/packages/gooddata-eval/tests/test_agentic_metric_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_metric_skill.py @@ -2,9 +2,11 @@ # SPDX-License-Identifier: LicenseRef-GoodData-Enterprise from unittest.mock import MagicMock, patch +import pytest from gooddata_eval.core.agentic.metric_skill import ( AgenticMetricSummary, MetricRunResult, + _delete_metric, _normalize_maql, run_agentic_metric_skill, ) @@ -144,3 +146,81 @@ def test_run_agentic_metric_skill_creates_fresh_conversations_for_remaining_runs # Runs 1 and 2 always create fresh; run 0 uses existing-conv assert mock_client.create_conversation.call_count == 2 assert mock_client.delete_conversation.call_count == 2 + + +def test_delete_metric_uses_sdk_entities_api(): + sdk = MagicMock() + _delete_metric(sdk, "ws1", "foo_metric") + sdk._client.entities_api.delete_entity_metrics.assert_called_once_with("ws1", "foo_metric") + + +def test_delete_metric_swallows_failures(): + sdk = MagicMock() + sdk._client.entities_api.delete_entity_metrics.side_effect = RuntimeError("500") + # Cleanup is best-effort — a failed delete is logged, never propagated. + _delete_metric(sdk, "ws1", "foo_metric") + + +def _create_metric_chat_result(metric_id: str = "foo_metric"): + return ChatResult.model_validate( + { + "textResponse": "done", + "toolCallEvents": [ + { + "functionName": "create_metric", + "functionArguments": "{}", + "result": f'{{"data": {{"maql": "SELECT {{metric/foo}}", "metric_id": "{metric_id}"}}}}', + } + ], + "reasoningStepCount": 1, + } + ) + + +def test_run_agentic_metric_skill_deletes_created_metric(): + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.return_value = _create_metric_chat_result() + with ( + patch("gooddata_eval.core.agentic.metric_skill.ChatClient", return_value=mock_client), + patch("gooddata_eval.core.agentic.metric_skill.GoodDataSdk") as mock_sdk_cls, + ): + mock_sdk = mock_sdk_cls.create.return_value + run_agentic_metric_skill( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + question="Create metric foo", + expected_output={"maql": "SELECT {metric/foo}"}, + k=1, + max_iterations=1, + ) + # The metric the run created is deleted on the way out, by its exact id, via the SDK. + mock_sdk._client.entities_api.delete_entity_metrics.assert_called_once_with("ws1", "foo_metric") + + +def test_run_agentic_metric_skill_deletes_metric_even_when_teardown_fails(): + # A metric is created, then conversation teardown raises; the created metric must still + # have been cleaned up (its deletion happens inside the per-run finally, before teardown). + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.return_value = _create_metric_chat_result() + mock_client.delete_conversation.side_effect = RuntimeError("teardown boom") + + with ( + patch("gooddata_eval.core.agentic.metric_skill.ChatClient", return_value=mock_client), + patch("gooddata_eval.core.agentic.metric_skill.GoodDataSdk") as mock_sdk_cls, + pytest.raises(RuntimeError), + ): + mock_sdk = mock_sdk_cls.create.return_value + run_agentic_metric_skill( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + question="Create metric foo", + expected_output={"maql": "SELECT {metric/foo}"}, + k=1, + max_iterations=1, + ) + + mock_sdk._client.entities_api.delete_entity_metrics.assert_called_once_with("ws1", "foo_metric")