From 7eb2660c33ea707b749adbe7bfacc3c31f42da4d Mon Sep 17 00:00:00 2001 From: Roman Valovic Date: Mon, 31 Aug 2026 17:11:27 +0200 Subject: [PATCH 01/15] feat(gooddata-eval): generate eval datasets from a workspace's insights `gd-eval generate` reverse-engineers a `visualization` dataset out of the charts a customer has already built: it reads the declarative analytics model, turns each visible insight's buckets, sorts and filters into an `expected_output.visualization` spec, then asks an LLM to write the analyst question that chart answers. `expected_output` is copied out of a live object rather than authored, so "answerable with the current data model" holds by construction -- the LLM only writes English. Insights that can't be expressed without guessing (derived measures, measure-level filters, uri-form attribute filters, unmapped chart types) are skipped with a printed reason, never approximated, and the run is gated on question count, shape diversity and filter coverage rather than padded to hit a minimum. Every generated question is checked against its own spec: ranking words require a real sort or ranking filter, filter words a real date or attribute filter, a breakdown clause a non-empty view_by/segment_by and vice versa. A violation is fed back once for a rewrite, then dropped -- and a drop fails the run. Output is a flat folder `gd-eval run --dataset` reads directly, plus an optional Langfuse export (`--id-prefix` for carrying items into a second dataset, since Langfuse ids are unique per project). Each written item is validated as a `DatasetItem` with a scorable AAC visualization before the command reports success. Ported from gdc-mic-ai-evaluation's `scripts/authoring/generate_from_insights.py`, with the repo-specific parent registry and CI validator replaced by `--dataset-name`/`--out` and the package's own pydantic models, and connection handling moved onto `resolve_connection` so `--profile` works. Also: two conversion bugs the original corpus never hit -- an empty "All" attribute filter or all-time date window is a no-op to drop, not a reason to skip the insight; and `rankingFilter` is understood in its singular `measure: {localIdentifier}` form as well as the plural list form. On the GoodData demo workspace those two fixes take 4 convertible insights to 15. Co-Authored-By: Claude Opus 5 (1M context) --- packages/gooddata-eval/README.md | 92 +- .../src/gooddata_eval/cli/main.py | 78 +- .../core/dataset/from_insights.py | 871 ++++++++++++++++++ .../gooddata-eval/tests/test_from_insights.py | 628 +++++++++++++ 4 files changed, 1666 insertions(+), 3 deletions(-) create mode 100644 packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py create mode 100644 packages/gooddata-eval/tests/test_from_insights.py diff --git a/packages/gooddata-eval/README.md b/packages/gooddata-eval/README.md index e5f321027..356b64989 100644 --- a/packages/gooddata-eval/README.md +++ b/packages/gooddata-eval/README.md @@ -17,6 +17,7 @@ Or install `gd-eval` as a standalone tool: |---|---| | `gd-eval run` | Run an evaluation dataset against one or more models. | | `gd-eval models` | List LLM providers and models configured in the org. | +| `gd-eval generate` | Generate a `visualization` dataset from a workspace's existing insights. | --- @@ -184,6 +185,90 @@ gd-eval models \ --- +## `gd-eval generate` + +Reverse-engineers a `visualization` dataset out of the charts a customer has already +built, so you get eval questions without hand-authoring any. Reads the workspace's +declarative analytics model (read-only), translates each visible insight's buckets, +sorts and filters into an `expected_output.visualization` spec, then asks an LLM to +write the analyst question that chart answers. Because `expected_output` is copied from +a live object rather than authored, every question is grounded in the real LDM by +construction — the LLM only writes English. + +**Setup:** host + token (read access to the workspace), and `OPENAI_API_KEY` plus the +`llm-judge` extra for the phrasing step (`uv add 'gooddata-eval[llm-judge]'`; skip both +with `--no-phrase`). + +```bash +export GOODDATA_TOKEN='your-api-token' + +# 1. see what a workspace yields before writing anything +gd-eval generate \ + --host https://your.gooddata.cloud \ + --workspace ecommerce_demo \ + --dataset-name ecommerce \ + --dry-run + +# 2. generate, phrase, validate, and export +gd-eval generate \ + --host https://your.gooddata.cloud \ + --workspace ecommerce_demo \ + --dataset-name ecommerce \ + --dashboard dash_1_returns \ + --out ./my-dataset \ + --langfuse-out out/langfuse-dataset.json + +# 3. run it +gd-eval run --host … --workspace ecommerce_demo --dataset ./my-dataset --model gpt-5.2 +``` + +`--workspace` is where insights are read from; `--dataset-name` is the `dataset_name` +written into every item (and the default output folder). + +| Flag | Effect | +|---|---| +| `--dashboard ` | restrict to insights on that dashboard (repeatable); default is the whole workspace | +| `--out ` | output folder (default `./`); this is what `gd-eval run --dataset` reads | +| `--snapshot-out` / `--snapshot-in` | save/replay the fetched model — replay needs no host, token, or network | +| `--langfuse-out ` | also write a Langfuse-importable dataset JSON | +| `--id-prefix` | prefix exported Langfuse item ids (they're unique per *project*, so re-importing an item under its original id is a 409) | +| `--no-phrase` | skip the LLM; emit mechanical `Show ` questions | +| `--phrase-model` | OpenAI model for phrasing (default `gpt-4o`) | +| `--no-viz-type` | always blank the expected chart type | +| `--min-questions` / `--min-shapes` / `--min-filtered` | quality gate, default 15, 3 and 1 | + +**The question must never contradict its own expected output.** Four rules enforce that: + +- The writer is briefed on buckets, sorts and filters only — never the insight title, + and never the chart type. Titles routinely describe intent the definition doesn't + implement ("Products by Most Items Sold" over `sorts: []`). +- Every generated question is checked against its spec, and any hit is a hard error: + ranking words (`top`, `most`, `highest`, …) require a real sort or ranking filter; + filter words (`only`, `last quarter`, `in 2025`, …) require a real date or attribute + filter; a breakdown clause requires a non-empty `view_by`/`segment_by` and vice versa; + a metric may never be broken down by itself; and no template residue (`breakdown + dimension`, `{…}`) may survive. A violation is fed back once for a rewrite, then + dropped — and a drop fails the run. +- The writer's rules are built per insight, so an insight with no `view_by` is never + asked to name a breakdown at all. +- `type` is set only when the question actually names a chart form. An insight's + `visualizationUrl` records what a human clicked, not what the question constrains. + +Everything the writer sees is a display name (`Spend Amount`, `Merchant Name`), never a +raw URI, so questions read like a person wrote them. + +**What it won't do.** Insights it can't express without guessing are skipped with a +printed reason, never approximated: derived (arithmetic/PoP) measures, measure-level +filters, `uris`-form attribute filters, unmapped chart types, hidden objects, and +insights whose title promises behaviour their definition lacks. If too few survive, the +quality gate fails the run rather than fabricating items to hit the minimum — point at +more dashboards, or lower `--min-questions`. + +Every written item is validated as a `DatasetItem` with a scorable AAC visualization +before the command reports success. + +--- + ## Dataset format A dataset is a folder of `.json` files, one per question: @@ -254,7 +339,8 @@ is the fraction of satisfied criteria. ### `[llm-judge]` — LLM-as-judge evaluators -`general_question` and `guardrail` items are scored by a GPT-4o judge. +`general_question` and `guardrail` items are scored by a GPT-4o judge, and +`gd-eval generate` uses the same package to write question text. Requires the OpenAI package and `OPENAI_API_KEY`: ```bash @@ -263,13 +349,15 @@ uv add 'gooddata-eval[llm-judge]' uv tool install 'gooddata-eval[llm-judge]' ``` -Without `[llm-judge]`, those items are **skipped**. +Without `[llm-judge]`, those items are **skipped** and `gd-eval generate` needs +`--no-phrase`. ## Exit codes | Code | Meaning | |---|---| | `0` | Run completed. Evaluation failures do **not** cause a non-zero exit. | +| `1` | `gd-eval generate` only: a quality gate failed, an item was dropped, or a written item failed validation. | | `2` | Operational error: bad connection, missing model, unreadable dataset, missing credentials. | ## Scores (in JSON report and Langfuse) diff --git a/packages/gooddata-eval/src/gooddata_eval/cli/main.py b/packages/gooddata-eval/src/gooddata_eval/cli/main.py index 1e40efd77..f40cf9daf 100644 --- a/packages/gooddata-eval/src/gooddata_eval/cli/main.py +++ b/packages/gooddata-eval/src/gooddata_eval/cli/main.py @@ -10,7 +10,7 @@ from typing import get_args import httpx -from gooddata_api_client.exceptions import ApiException +from gooddata_api_client.exceptions import ApiException, ApiTypeError from rich.console import Console from rich.table import Table @@ -18,6 +18,7 @@ from gooddata_eval.core.chat.sse_client import ChatClient from gooddata_eval.core.config import ReasoningEffort, RunConfig from gooddata_eval.core.connection import ConnectionError_, resolve_connection +from gooddata_eval.core.dataset.from_insights import generate as generate_from_insights from gooddata_eval.core.dataset.local import load_local_dataset from gooddata_eval.core.langfuse.sink import LangfuseSink from gooddata_eval.core.models import ChatResult, DatasetItem @@ -128,6 +129,59 @@ def _build_parser() -> argparse.ArgumentParser: "resolves, which may not have every skill under test enabled." ), ) + gen = sub.add_parser( + "generate", + help="Generate a visualization dataset by reverse-engineering a workspace's insights.", + ) + gen.add_argument("--host", help="GoodData host URL.") + gen.add_argument("--token", help="API token (or set GOODDATA_TOKEN).") + gen.add_argument("--profile", help="Profile name in ~/.gooddata/profiles.yaml.") + gen.add_argument("--workspace", help="Workspace id to read insights from.") + gen.add_argument( + "--dataset-name", dest="dataset_name", required=True, help="`dataset_name` written into every item." + ) + gen.add_argument("--out", help="Output folder for the dataset JSON files (default: ./<dataset-name>).") + gen.add_argument( + "--dashboard", + action="append", + default=[], + help="Restrict to insights placed on this dashboard (repeatable). Default: the whole workspace.", + ) + gen.add_argument( + "--snapshot-in", dest="snapshot_in", help="Replay a saved model snapshot instead of calling the API." + ) + gen.add_argument("--snapshot-out", dest="snapshot_out", help="Save the fetched model snapshot for later replay.") + gen.add_argument("--langfuse-out", dest="langfuse_out", help="Also write a Langfuse-importable dataset JSON here.") + gen.add_argument( + "--id-prefix", + dest="id_prefix", + default="", + help="Prefix every exported Langfuse item id. Langfuse ids are unique per PROJECT, so " + "carrying an item into a second dataset under its original id is a 409.", + ) + gen.add_argument( + "--no-phrase", dest="no_phrase", action="store_true", help="Skip the LLM step; emit mechanical questions." + ) + gen.add_argument( + "--phrase-model", dest="phrase_model", default="gpt-4o", help="OpenAI model for the phrasing step." + ) + gen.add_argument( + "--no-viz-type", dest="no_viz_type", action="store_true", help="Always blank the expected chart type." + ) + gen.add_argument( + "--min-questions", dest="min_questions", type=int, default=15, help="Fail below this many questions." + ) + gen.add_argument( + "--min-shapes", dest="min_shapes", type=int, default=3, help="Fail below this many distinct question shapes." + ) + gen.add_argument( + "--min-filtered", + dest="min_filtered", + type=int, + default=1, + help="Fail below this many questions carrying a filter.", + ) + gen.add_argument("--dry-run", dest="dry_run", action="store_true", help="Report only; write nothing.") models_cmd = sub.add_parser("models", help="List LLM providers and models configured in the org.") models_cmd.add_argument("--host", help="GoodData host URL.") models_cmd.add_argument("--token", help="API token (or set GOODDATA_TOKEN).") @@ -438,12 +492,31 @@ def on_langfuse_item_done( return _EXIT_OK +def _generate(args: argparse.Namespace) -> int: + """`gd-eval generate` -- reverse-engineer a dataset from a workspace's insights.""" + if not args.snapshot_in and not args.workspace: + print("error: generate needs --workspace, or --snapshot-in to replay a saved model.", file=sys.stderr) + return _EXIT_OPERATIONAL_ERROR + if args.out is None: + args.out = args.dataset_name + + def sdk_factory(): + from gooddata_sdk import GoodDataSdk # noqa: PLC0415 + + host, token = resolve_connection(host=args.host, token=args.token, profile=args.profile) + return GoodDataSdk.create(host, token) + + return generate_from_insights(args, sdk_factory) + + def main(argv: list[str] | None = None) -> int: args = parse_args(argv if argv is not None else sys.argv[1:]) if hasattr(args, "concurrency") and args.concurrency < 1: print("error: --concurrency must be >= 1.", file=sys.stderr) return _EXIT_OPERATIONAL_ERROR try: + if args.command == "generate": + return _generate(args) host, token = resolve_connection(host=args.host, token=args.token, profile=args.profile) if args.command == "models": return _list_models(host, token, getattr(args, "workspace", None)) @@ -472,6 +545,9 @@ def main(argv: list[str] | None = None) -> int: ValueError, httpx.HTTPError, ApiException, + # A host pointing at the UI (or any non-API endpoint) deserializes as HTML, not + # a model -- an operator error, not a bug worth a traceback. + ApiTypeError, RuntimeError, ) as e: print(f"error: {e}", file=sys.stderr) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py b/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py new file mode 100644 index 000000000..25c83e5df --- /dev/null +++ b/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py @@ -0,0 +1,871 @@ +# (C) 2026 GoodData Corporation +"""Reverse-generate `visualization` dataset items from a workspace's real insights. + +The inverse of hand-authoring: instead of writing a question and then guessing the +expected metric/dimension/filter, this reads the *existing* visualizations a customer +already built (via the read-only declarative analytics model), translates each one's +buckets/filters into an `expected_output.visualization` AAC spec, and only then asks an +LLM to write the analyst question a user would ask to get that chart back. + +`expected_output` is therefore copied out of a real object, never invented -- which is +what satisfies "answerable with the current data model" and "expected answers reference +metrics that exist in the LDM" by construction. The LLM only writes English. + +Driven by `gd-eval generate`; the functions here are importable for programmatic use. +""" + +import hashlib +import json +import os +import re +import sys +import unicodedata +from datetime import datetime, timezone +from pathlib import Path +from typing import Any + +from gooddata_eval.core.models import CreatedVisualization, DatasetItem + +# AD `visualizationUrl` -> AAC type. Explicit and exhaustive: an unmapped url raises +# `Unsupported` and the insight is skipped loudly, rather than silently degrading to an +# unscored "". Types outside the evaluator's own type map still normalize predictably +# (`x_chart` -> `X`), so keeping them is safe. +VIZ_TYPE_MAP = { + "local:area": "area_chart", + "local:bar": "bar_chart", + "local:bubble": "bubble_chart", + "local:bullet": "bullet_chart", + "local:column": "column_chart", + "local:combo": "combo_chart", + "local:combo2": "combo_chart", + "local:dependencywheel": "dependency_wheel_chart", + "local:donut": "donut_chart", + "local:funnel": "funnel_chart", + "local:headline": "headline", + "local:heatmap": "heatmap", + "local:line": "line_chart", + "local:pie": "pie_chart", + "local:pushpin": "geo_pushpin_chart", + "local:pyramid": "pyramid_chart", + "local:repeater": "repeater", + "local:sankey": "sankey_chart", + "local:scatter": "scatter_plot", + "local:table": "table", + "local:treemap": "treemap", + "local:waterfall": "waterfall_chart", + "local:xirr": "xirr", +} + +# Words that make a question *name* a chart form. `expected_output.type` is only set +# when the question actually constrains the form -- an insight's `visualizationUrl` +# records what a human clicked, not what the question asks for, so copying it in +# unconditionally scores the agent on a choice the question never made. +TYPE_WORDS = { + "area_chart": ("area chart",), + "bar_chart": ("bar chart", "bar graph"), + "bubble_chart": ("bubble chart",), + "bullet_chart": ("bullet chart",), + "column_chart": ("column chart",), + "combo_chart": ("combo chart", "combination chart"), + "donut_chart": ("donut chart", "doughnut chart"), + "funnel_chart": ("funnel chart",), + "geo_pushpin_chart": ("map", "pushpin"), + "headline": ("headline", "single number", "kpi", "big number"), + "heatmap": ("heatmap", "heat map"), + "line_chart": ("line chart", "line graph"), + "pie_chart": ("pie chart",), + "pyramid_chart": ("pyramid chart",), + "scatter_plot": ("scatter plot", "scatterplot"), + "table": ("table",), + "treemap": ("treemap", "tree map"), + "waterfall_chart": ("waterfall chart",), +} + +# AD bucket localIdentifier -> AAC bucket. The evaluator unions view_by/segment_by/ +# rows/columns into one dimension set when scoring, so an imperfect row/column split +# costs nothing. +BUCKET_MAP = { + "measures": "metrics", + "secondary_measures": "metrics", + "view": "view_by", + "attribute": "view_by", + "trend": "view_by", + "segment": "segment_by", + "stack": "segment_by", + "columns": "columns", +} + +SHAPES = ( + "single_metric_callout", + "breakdown_by_dimension", + "filtered_view", + "time_series", + "comparison", +) + +# A question may only use ranking language if the spec actually ranks, and filter +# language if the spec actually filters. Otherwise the expected output contradicts the +# question and the item punishes the agent for reading it correctly. +RANK_WORDS = re.compile(r"\b(top|bottom|most|fewest|highest|lowest|ranked|rank|limit it to)\b", re.I) +FILTER_WORDS = re.compile( + r"\b(only|excluding|exclude|filtered|restricted to|limited to|just the" + r"|last (?:year|quarter|month|week)|this (?:year|quarter|month|week)" + r"|year to date|ytd|in \d{4})\b", + re.I, +) + +# A slot the writer failed to fill: it copied the instruction instead of a real name. +PLACEHOLDER = re.compile(r"\b(breakdown|split|filter)\s+dimension\b|[{}<>]") +# The text a question breaks down by. `(?<!...)` keeps a bare "by" from matching the +# ranking phrasing ("top 5 by Spend"), which is legitimate without any dimension. +BY_CLAUSE = re.compile( + r"\b(?:broken down by|split by|grouped by|(?<!ranked )(?<!sorted )(?<!\d )by)\s+(.+?)(?:\?|$|,| for | with | in | over )", + re.I, +) +# Phrasings that deliberately assert the absence of a breakdown. +NO_BREAKDOWN = re.compile( + r"\b(?:no|without|not)\b[^?.]{0,40}?" + r"\b(?:breakdown|break(?:ing)? (?:it|them) down|split|splits|grouping|dimensions?)\b", + re.I, +) + +PHRASE_SYSTEM = ( + "You write the question a business analyst would type into a BI chat assistant to get " + "a specific chart back. You are given that chart's exact definition. Reply with the " + "question only -- no quotes, no preamble, no explanation." +) + +TEST_KIND = "visualization" + +_MAX_SLUG_LEN = 50 +_HASH_LEN = 4 + + +class Unsupported(Exception): + """This insight cannot be expressed as an AAC spec without guessing.""" + + +def _slugify(text: str) -> str: + ascii_text = unicodedata.normalize("NFKD", text).encode("ascii", "ignore").decode("ascii").lower() + slug = re.sub(r"[^a-z0-9]+", "-", ascii_text).strip("-") + if len(slug) <= _MAX_SLUG_LEN: + return slug + truncated = slug[:_MAX_SLUG_LEN] + if "-" in truncated: + truncated = truncated.rsplit("-", 1)[0] + return truncated.strip("-") + + +def mint_id(question: str, existing_ids: set[str]) -> str: + """Stable slug id for a question, with a content hash appended on collision.""" + candidate = _slugify(question) or "question" + if candidate not in existing_ids: + return candidate + return f"{candidate}-{hashlib.sha256(question.encode()).hexdigest()[:_HASH_LEN]}" + + +def list_ids(directory: Path) -> set[str]: + """Ids already present in `directory` (recursively), so new ones don't collide.""" + ids: set[str] = set() + if not Path(directory).is_dir(): + return ids + for path in Path(directory).glob("**/*.json"): + try: + raw = json.loads(path.read_text()) + except (OSError, json.JSONDecodeError): + continue + if isinstance(raw, dict) and isinstance(raw.get("id"), str): + ids.add(raw["id"]) + return ids + + +def _alias(prefix: str, uri: str, taken: set) -> str: + stem = re.sub(r"[^a-z0-9]+", "_", uri.split("/", 1)[-1].lower()).strip("_") + base = prefix + re.sub(rf"^{prefix}", "", stem)[:40] + alias, n = base, 2 + while alias in taken: + alias, n = f"{base}_{n}", n + 1 + taken.add(alias) + return alias + + +def _measure_field(measure: dict) -> dict: + """AD measure -> AAC query field. Raises `Unsupported` for anything derived.""" + definition = measure.get("definition") or {} + simple = definition.get("measureDefinition") + if simple is None: + raise Unsupported(f"derived measure ({', '.join(definition) or 'unknown'})") + if simple.get("filters"): + # Measure-level filters have no AAC `filter_by` equivalent -- they'd silently + # vanish and turn a filtered number into an unfiltered one. + raise Unsupported("measure-level filters") + identifier = (simple.get("item") or {}).get("identifier") or {} + obj_id, obj_type = identifier.get("id"), identifier.get("type") + if not obj_id or obj_type not in ("metric", "fact"): + raise Unsupported(f"unresolvable measure item ({obj_type})") + field = {"using": f"{obj_type}/{obj_id}"} + if obj_type == "fact": + field["aggregation"] = (simple.get("aggregation") or "sum").upper() + return field + + +def _granularity(raw: str) -> str: + """'GDC.time.month' -> 'MONTH'.""" + return (raw or "").rsplit(".", 1)[-1].upper() + + +def _local_id(ref) -> str | None: + """A field reference, in either the bare-string or `{"localIdentifier": ...}` form.""" + if isinstance(ref, dict): + return ref.get("localIdentifier") + return ref if isinstance(ref, str) else None + + +def _convert_filter(raw: dict, alias_of: dict) -> dict | None: + """AD filter -> AAC filter_by entry. + + Returns None for a no-op filter (AD's "All" selection), which carries no meaning to + express and must not make the whole insight unusable. Raises `Unsupported` when a + filter does mean something this can't express. + """ + if "relativeDateFilter" in raw: + f = raw["relativeDateFilter"] + if f.get("from") is None and f.get("to") is None: + return None # all-time window: no restriction to state + return { + "type": "date_filter", + "using": f"dataset/{(f.get('dataSet') or {}).get('identifier', {}).get('id', '')}", + "granularity": _granularity(f.get("granularity")), + "from": f.get("from"), + "to": f.get("to"), + } + if "absoluteDateFilter" in raw: + f = raw["absoluteDateFilter"] + return { + "type": "date_filter", + "using": f"dataset/{(f.get('dataSet') or {}).get('identifier', {}).get('id', '')}", + "from": f.get("from"), + "to": f.get("to"), + } + for key, state_key in (("positiveAttributeFilter", "include"), ("negativeAttributeFilter", "exclude")): + if key in raw: + f = raw[key] + elements = f.get("in") if "in" in f else f.get("notIn") or {} + values = elements.get("values") + if values == []: + # AD's "All" selection -- an empty exclusion restricts nothing, and an empty + # inclusion is not something a question can ask for either. + return None + if not values: + # `uris`-form element refs can't be turned back into the literal strings + # the evaluator compares on -- authoring a guessed string fails silently. + raise Unsupported(f"{key} without literal values") + label_id = (f.get("displayForm") or {}).get("identifier", {}).get("id") + if not label_id: + raise Unsupported(f"{key} without a resolvable displayForm") + return {"type": "attribute_filter", "using": f"label/{label_id}", "state": {state_key: values}} + if "rankingFilter" in raw: + f = raw["rankingFilter"] + # Both AD spellings: plural lists of local ids, and the singular object form. + measures = f.get("measures") or ([f["measure"]] if f.get("measure") else []) + measure_id = _local_id(measures[0]) if measures else None + if measure_id not in alias_of: + raise Unsupported("ranking filter over an unresolvable measure") + entry = {"type": "ranking_filter", "using": alias_of[measure_id]} + attributes = f.get("attributes") or ([f["attribute"]] if f.get("attribute") else []) + attribute_id = _local_id(attributes[0]) if attributes else None + if attribute_id in alias_of: + entry["attribute"] = alias_of[attribute_id] + entry["bottom" if f.get("operator") == "BOTTOM" else "top"] = f.get("value") + return entry + raise Unsupported(f"filter type {', '.join(raw) or 'unknown'}") + + +def _sorts(content: dict, alias_of: dict) -> list: + """AD sorts -> AAC `sort_by`. Raises `Unsupported` for an unresolvable sort.""" + out = [] + for raw in content.get("sorts") or []: + if "attributeSortItem" in raw: + item = raw["attributeSortItem"] + local_id = item.get("attributeIdentifier") + elif "measureSortItem" in raw: + item = raw["measureSortItem"] + locators = item.get("locators") or [] + local_id = next( + (loc["measureLocatorItem"].get("measureIdentifier") for loc in locators if "measureLocatorItem" in loc), + None, + ) + else: + raise Unsupported(f"sort type {', '.join(raw) or 'unknown'}") + if local_id not in alias_of: + raise Unsupported("sort over an unresolvable field") + out.append({"field": alias_of[local_id], "direction": (item.get("direction") or "desc").upper()}) + return out + + +def convert(viz: dict, date_instance_ids: set, display_names: dict | None = None) -> dict: + """Declarative visualization object -> AAC `visualization` spec. Raises `Unsupported`.""" + content = viz.get("content") or {} + url = content.get("visualizationUrl") + if url not in VIZ_TYPE_MAP: + raise Unsupported(f"unmapped visualizationUrl '{url}' -- add it to VIZ_TYPE_MAP") + spec: dict[str, Any] = { + "id": re.sub(r"[^a-z0-9_]+", "_", (viz.get("id") or "viz").lower())[:30], + "type": VIZ_TYPE_MAP[url], + "title": viz.get("title") or viz.get("id"), + "query": {"fields": {}, "filter_by": {}}, + "metrics": [], + "view_by": [], + "segment_by": [], + "columns": [], + "rows": [], + "sort_by": [], + } + fields, taken, alias_of = spec["query"]["fields"], set(), {} + + for bucket in content.get("buckets") or []: + target = BUCKET_MAP.get(bucket.get("localIdentifier")) + if target is None: + raise Unsupported(f"unknown bucket '{bucket.get('localIdentifier')}'") + for item in bucket.get("items") or []: + if "measure" in item: + measure = item["measure"] + field = _measure_field(measure) + alias = _alias("m_", field["using"], taken) + alias_of[measure.get("localIdentifier")] = alias + elif "attribute" in item: + attribute = item["attribute"] + label_id = (attribute.get("displayForm") or {}).get("identifier", {}).get("id") + if not label_id: + raise Unsupported("attribute without a resolvable displayForm") + field = {"using": f"label/{label_id}"} + alias = _alias("d_", field["using"], taken) + alias_of[attribute.get("localIdentifier")] = alias + else: + raise Unsupported(f"unknown bucket item ({', '.join(item) or 'empty'})") + fields[alias] = field + spec[target].append(alias) + + if not spec["metrics"]: + raise Unsupported("no measures") + + converted = [_convert_filter(raw, alias_of) for raw in content.get("filters") or []] + for i, entry in enumerate(f for f in converted if f is not None): + spec["query"]["filter_by"][f"f{i}"] = entry + spec["sort_by"] = _sorts(content, alias_of) + + _reject_degenerate(spec) + spec["_shape"] = classify(spec, date_instance_ids) + return spec + + +def ranks(spec: dict) -> bool: + return bool(spec["sort_by"]) or any(f.get("type") == "ranking_filter" for f in spec["query"]["filter_by"].values()) + + +def filters(spec: dict) -> bool: + return any(f.get("type") in ("date_filter", "attribute_filter") for f in spec["query"]["filter_by"].values()) + + +def _reject_degenerate(spec: dict) -> None: + """Skip insights whose title promises behaviour their definition doesn't implement. + + A chart called "Products by Most Items Sold" with `sorts: []` and `filters: []` is a + mis-specified object, not a fixture: any faithful question about its definition + contradicts its name, and any question true to its name contradicts its + `expected_output`. Excluding it is the only honest option. + """ + title = spec["title"] or "" + if RANK_WORDS.search(title) and not ranks(spec): + raise Unsupported(f"title '{title}' promises a ranking the definition has no sort/ranking filter for") + if FILTER_WORDS.search(title) and not filters(spec): + raise Unsupported(f"title '{title}' promises a filter the definition has no date/attribute filter for") + + +def classify(spec: dict, date_instance_ids: set) -> str: + """Question shape, for the coverage report. + + ponytail: first-match-wins heuristic -- a top-5 breakdown counts as `filtered_view`, + not `breakdown_by_dimension`. Good enough to prove the corpus isn't all one type; + replace with per-insight labels if the mix ever needs to be exact. + """ + dims = spec["view_by"] + spec["segment_by"] + spec["columns"] + spec["rows"] + fields = spec["query"]["fields"] + filter_types = {f.get("type") for f in spec["query"]["filter_by"].values()} + if filter_types & {"attribute_filter", "ranking_filter"}: + return "filtered_view" + if not dims: + return "single_metric_callout" + if any(field_uri(fields, a).split("/", 1)[-1].split(".", 1)[0] in date_instance_ids for a in dims): + return "time_series" + if spec["segment_by"] or len(spec["metrics"]) > 1: + return "comparison" + return "breakdown_by_dimension" + + +def fetch_snapshot(sdk, workspace_id: str) -> dict: + """Two read-only SDK calls, assembled into a replayable JSON snapshot.""" + analytics = sdk.catalog_workspace_content.get_declarative_analytics_model(workspace_id).analytics.to_dict( + camel_case=True + ) + ldm = sdk.catalog_workspace_content.get_declarative_ldm(workspace_id).ldm.to_dict(camel_case=True) + return { + "workspace_id": workspace_id, + "fetched_at": datetime.now(timezone.utc).isoformat(), + "analytics": analytics, + "date_instance_ids": sorted(di["id"] for di in ldm.get("dateInstances") or []), + "display_names": build_display_names(analytics, ldm), + } + + +def insight_ids_on(analytics: dict, dashboard_ids: list) -> set: + """Insight ids placed on the given dashboards, walking nested layout sections.""" + wanted = set(dashboard_ids) + found = set() + + def walk(node): + if isinstance(node, dict): + if node.get("type") == "insight": + identifier = (node.get("insight") or {}).get("identifier") or {} + if identifier.get("id"): + found.add(identifier["id"]) + for value in node.values(): + walk(value) + elif isinstance(node, list): + for value in node: + walk(value) + + for dashboard in analytics.get("analyticalDashboards") or []: + if dashboard.get("id") in wanted: + walk(dashboard.get("content") or {}) + return found + + +GRANULARITY_TITLES = { + "minute": "Minute", + "hour": "Hour", + "day": "Day", + "week": "Week", + "month": "Month", + "quarter": "Quarter", + "year": "Year", +} + + +def build_display_names(analytics: dict, ldm: dict) -> dict: + """`{uri: human title}` for every metric, fact, label and date dataset. + + Raw ids leak into question text otherwise ("the metric metric/m_units_sold"), which + is both unreadable and a giveaway that no analyst wrote the question. + """ + names = {} + for metric in analytics.get("metrics") or []: + names[f"metric/{metric['id']}"] = metric.get("title") or metric["id"] + for dataset in ldm.get("datasets") or []: + names[f"dataset/{dataset['id']}"] = dataset.get("title") or dataset["id"] + for fact in dataset.get("facts") or []: + names[f"fact/{fact['id']}"] = fact.get("title") or fact["id"] + for attribute in dataset.get("attributes") or []: + labels = attribute.get("labels") or [] + for label in labels: + names[f"label/{label['id']}"] = label.get("title") or label["id"] + if not labels: + # An attribute with no explicit label is referenced by its own id. + names[f"label/{attribute['id']}"] = attribute.get("title") or attribute["id"] + for instance in ldm.get("dateInstances") or []: + title = instance.get("title") or instance["id"] + names[f"dataset/{instance['id']}"] = title + for granularity in instance.get("granularities") or []: + key = granularity.lower() + names[f"label/{instance['id']}.{key}"] = f"{title} - {GRANULARITY_TITLES.get(key, granularity.title())}" + return names + + +def display_name(uri: str, display_names: dict) -> str: + """Human title for a URI, falling back to a de-slugged id.""" + if uri in display_names: + return display_names[uri] + for key, value in display_names.items(): # ids are case-inconsistent across LDM/AD + if key.lower() == uri.lower(): + return value + return uri.split("/", 1)[-1].replace(".", " - ").replace("_", " ").strip().title() + + +def _filter_phrase(f: dict, fields: dict, display_names: dict) -> str: + """One filter, in words -- never raw JSON, which the writer would copy verbatim.""" + + def resolve(alias: str) -> str: + return display_name(field_uri(fields, alias), display_names) + + if f["type"] == "date_filter": + on = display_name(f.get("using", ""), display_names) + if isinstance(f.get("from"), str): + return f"date range {f['from']} to {f['to']} on {on}" + granularity = (f.get("granularity") or "period").lower() + return ( + f"a relative {granularity} window from {f.get('from')} to {f.get('to')} " + f"({granularity}s back from the current one, 0 = current) on {on}" + ) + if f["type"] == "attribute_filter": + on = display_name(f.get("using", ""), display_names) + state = f.get("state") or {} + if state.get("include"): + return f"only these {on} values: {', '.join(state['include'])}" + return f"excluding these {on} values: {', '.join(state.get('exclude') or [])}" + if f["type"] == "ranking_filter": + n = f.get("top") or f.get("bottom") + end = "top" if "top" in f else "bottom" + within = f", ranked within {resolve(f['attribute'])}" if f.get("attribute") else "" + return f"{end} {n} by {resolve(f.get('using', ''))}{within}" + return json.dumps(f) + + +def describe(spec: dict, display_names: dict | None = None) -> str: + """The writer's brief: buckets, sorts and filters as display names. + + Deliberately excludes the insight title and the chart type. Titles describe intent + the definition often doesn't implement, and every contradiction between a generated + question and its `expected_output` traced back to one; the chart type is a UI choice + the question isn't meant to constrain. + """ + display_names = display_names or {} + fields = spec["query"]["fields"] + + def name(alias: str) -> str: + return display_name(field_uri(fields, alias), display_names) + + lines = [f"metric: {name(a)}" for a in spec["metrics"]] + lines += [f"broken down by: {name(a)}" for a in spec["view_by"] + spec["columns"] + spec["rows"]] + lines += [f"split by: {name(a)}" for a in spec["segment_by"]] + lines += [f"sorted by: {name(s['field'])}, {s['direction'].lower()}ending" for s in spec["sort_by"]] + lines += [f"filter: {_filter_phrase(f, fields, display_names)}" for f in spec["query"]["filter_by"].values()] + return "\n".join(lines) + + +def field_uri(fields: dict, alias: str) -> str: + """Resolve a bucket alias to its URI. + + A field may be `{"using": uri}` or a bare uri string (the AAC schema allows both), + and an alias may already be a uri. + """ + field = fields.get(alias) + if field is None: + return alias + return field["using"] if isinstance(field, dict) else field + + +def _dim_names(spec: dict, display_names: dict) -> list: + fields = spec["query"]["fields"] + aliases = spec["view_by"] + spec["segment_by"] + spec["columns"] + spec["rows"] + return [display_name(field_uri(fields, a), display_names) for a in aliases] + + +def _metric_names(spec: dict, display_names: dict) -> list: + fields = spec["query"]["fields"] + return [display_name(field_uri(fields, a), display_names) for a in spec["metrics"]] + + +def _normalize(text: str) -> str: + return re.sub(r"[^a-z0-9 ]+", "", text.lower()).strip() + + +def _mentions(name: str, question: str) -> bool: + """Whether `question` names `name`, tolerating plurals and word order.""" + lowered = question.lower() + tokens = [t for t in _normalize(name).split() if len(t) >= 4] + return any(t in lowered for t in tokens) if tokens else _normalize(name) in lowered + + +def contradictions(question: str, spec: dict, display_names: dict | None = None) -> list: + """Ways `question` and `spec` disagree. Any hit is a hard error, never a warning.""" + display_names = display_names or {} + problems = [] + if not ranks(spec): + hit = RANK_WORDS.search(question) + if hit: + problems.append(f"uses ranking word '{hit.group(0)}' but the chart has no sort or ranking filter") + if not filters(spec): + hit = FILTER_WORDS.search(question) + if hit: + problems.append(f"uses filter word '{hit.group(0)}' but the chart has no date or attribute filter") + + hit = PLACEHOLDER.search(question) + if hit: + problems.append(f"leaks the un-substituted placeholder '{hit.group(0)}'") + + dims = _dim_names(spec, display_names) + clause = BY_CLAUSE.search(question) + if clause: + subject = _normalize(clause.group(1)) + echoes_metric = any(subject == _normalize(m) for m in _metric_names(spec, display_names)) + if echoes_metric and not ranks(spec): + # "Show me X by X" -- the metric echoed into its own breakdown slot. Harmless + # when the chart ranks, where "by <metric>" is how you say what it ranks on. + problems.append(f"breaks down '{clause.group(1).strip()}' by itself; it is a metric, not a dimension") + elif not dims and not NO_BREAKDOWN.search(question): + problems.append(f"asks for a breakdown by '{clause.group(1).strip()}' but view_by and segment_by are empty") + elif dims and not any(_mentions(d, question) for d in dims): + # The inverse error: the chart breaks down, the question never says so. + problems.append(f"names no dimension, but the chart breaks down by {', '.join(dims)}") + return problems + + +def _rules_for(spec: dict, display_names: dict) -> str: + dims = _dim_names(spec, display_names) + segments = [display_name(field_uri(spec["query"]["fields"], a), display_names) for a in spec["segment_by"]] + lines = [ + "Write the question an analyst would ask to get exactly this chart. Rules:", + "- Name every metric listed above explicitly, using its name verbatim.", + ] + if dims: + lines.append("- Say the question is broken down by " + ", ".join(dims) + ", naming each verbatim.") + else: + lines.append( + "- This chart has NO breakdown. Ask for the metric on its own -- do not write " + "'by ...', 'broken down by ...' or 'grouped by ...' at all. Never break a metric " + "down by itself." + ) + if segments: + lines.append("- Say it is split by " + ", ".join(segments) + ".") + lines += [ + "- State every filter and sort listed above in words (time period, included values, top/bottom N).", + "- Claim NOTHING that is not listed above. If no sort or ranking is listed, do not say " + "top/bottom/most/highest/lowest/ranked. If no filter is listed, do not restrict to a " + "time period or a subset of values.", + "- Write real names only. Never emit a literal word like 'breakdown dimension', 'metric' " + "or 'dimension' as a stand-in for a name.", + "- Do not name the chart type; the assistant should infer it.", + "- Sound like a person asking a colleague, not like a chart title.", + "- One sentence.", + ] + return "\n".join(lines) + + +def phrase(specs: list, model: str, display_names: dict) -> list: + """Question per insight, or None where the writer kept contradicting the spec. + + One retry with the specific contradiction quoted back; a second failure drops the + item rather than shipping a question its own `expected_output` disagrees with. + """ + try: + from openai import OpenAI # noqa: PLC0415 + except ImportError as err: + raise ImportError( + "Question phrasing requires the llm-judge extra: uv add 'gooddata-eval[llm-judge]' (or pass --no-phrase)" + ) from err + if not os.environ.get("OPENAI_API_KEY"): + raise OSError("OPENAI_API_KEY environment variable is required for the phrasing step.") + + client = OpenAI() + questions = [] + for i, spec in enumerate(specs, 1): + messages = [ + {"role": "system", "content": PHRASE_SYSTEM}, + {"role": "user", "content": f"{describe(spec, display_names)}\n\n{_rules_for(spec, display_names)}"}, + ] + question, problems = None, [] + for _attempt in range(2): + reply = client.chat.completions.create(model=model, messages=messages) + candidate = reply.choices[0].message.content.strip().strip('"') + problems = contradictions(candidate, spec, display_names) + if not problems: + question = candidate + break + messages += [ + {"role": "assistant", "content": candidate}, + { + "role": "user", + "content": "That question " + + "; and ".join(problems) + + ". Rewrite it describing only what the definition above actually contains.", + }, + ] + if question is None: + print(f" DROP {spec['title']}: {'; '.join(problems)}", file=sys.stderr) + questions.append(question) + print(f" phrased {i}/{len(specs)}", file=sys.stderr) + return questions + + +def resolve_type(spec: dict, question: str) -> str: + """The insight's chart type, but only when the question actually names that form.""" + lowered = question.lower() + return spec["type"] if any(w in lowered for w in TYPE_WORDS.get(spec["type"], ())) else "" + + +def build(spec: dict, question: str, dataset_name: str, existing_ids: set) -> dict: + spec = {k: v for k, v in spec.items() if k != "_shape"} + spec["type"] = resolve_type(spec, question) + question_id = mint_id(question, existing_ids) + existing_ids.add(question_id) + return { + "id": question_id, + "dataset_name": dataset_name, + "test_kind": TEST_KIND, + "question": question, + "expected_output": {"visualization": spec}, + } + + +def langfuse_payload(envelopes: list, dataset: str, workspace_id: str, origin: str, id_prefix: str = "") -> dict: + """Langfuse-importable dataset JSON. + + `id_prefix` rewrites ids on export only: Langfuse item ids are unique per PROJECT, + so importing the same item into a second dataset under its original id is a 409. + """ + return { + "dataset": dataset, + "workspace": workspace_id, + "items": [ + { + "id": id_prefix + e["id"], + "input": {"question": e["question"]}, + "expected_output": e["expected_output"], + "metadata": { + "synthetic": True, + "test_kind": TEST_KIND, + "workspace": workspace_id, + "origin": origin, + }, + } + for e in envelopes + ], + } + + +def _validation_errors(envelope: dict) -> str | None: + """The envelope must load as both a DatasetItem and a scorable AAC visualization.""" + try: + DatasetItem.model_validate(envelope) + CreatedVisualization.model_validate(envelope["expected_output"]["visualization"]) + except Exception as exc: # pydantic ValidationError, or a missing key + return str(exc) + return None + + +def generate(args, sdk_factory=None) -> int: + """Run the whole generation pipeline. Returns a process exit code.""" + if args.snapshot_in: + snapshot = json.loads(Path(args.snapshot_in).read_text()) + else: + sdk = sdk_factory() + snapshot = fetch_snapshot(sdk, args.workspace) + + if args.snapshot_out: + Path(args.snapshot_out).write_text(json.dumps(snapshot, indent=2)) + + analytics = snapshot["analytics"] + date_instance_ids = set(snapshot.get("date_instance_ids") or []) + display_names = snapshot.get("display_names") or {} + visualizations = analytics.get("visualizationObjects") or [] + if args.dashboard: + keep = insight_ids_on(analytics, args.dashboard) + if not keep: + print(f"ERROR: no insights found on dashboard(s) {', '.join(args.dashboard)}", file=sys.stderr) + return 1 + visualizations = [v for v in visualizations if v.get("id") in keep] + + specs, skipped = [], [] + for viz in visualizations: + if viz.get("isHidden"): + # Hidden objects are invisible to the AI assistant's catalog search, so a + # question about one is unwinnable rather than merely hard. + skipped.append((viz.get("id"), "hidden")) + continue + try: + specs.append(convert(viz, date_instance_ids, display_names)) + except Unsupported as exc: + skipped.append((viz.get("id"), str(exc))) + + shapes: dict[str, list] = {} + for spec in specs: + shapes.setdefault(spec["_shape"], []).append(spec["title"]) + + n_filtered = sum(1 for s in specs if filters(s)) + n_ranked = sum(1 for s in specs if ranks(s)) + print( + f"workspace {snapshot['workspace_id']}: {len(visualizations)} insights read, " + f"{len(specs)} convertible, {len(skipped)} skipped" + ) + for shape in SHAPES: + print(f" {shape:<24} {len(shapes.get(shape, []))}") + print(f" with filters {n_filtered}") + print(f" with sort/ranking {n_ranked}") + for viz_id, reason in skipped: + print(f" SKIP {viz_id}: {reason}") + + failures = [] + if len(specs) < args.min_questions: + failures.append(f"only {len(specs)} questions, need >= {args.min_questions}") + if len(shapes) < args.min_shapes: + failures.append( + f"only {len(shapes)} distinct shapes ({', '.join(shapes) or 'none'}), need >= {args.min_shapes}" + ) + if n_filtered < args.min_filtered: + # With zero filtered items the eval can only punish a spurious filter, never + # confirm the agent builds a required one -- half the behaviour goes untested. + failures.append( + f"only {n_filtered} items carry a filter, need >= {args.min_filtered}; " + "point at dashboards whose insights actually filter" + ) + for failure in failures: + print(f"QUALITY GATE: {failure}", file=sys.stderr) + if failures: + print( + "Not enough real insights to build a usable dataset -- nothing is fabricated to " + "fill the gap. Point at more dashboards, or accept a smaller set with " + "--min-questions/--min-shapes.", + file=sys.stderr, + ) + + if args.dry_run: + for spec in specs: + print(f"\n[{spec['_shape']}] {spec['title']}\n{describe(spec, display_names)}") + return 1 if failures else 0 + + if args.no_phrase: + questions = [f"Show {s['title']}" for s in specs] + else: + questions = phrase(specs, args.phrase_model, display_names) + + dropped = [spec["title"] for spec, q in zip(specs, questions) if q is None] + specs, questions = zip(*[(s, q) for s, q in zip(specs, questions) if q]) if any(questions) else ([], []) + if dropped: + failures.append(f"{len(dropped)} question(s) dropped as self-contradictory: {', '.join(dropped[:5])}") + print(f"DROPPED {len(dropped)} self-contradictory question(s)", file=sys.stderr) + + out_dir = Path(args.out) + out_dir.mkdir(parents=True, exist_ok=True) + existing_ids = list_ids(out_dir) + envelopes = [build(spec, q, args.dataset_name, existing_ids) for spec, q in zip(specs, questions)] + if args.no_viz_type: + for envelope in envelopes: + envelope["expected_output"]["visualization"]["type"] = "" + + written = [] + for envelope in envelopes: + path = out_dir / f"{envelope['id']}.json" + path.write_text(json.dumps(envelope, indent=2) + "\n") + written.append(path) + print(f"wrote {len(written)} questions to {out_dir}") + + if args.langfuse_out: + origin = ( + f"AUTO-GENERATED by reverse-engineering real insights in workspace " + f"{snapshot['workspace_id']} -- expected_output copied from live " + f"visualization definitions, question text written by " + f"{'a mechanical template' if args.no_phrase else args.phrase_model}" + ) + payload = langfuse_payload(envelopes, args.dataset_name, snapshot["workspace_id"], origin, args.id_prefix) + Path(args.langfuse_out).parent.mkdir(parents=True, exist_ok=True) + Path(args.langfuse_out).write_text(json.dumps(payload, indent=2) + "\n") + print(f"wrote Langfuse dataset to {args.langfuse_out}") + + invalid = [(e["id"], err) for e in envelopes if (err := _validation_errors(e))] + if invalid: + print(f"VALIDATION FAILED for {len(invalid)} item(s):", file=sys.stderr) + for item_id, err in invalid[:5]: + print(f" {item_id}: {err}", file=sys.stderr) + return 1 + print(f"validated {len(written)}/{len(written)}") + return 1 if failures else 0 diff --git a/packages/gooddata-eval/tests/test_from_insights.py b/packages/gooddata-eval/tests/test_from_insights.py new file mode 100644 index 000000000..bf343547c --- /dev/null +++ b/packages/gooddata-eval/tests/test_from_insights.py @@ -0,0 +1,628 @@ +# (C) 2026 GoodData Corporation +import json + +import pytest +from gooddata_eval.core.dataset.from_insights import ( + Unsupported, + _validation_errors, + build, + build_display_names, + contradictions, + convert, + describe, + display_name, + insight_ids_on, + langfuse_payload, + list_ids, + mint_id, + resolve_type, +) +from gooddata_eval.core.dataset.local import load_local_dataset +from gooddata_eval.core.models import CreatedVisualization +from gooddata_eval.core.scoring import check_filters, get_metric_uri_set, validate_cross_references + +DATE_IDS = {"process_date"} + + +def viz(url, buckets, filters=(), sorts=(), **kw): + return { + "id": "v_x", + "title": kw.pop("title", "X"), + "content": {"visualizationUrl": url, "buckets": buckets, "filters": list(filters), "sorts": list(sorts)}, + **kw, + } + + +def measure(local_id, obj_id, obj_type="metric", **definition): + return { + "measure": { + "localIdentifier": local_id, + "definition": { + "measureDefinition": {"item": {"identifier": {"type": obj_type, "id": obj_id}}, **definition} + }, + } + } + + +def attribute(local_id, label_id): + return { + "attribute": {"localIdentifier": local_id, "displayForm": {"identifier": {"type": "label", "id": label_id}}} + } + + +def test_headline_converts_to_scorable_single_metric_spec(): + spec = convert( + viz("local:headline", [{"localIdentifier": "measures", "items": [measure("m", "gross_revenue")]}]), DATE_IDS + ) + assert spec["type"] == "headline" + assert spec["_shape"] == "single_metric_callout" + # The alias is arbitrary; what must survive is the URI gd-eval scores on. + parsed = CreatedVisualization(**{k: v for k, v in spec.items() if k != "_shape"}) + + assert get_metric_uri_set(parsed) == {"metric/gross_revenue"} + + +def test_breakdown_and_time_series_are_distinguished_by_date_instance(): + by_dim = convert( + viz( + "local:bar", + [ + {"localIdentifier": "measures", "items": [measure("m", "spend")]}, + {"localIdentifier": "view", "items": [attribute("a", "merchant.name")]}, + ], + ), + DATE_IDS, + ) + by_time = convert( + viz( + "local:column", + [ + {"localIdentifier": "measures", "items": [measure("m", "spend")]}, + {"localIdentifier": "view", "items": [attribute("a", "process_date.month")]}, + ], + ), + DATE_IDS, + ) + assert by_dim["_shape"] == "breakdown_by_dimension" + assert by_time["_shape"] == "time_series" + + +def test_filters_round_trip_into_scorable_filter_by(): + spec = convert( + viz( + "local:bar", + [ + {"localIdentifier": "measures", "items": [measure("m", "spend")]}, + {"localIdentifier": "view", "items": [attribute("a", "merchant.name")]}, + ], + filters=[ + { + "absoluteDateFilter": { + "dataSet": {"identifier": {"id": "process_date"}}, + "from": "2025-01-01", + "to": "2025-12-31", + } + }, + { + "positiveAttributeFilter": { + "displayForm": {"identifier": {"id": "region"}}, + "in": {"values": ["EMEA"]}, + } + }, + {"rankingFilter": {"measures": ["m"], "attributes": ["a"], "operator": "TOP", "value": 5}}, + ], + ), + DATE_IDS, + ) + assert spec["_shape"] == "filtered_view" + parsed = CreatedVisualization(**{k: v for k, v in spec.items() if k != "_shape"}) + + # Ranking-filter aliases must resolve to metric/ and label/ URIs or gd-eval rejects them. + assert validate_cross_references(parsed) == (True, []) + assert check_filters(parsed, parsed).all_ok + + +def test_relative_date_granularity_is_normalized(): + spec = convert( + viz( + "local:headline", + [{"localIdentifier": "measures", "items": [measure("m", "spend")]}], + filters=[ + { + "relativeDateFilter": { + "dataSet": {"identifier": {"id": "process_date"}}, + "granularity": "GDC.time.quarter", + "from": -1, + "to": -1, + } + } + ], + ), + DATE_IDS, + ) + assert spec["query"]["filter_by"]["f0"] == { + "type": "date_filter", + "using": "dataset/process_date", + "granularity": "QUARTER", + "from": -1, + "to": -1, + } + + +def test_fact_measure_carries_its_aggregation(): + spec = convert( + viz( + "local:headline", + [{"localIdentifier": "measures", "items": [measure("m", "amount", "fact", aggregation="sum")]}], + ), + DATE_IDS, + ) + assert spec["query"]["fields"]["m_amount"] == {"using": "fact/amount", "aggregation": "SUM"} + + +@pytest.mark.parametrize( + "bad,reason", + [ + ({"measure": {"localIdentifier": "m", "definition": {"arithmeticMeasure": {}}}}, "derived"), + ( + { + "measure": { + "localIdentifier": "m", + "definition": { + "measureDefinition": { + "item": {"identifier": {"type": "metric", "id": "x"}}, + "filters": [{"positiveAttributeFilter": {}}], + } + }, + } + }, + "measure-level", + ), + ], +) +def test_underivable_measures_are_skipped_not_guessed(bad, reason): + with pytest.raises(Unsupported, match=reason): + convert(viz("local:headline", [{"localIdentifier": "measures", "items": [bad]}]), DATE_IDS) + + +def test_uri_form_attribute_filter_is_skipped_rather_than_guessed(): + with pytest.raises(Unsupported, match="literal values"): + convert( + viz( + "local:bar", + [{"localIdentifier": "measures", "items": [measure("m", "spend")]}], + filters=[ + { + "positiveAttributeFilter": { + "displayForm": {"identifier": {"id": "region"}}, + "in": {"uris": ["/obj/1?id=2"]}, + } + } + ], + ), + DATE_IDS, + ) + + +def test_treemap_is_mapped_not_dropped(): + spec = convert(viz("local:treemap", [{"localIdentifier": "measures", "items": [measure("m", "spend")]}]), DATE_IDS) + assert spec["type"] == "treemap" + + +def test_unmapped_viz_url_fails_loudly(): + with pytest.raises(Unsupported, match="unmapped visualizationUrl"): + convert(viz("local:brandnew", [{"localIdentifier": "measures", "items": [measure("m", "spend")]}]), DATE_IDS) + + +def test_insight_ids_on_walks_nested_dashboard_layout(): + analytics = { + "analyticalDashboards": [ + { + "id": "d1", + "content": { + "layout": { + "sections": [ + { + "items": [ + {"widget": {"type": "insight", "insight": {"identifier": {"id": "v_a"}}}}, + { + "widget": { + "type": "IDashboardLayoutNested", + "sections": [ + { + "items": [ + { + "widget": { + "type": "insight", + "insight": {"identifier": {"id": "v_b"}}, + } + } + ] + } + ], + } + }, + ] + } + ] + } + }, + }, + { + "id": "d2", + "content": { + "layout": { + "sections": [ + {"items": [{"widget": {"type": "insight", "insight": {"identifier": {"id": "v_c"}}}}]} + ] + } + }, + }, + ] + } + assert insight_ids_on(analytics, ["d1"]) == {"v_a", "v_b"} + assert insight_ids_on(analytics, ["d1", "d2"]) == {"v_a", "v_b", "v_c"} + assert insight_ids_on(analytics, ["nope"]) == set() + + +def test_langfuse_payload_shape(): + envelope = {"id": "q1", "question": "How much?", "expected_output": {"visualization": {}}} + payload = langfuse_payload([envelope], "cust", "ws1", "origin note") + assert payload["dataset"] == "cust" and payload["workspace"] == "ws1" + item = payload["items"][0] + assert item["id"] == "q1" + assert item["input"] == {"question": "How much?"} + assert item["expected_output"] == {"visualization": {}} + assert item["metadata"]["origin"] == "origin note" + + +def test_built_envelope_is_loadable_as_a_dataset_item(tmp_path): + """The whole point: what this writes must be runnable by `gd-eval run` as-is.""" + + spec = convert( + viz( + "local:column", + [ + {"localIdentifier": "measures", "items": [measure("m", "spend")]}, + {"localIdentifier": "view", "items": [attribute("a", "process_date.month")]}, + ], + ), + DATE_IDS, + ) + envelope = build(spec, "How did spend trend by month?", "micai_diagnose_master", set()) + assert "_shape" not in envelope["expected_output"]["visualization"] + assert _validation_errors(envelope) is None + + (tmp_path / f"{envelope['id']}.json").write_text(json.dumps(envelope, indent=2)) + items = load_local_dataset(tmp_path) + assert [i.id for i in items] == [envelope["id"]] + assert items[0].test_kind == "visualization" + assert items[0].dataset_name == "micai_diagnose_master" + + +def test_mint_id_is_stable_and_collision_safe(): + q = "How did spend trend by month?" + assert mint_id(q, set()) == "how-did-spend-trend-by-month" + second = mint_id(q, {"how-did-spend-trend-by-month"}) + assert second.startswith("how-did-spend-trend-by-month-") and second != q + + +def test_list_ids_reads_ids_already_in_the_output_folder(tmp_path): + (tmp_path / "a.json").write_text(json.dumps({"id": "already-there"})) + (tmp_path / "broken.json").write_text("{not json") + assert list_ids(tmp_path) == {"already-there"} + + +def test_langfuse_id_prefix_applies_to_the_export_only(): + envelope = {"id": "q1", "question": "How much?", "expected_output": {"visualization": {}}} + payload = langfuse_payload([envelope], "cust", "ws1", "origin", id_prefix="loop3-") + assert payload["items"][0]["id"] == "loop3-q1" + assert envelope["id"] == "q1" + + +# --- the fixes: no title leakage, no contradictions, real sorts/filters ------ + +DISPLAY = { + "metric/spend": "Spend Amount", + "label/merchant.NAME": "Merchant Name", + "label/process_date.month": "Process Date - Month", + "dataset/process_date": "Process Date", +} + + +def spend_by_merchant(**kw): + return viz( + "local:bar", + [ + {"localIdentifier": "measures", "items": [measure("m", "spend")]}, + {"localIdentifier": "view", "items": [attribute("a", "merchant.NAME")]}, + ], + **kw, + ) + + +def test_brief_omits_title_and_chart_type_and_uses_display_names(): + spec = convert( + spend_by_merchant( + title="Top Merchants", + sorts=[ + { + "measureSortItem": { + "direction": "desc", + "locators": [{"measureLocatorItem": {"measureIdentifier": "m"}}], + } + }, + ], + ), + DATE_IDS, + ) + brief = describe(spec, DISPLAY) + assert "Top Merchants" not in brief + assert "bar" not in brief.lower() + assert "metric/spend" not in brief and "merchant.NAME" not in brief + assert "metric: Spend Amount" in brief + assert "broken down by: Merchant Name" in brief + assert "sorted by: Spend Amount, descending" in brief + + +def test_filters_are_briefed_in_words_not_json(): + spec = convert( + spend_by_merchant( + filters=[ + { + "absoluteDateFilter": { + "dataSet": {"identifier": {"id": "process_date"}}, + "from": "2025-01-01", + "to": "2025-12-31", + } + }, + {"rankingFilter": {"measures": ["m"], "attributes": ["a"], "operator": "TOP", "value": 5}}, + ] + ), + DATE_IDS, + ) + brief = describe(spec, DISPLAY) + assert "date range 2025-01-01 to 2025-12-31 on Process Date" in brief + assert "top 5 by Spend Amount, ranked within Merchant Name" in brief + assert "{" not in brief + + +def test_degenerate_titles_are_skipped_rather_than_contradicted(): + with pytest.raises(Unsupported, match="promises a ranking"): + convert(spend_by_merchant(title="Products by Most Items Sold"), DATE_IDS) + with pytest.raises(Unsupported, match="promises a filter"): + convert(spend_by_merchant(title="Spend for repeat purchases only"), DATE_IDS) + + +def test_a_real_sort_or_ranking_legitimises_a_ranking_title(): + spec = convert( + spend_by_merchant( + title="Top 5 Merchants", filters=[{"rankingFilter": {"measures": ["m"], "operator": "TOP", "value": 5}}] + ), + DATE_IDS, + ) + assert spec["_shape"] == "filtered_view" + sorted_spec = convert( + spend_by_merchant( + title="Merchants, Most Spend First", + sorts=[{"attributeSortItem": {"attributeIdentifier": "a", "direction": "asc"}}], + ), + DATE_IDS, + ) + assert sorted_spec["sort_by"] == [{"field": "d_merchant_name", "direction": "ASC"}] + + +def test_contradictions_flag_ranking_and_filter_language_the_spec_lacks(): + plain = convert(spend_by_merchant(), DATE_IDS) + assert contradictions("Which merchants drove the most spend?", plain) + assert contradictions("Show spend by merchant for last quarter", plain) + assert contradictions("How does spend break down across merchants?", plain) == [] + + ranked = convert( + spend_by_merchant(filters=[{"rankingFilter": {"measures": ["m"], "operator": "TOP", "value": 5}}]), DATE_IDS + ) + assert contradictions("What are the top 5 merchants by spend?", ranked) == [] + + +def test_type_is_kept_only_when_the_question_names_the_chart_form(): + spec = convert(spend_by_merchant(), DATE_IDS) + assert resolve_type(spec, "Show me spend by merchant as a bar chart") == "bar_chart" + assert resolve_type(spec, "Which merchants did we spend the most with?") == "" + + +def test_build_blanks_type_for_a_question_that_names_no_chart_form(): + + spec = convert(spend_by_merchant(), DATE_IDS) + envelope = build(spec, "How does spend break down across merchants?", "p", set()) + assert envelope["expected_output"]["visualization"]["type"] == "" + + +def test_display_names_cover_metrics_facts_labels_and_date_granularities(): + names = build_display_names( + {"metrics": [{"id": "m_spend", "title": "Spend Amount"}]}, + { + "datasets": [ + { + "id": "merchant", + "title": "Merchant", + "facts": [{"id": "amt", "title": "Amount"}], + "attributes": [ + {"id": "merchant.NAME", "title": "Merchant Name", "labels": []}, + { + "id": "merchant.CTRY", + "title": "Country", + "labels": [{"id": "merchant.CTRY_ISO", "title": "Country ISO"}], + }, + ], + } + ], + "dateInstances": [{"id": "process_date", "title": "Process Date", "granularities": ["MONTH", "YEAR"]}], + }, + ) + assert names["metric/m_spend"] == "Spend Amount" + assert names["fact/amt"] == "Amount" + assert names["label/merchant.NAME"] == "Merchant Name" + assert names["label/merchant.CTRY_ISO"] == "Country ISO" + assert names["label/process_date.month"] == "Process Date - Month" + assert names["dataset/process_date"] == "Process Date" + # No raw id ever reaches question text, even for something the LDM didn't name. + assert display_name("metric/m_units_sold", names) == "M Units Sold" + + +# --- breakdown clause must match the spec's actual dimensions --------------- + + +def test_metric_echoed_as_its_own_dimension_is_a_hard_error(): + headline = convert( + viz("local:headline", [{"localIdentifier": "measures", "items": [measure("m", "spend")]}]), DATE_IDS + ) + assert contradictions("Can you show me Spend Amount by Spend Amount?", headline, DISPLAY) + assert contradictions("Can you show me Spend Amount broken down by Spend Amount?", headline, DISPLAY) + assert contradictions("Can you show me Spend Amount?", headline, DISPLAY) == [] + + +def test_unsubstituted_placeholder_is_a_hard_error(): + headline = convert( + viz("local:headline", [{"localIdentifier": "measures", "items": [measure("m", "spend")]}]), DATE_IDS + ) + for bad in ( + "Can you show me Spend Amount by breakdown dimension?", + "Can you show me Spend Amount by {dimension}?", + "Can you show me Spend Amount by <split dimension>?", + ): + assert contradictions(bad, headline, DISPLAY), bad + + +def test_breakdown_promised_but_not_expected(): + headline = convert( + viz("local:headline", [{"localIdentifier": "measures", "items": [measure("m", "spend")]}]), DATE_IDS + ) + assert contradictions("Can you show me Spend Amount by Merchant Name?", headline, DISPLAY) + # Explicit negation is legitimate phrasing, not a contradiction. + for ok in ( + "Can you show me Spend Amount with no breakdown?", + "Can you show me Spend Amount without breaking it down by any dimension?", + ): + assert contradictions(ok, headline, DISPLAY) == [], ok + + +def test_breakdown_expected_but_not_asked_is_the_same_severity(): + spec = convert(spend_by_merchant(), DATE_IDS) + assert contradictions("Can you show me Spend Amount?", spec, DISPLAY) + assert contradictions("Can you show me Spend Amount by Merchant Name?", spec, DISPLAY) == [] + # Plurals and reordering still count as naming the dimension. + assert contradictions("How does Spend Amount break down across merchants?", spec, DISPLAY) == [] + + +def test_ranking_phrasing_without_a_dimension_is_not_a_false_breakdown(): + ranked = convert( + viz( + "local:headline", + [{"localIdentifier": "measures", "items": [measure("m", "spend")]}], + filters=[{"rankingFilter": {"measures": ["m"], "operator": "TOP", "value": 5}}], + ), + DATE_IDS, + ) + assert contradictions("What is the top 5 by Spend Amount?", ranked, DISPLAY) == [] + + +def test_every_reported_malformed_question_is_caught(): + """The 7 real failures from the gpt-5.4 run over the Loop workspace.""" + headline = convert( + viz("local:headline", [{"localIdentifier": "measures", "items": [measure("m", "spend")]}]), DATE_IDS + ) + names = {"metric/spend": "Variant Exchange Ratio"} + for bad in ( + "Can you show me Variant Exchange Ratio broken down by Variant Exchange Ratio?", + "Can you show me the Variant Exchange Ratio by Variant Exchange Ratio?", + "Can you show me Variant Exchange Ratio by breakdown dimension?", + ): + assert contradictions(bad, headline, names), bad + assert contradictions("Can you show me Variant Exchange Ratio?", headline, names) == [] + + +# --- AD's "All" filters and the singular ranking form ----------------------- + + +@pytest.mark.parametrize( + "noop", + [ + {"negativeAttributeFilter": {"displayForm": {"identifier": {"id": "product_name"}}, "notIn": {"values": []}}}, + {"positiveAttributeFilter": {"displayForm": {"identifier": {"id": "product_name"}}, "in": {"values": []}}}, + {"relativeDateFilter": {"dataSet": {"identifier": {"id": "process_date"}}, "granularity": "GDC.time.month"}}, + ], +) +def test_all_selection_filters_are_dropped_not_fatal(noop): + """AD writes an unset filter as an empty exclusion or an all-time window. + + It restricts nothing, so it must not appear in the spec -- and must not cost the + insight, which is otherwise perfectly expressible. + """ + spec = convert(spend_by_merchant(filters=[noop]), DATE_IDS) + assert spec["query"]["filter_by"] == {} + assert spec["_shape"] == "breakdown_by_dimension" + + +def test_dropped_noop_filter_does_not_leave_a_gap_in_filter_keys(): + spec = convert( + spend_by_merchant( + filters=[ + {"negativeAttributeFilter": {"displayForm": {"identifier": {"id": "x"}}, "notIn": {"values": []}}}, + { + "positiveAttributeFilter": { + "displayForm": {"identifier": {"id": "region"}}, + "in": {"values": ["EMEA"]}, + } + }, + ] + ), + DATE_IDS, + ) + assert list(spec["query"]["filter_by"]) == ["f0"] + + +def test_singular_ranking_filter_form_is_understood(): + """AD emits `measure: {localIdentifier}`, not only `measures: [localId]`.""" + spec = convert( + spend_by_merchant( + filters=[{"rankingFilter": {"measure": {"localIdentifier": "m"}, "operator": "TOP", "value": 3}}] + ), + DATE_IDS, + ) + assert spec["query"]["filter_by"]["f0"] == {"type": "ranking_filter", "using": "m_spend", "top": 3} + + with_attribute = convert( + spend_by_merchant( + filters=[ + { + "rankingFilter": { + "measure": {"localIdentifier": "m"}, + "attribute": {"localIdentifier": "a"}, + "operator": "BOTTOM", + "value": 5, + } + } + ] + ), + DATE_IDS, + ) + assert with_attribute["query"]["filter_by"]["f0"]["attribute"] == "d_merchant_name" + assert with_attribute["query"]["filter_by"]["f0"]["bottom"] == 5 + + +def test_uri_form_attribute_filter_is_still_skipped(): + """The guard the empty-values case was wrongly sharing: uris can't become literals.""" + with pytest.raises(Unsupported, match="literal values"): + convert( + spend_by_merchant( + filters=[ + { + "negativeAttributeFilter": { + "displayForm": {"identifier": {"id": "x"}}, + "notIn": {"uris": ["/obj/1"]}, + } + } + ] + ), + DATE_IDS, + ) From acb774aabcadda82106efdd0b17c498d3416ddb6 Mon Sep 17 00:00:00 2001 From: Roman Valovic <xvalovic@mendelu.cz> Date: Wed, 2 Sep 2026 09:40:46 +0200 Subject: [PATCH 02/15] feat(gooddata-eval): cap how long one turn and one item may run `httpx`'s `timeout` is per-read, so an agent that keeps emitting reasoning events resets it on every chunk and can stream for many minutes without ever tripping it. One loop-test item took 815s that way, and the run had no way to abandon it. Two independent budgets, both uncapped by default: --turn-timeout SECONDS (GOODDATA_EVAL_CHAT_TURN_TIMEOUT_S) --item-timeout SECONDS (GOODDATA_EVAL_CHAT_ITEM_TIMEOUT_S) The turn budget bounds a single stream; the item budget is anchored at conversation creation and spans every turn taken on it, which is what a multi-turn agentic item needs -- a turn cap alone lets a 4-turn conversation run to 4x the budget. Each turn takes whichever deadline falls first, so turn 4 of an item that has already spent 280s of a 300s budget gets 20s, not a fresh 60. Enforced between SSE events and, so a turn that goes silent is also cut off, by lowering the smaller cap onto the client's read timeout. Exceeding either raises `TurnTimeoutError`, deliberately non-retryable: a slow turn stays slow and a retry would just spend the budget again. The runner records the item as errored and moves to the next question. The caps are module defaults set once per run rather than constructor arguments, because the eight agentic evaluators build their own ChatClient deep in the call tree -- a flag threaded only through the CLI's own client would have silently skipped exactly the multi-turn items that need it most. Also: `parse_sse_lines` re-wrapped any ChatError raised from the stream iterator into a generic one, which relabelled the timeout as a transport failure and would have flipped a TransientChatError to non-retryable. It now re-raises an already-classified error untouched, attaching the partial result if it has none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --- .../src/gooddata_eval/cli/main.py | 23 ++++- .../src/gooddata_eval/core/chat/sse_client.py | 94 ++++++++++++++++++- .../src/gooddata_eval/core/config.py | 4 + .../gooddata-eval/tests/test_sse_client.py | 56 +++++++++++ 4 files changed, 174 insertions(+), 3 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/cli/main.py b/packages/gooddata-eval/src/gooddata_eval/cli/main.py index f40cf9daf..b3d1c9728 100644 --- a/packages/gooddata-eval/src/gooddata_eval/cli/main.py +++ b/packages/gooddata-eval/src/gooddata_eval/cli/main.py @@ -15,7 +15,7 @@ from rich.table import Table from gooddata_eval.cli.agentic_runner import AGENTIC_TEST_KINDS, run_agentic_items -from gooddata_eval.core.chat.sse_client import ChatClient +from gooddata_eval.core.chat.sse_client import ChatClient, set_default_item_timeout, set_default_turn_timeout from gooddata_eval.core.config import ReasoningEffort, RunConfig from gooddata_eval.core.connection import ConnectionError_, resolve_connection from gooddata_eval.core.dataset.from_insights import generate as generate_from_insights @@ -99,6 +99,20 @@ def _build_parser() -> argparse.ArgumentParser: help="Number of items evaluated concurrently (default 1 = sequential). " "Increase to load-test the agent under simultaneous requests.", ) + run.add_argument( + "--turn-timeout", + dest="turn_timeout", + type=float, + help="Wall-clock seconds a single agent turn may take before the item is failed and the " + "run moves on (or set GOODDATA_EVAL_CHAT_TURN_TIMEOUT_S). Default: uncapped.", + ) + run.add_argument( + "--item-timeout", + dest="item_timeout", + type=float, + help="Wall-clock seconds one item may take across ALL its turns before it is failed and " + "the run moves on (or set GOODDATA_EVAL_CHAT_ITEM_TIMEOUT_S). Default: uncapped.", + ) run.add_argument("--json", dest="json_path", help="Write a JSON report to this path.") run.add_argument("--quiet", action="store_true", help="Suppress per-item progress output.") run.add_argument( @@ -323,6 +337,9 @@ def _list_models(host: str, token: str, workspace_id: str | None) -> int: def _run(config: RunConfig) -> int: + # Applies to the agentic evaluators' own clients too, which this function never sees. + set_default_turn_timeout(config.turn_timeout_s) + set_default_item_timeout(config.item_timeout_s) if config.log_to_langfuse and config.langfuse_dataset is None: print( "error: --langfuse requires --langfuse-dataset (local datasets have no Langfuse item ids to link to).", @@ -424,6 +441,8 @@ def on_langfuse_item_done( preserve_failed=config.preserve_failed, reasoning_effort=config.reasoning_effort, agent_id=config.agent_id, + turn_timeout_s=config.turn_timeout_s, + item_timeout_s=config.item_timeout_s, ), SummaryClient(host=config.host, token=config.token, workspace_id=config.workspace_id), ) @@ -536,6 +555,8 @@ def main(argv: list[str] | None = None) -> int: preserve_failed=args.preserve_failed, reasoning_effort=args.reasoning_effort, agent_id=args.agent_id or os.environ.get("GD_EVAL_AGENT_ID"), + turn_timeout_s=args.turn_timeout, + item_timeout_s=args.item_timeout, ) return _run(config) except ( diff --git a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py index 7d52dad3f..c440ea85f 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py @@ -62,6 +62,12 @@ class TransientChatError(ChatError): """Retryable transient error: gen-ai temporarily unavailable or still syncing metadata.""" +class TurnTimeoutError(ChatError): + """The agent exceeded a wall-clock budget -- either the per-turn one or the per-item + one spanning every turn of a conversation. Not retryable: a slow turn stays slow, and + retrying it spends the budget again.""" + + def _int_env(name: str, default: int) -> int: """Read an int from the environment, falling back to ``default`` when unset or blank.""" raw = os.getenv(name) @@ -81,6 +87,33 @@ def _float_env(name: str, default: float) -> float: _BACKOFF_FACTOR = _float_env("GOODDATA_EVAL_CHAT_BACKOFF_FACTOR", 2.0) _MAX_BACKOFF_S = _float_env("GOODDATA_EVAL_CHAT_MAX_BACKOFF_S", 60.0) +# Wall-clock cap on a single agent turn, 0 = uncapped. httpx's `timeout` is per-read, +# so an agent that keeps emitting reasoning events can stream for many minutes without +# ever tripping it -- this is what bounds a runaway item and lets the run move on. +_TURN_TIMEOUT_S = _float_env("GOODDATA_EVAL_CHAT_TURN_TIMEOUT_S", 0.0) + +# Wall-clock cap on one whole item, 0 = uncapped. Anchored at conversation creation, so +# for a multi-turn agentic item it bounds every turn together -- a turn cap alone lets a +# 4-turn conversation run to 4x the budget, which is not what a user would sit through. +_ITEM_TIMEOUT_S = _float_env("GOODDATA_EVAL_CHAT_ITEM_TIMEOUT_S", 0.0) + + +def set_default_turn_timeout(seconds: float | None) -> None: + """Set the per-turn budget every ChatClient built afterwards inherits. + + The agentic evaluators construct their own clients deep in the call tree, so a CLI + flag has to land here rather than being threaded through eight signatures. + """ + global _TURN_TIMEOUT_S + _TURN_TIMEOUT_S = seconds or 0.0 + + +def set_default_item_timeout(seconds: float | None) -> None: + """Set the per-item budget every ChatClient built afterwards inherits.""" + global _ITEM_TIMEOUT_S + _ITEM_TIMEOUT_S = seconds or 0.0 + + T = TypeVar("T") @@ -225,6 +258,23 @@ def _build_chat_result(acc: _SseAccumulator) -> ChatResult: return result +def _until_deadline( + lines: Iterable[str], deadline: float | None, budget: float = 0.0, scope: str = "turn" +) -> Iterable[str]: + """Yield `lines`, aborting once `deadline` (a monotonic timestamp) has passed. + + Checked between events rather than mid-read, so the effective cap is the budget plus + the time of the event in flight; the client's read timeout bounds that tail. + """ + if deadline is None: + yield from lines + return + for line in lines: + if time.monotonic() > deadline: + raise TurnTimeoutError(f"agent exceeded the {budget:.0f}s {scope} budget") + yield line + + def parse_sse_lines(lines: Iterable[str]) -> ChatResult: """Parse an SSE stream (iterable of decoded lines) into a ChatResult.""" acc = _SseAccumulator() @@ -240,6 +290,13 @@ def parse_sse_lines(lines: Iterable[str]) -> ChatResult: # here -- a bug in the processing below must propagate uncaught, not get # mislabeled as a network error. partial = _build_chat_result(acc) + if isinstance(exc, ChatError): + # Already classified by the iterator (e.g. the turn-timeout guard): + # re-wrapping would relabel it as a transport failure and, for + # TransientChatError, silently flip it to non-retryable. + if exc.partial_result is None: + exc.partial_result = partial + raise if isinstance(exc, httpx.RemoteProtocolError): # Same mid-stream disconnect _is_retryable_exc already retries when it happens # at connect time -- here it surfaces from `next(it)` instead, so it must be @@ -315,6 +372,8 @@ def __init__( workspace_id: str, *, timeout: float = 300.0, + turn_timeout_s: float | None = None, + item_timeout_s: float | None = None, preserve_failed: bool = False, reasoning_effort: ReasoningEffort | None = None, agent_id: str | None = None, @@ -329,6 +388,17 @@ def __init__( """ self._base = f"{host.rstrip('/')}/api/v1/ai/workspaces/{workspace_id}/chat/conversations" self._auth = {"Authorization": f"Bearer {token}"} + # 0/None disables the cap. Also lowered onto the read timeout: the wall-clock check + # fires between events, so a turn that goes silent needs the transport to give up too. + budget = _TURN_TIMEOUT_S if turn_timeout_s is None else turn_timeout_s + self._turn_timeout_s = budget or None + item_budget = _ITEM_TIMEOUT_S if item_timeout_s is None else item_timeout_s + self._item_timeout_s = item_budget or None + # Anchored when a conversation is created; spans every turn taken on it. + self._conversation_started: float | None = None + caps = [c for c in (self._turn_timeout_s, self._item_timeout_s) if c is not None] + if caps: + timeout = httpx.Timeout(timeout, read=min(timeout, *caps)) self._client = httpx.Client(timeout=timeout) self._preserve_failed = preserve_failed self._reasoning_effort = normalize_reasoning_effort(reasoning_effort) @@ -346,7 +416,11 @@ def _do() -> str: # NOTE: retrying create is not idempotent — a created-then-503 can leak an # orphaned (ephemeral) conversation. Acceptable for eval; do not reuse blindly. - return _retry_transient(_do, is_retryable=_is_retryable_exc) + conversation_id = _retry_transient(_do, is_retryable=_is_retryable_exc) + # Anchor the per-item clock here: for an agentic item this conversation carries + # every turn, so the budget must run from its creation, not from each send. + self._conversation_started = time.monotonic() + return conversation_id def delete_conversation(self, conversation_id: str) -> None: try: @@ -366,10 +440,11 @@ def _do() -> ChatResult: # own connection setup time counts) -- excludes not just the sleep backoff between # attempts, but the entire duration of any earlier failed attempt. t0 = time.monotonic() + deadline, budget, scope = self._deadline(t0) with self._client.stream("POST", url, json=body, headers=headers) as resp: resp.raise_for_status() try: - result = parse_sse_lines(resp.iter_lines()) + result = parse_sse_lines(_until_deadline(resp.iter_lines(), deadline, budget, scope)) except ChatError as exc: if exc.partial_result is not None: exc.partial_result.turn_wall_clock_sec = time.monotonic() - t0 @@ -379,6 +454,21 @@ def _do() -> ChatResult: return _retry_transient(_do, is_retryable=_is_retryable_exc) + def _deadline(self, t0: float) -> tuple[float | None, float, str]: + """The earlier of the turn and item caps, as (deadline, budget, scope). + + The item cap runs from conversation creation, so on a multi-turn conversation the + remaining budget shrinks with every turn already spent. + """ + candidates = [] + if self._turn_timeout_s is not None: + candidates.append((t0 + self._turn_timeout_s, self._turn_timeout_s, "turn")) + if self._item_timeout_s is not None and self._conversation_started is not None: + candidates.append((self._conversation_started + self._item_timeout_s, self._item_timeout_s, "item")) + if not candidates: + return None, 0.0, "turn" + return min(candidates) + def ask(self, item: DatasetItem) -> ChatResult: """Run one conversation: create, send, parse, clean up. diff --git a/packages/gooddata-eval/src/gooddata_eval/core/config.py b/packages/gooddata-eval/src/gooddata_eval/core/config.py index 06a2dd926..b93852116 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/config.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/config.py @@ -45,3 +45,7 @@ class RunConfig: preserve_failed: bool = False reasoning_effort: ReasoningEffort | None = None agent_id: str | None = None + turn_timeout_s: float | None = None + """Wall-clock cap per agent turn; None keeps GOODDATA_EVAL_CHAT_TURN_TIMEOUT_S (0 = off).""" + item_timeout_s: float | None = None + """Wall-clock cap per item across all its turns; None keeps GOODDATA_EVAL_CHAT_ITEM_TIMEOUT_S.""" diff --git a/packages/gooddata-eval/tests/test_sse_client.py b/packages/gooddata-eval/tests/test_sse_client.py index 5cc205dc5..f78ddb0e7 100644 --- a/packages/gooddata-eval/tests/test_sse_client.py +++ b/packages/gooddata-eval/tests/test_sse_client.py @@ -867,3 +867,59 @@ def test_invalid_reasoning_effort_fails_at_construction(): """Fail locally rather than as an out-of-enum request partway through a run.""" with pytest.raises(ValueError, match="Invalid reasoning effort"): ChatClient(host="https://example.invalid", token="t", workspace_id="w", reasoning_effort="maximum") + + +def test_turn_timeout_aborts_a_streaming_turn_and_is_not_retried(monkeypatch): + """A chatty-but-slow agent must be cut off: httpx's per-read timeout never fires + for one, so only the wall-clock budget bounds the item.""" + clock = iter([0.0, 0.0, 1.0, 61.0, 61.0, 61.0]) + monkeypatch.setattr(sse_mod.time, "monotonic", lambda: next(clock)) + + def forever(): + while True: + yield 'data: {"role":"assistant","content":{"type":"reasoning","text":"thinking"}}' + + with pytest.raises(sse_mod.TurnTimeoutError, match="exceeded the 60s turn budget"): + sse_mod.parse_sse_lines(sse_mod._until_deadline(forever(), deadline=60.0, budget=60.0)) + + assert sse_mod._is_retryable_exc(sse_mod.TurnTimeoutError("x")) is False + + +def test_no_turn_timeout_leaves_the_stream_untouched(): + lines = ['data: {"role":"assistant","content":{"type":"text","text":"hi"}}'] + assert list(sse_mod._until_deadline(iter(lines), deadline=None)) == lines + + +def test_default_turn_timeout_reaches_clients_built_later(monkeypatch): + """The agentic evaluators build their own ChatClient, so the CLI flag has to be a + module default rather than a constructor argument threaded through them.""" + monkeypatch.setattr(sse_mod, "_TURN_TIMEOUT_S", 0.0) + sse_mod.set_default_turn_timeout(60) + client = sse_mod.ChatClient(host="http://h", token="t", workspace_id="w") + assert client._turn_timeout_s == 60 + + sse_mod.set_default_turn_timeout(None) + assert sse_mod.ChatClient(host="http://h", token="t", workspace_id="w")._turn_timeout_s is None + + +def test_item_budget_shrinks_across_turns_of_one_conversation(monkeypatch): + """A per-turn cap alone lets a 4-turn agentic item run to 4x the budget. The item cap + is anchored at conversation creation, so later turns inherit what is left of it.""" + monkeypatch.setattr(sse_mod, "_TURN_TIMEOUT_S", 0.0) + monkeypatch.setattr(sse_mod, "_ITEM_TIMEOUT_S", 0.0) + client = sse_mod.ChatClient(host="http://h", token="t", workspace_id="w", turn_timeout_s=60, item_timeout_s=90) + client._conversation_started = 0.0 + + # First turn: the turn cap (60s) bites before the item cap (90s). + assert client._deadline(0.0) == (60.0, 60, "turn") + # Third turn, 80s already spent: the item cap is what is left, and it is what fires. + assert client._deadline(80.0) == (90.0, 90, "item") + + +def test_item_timeout_alone_still_caps_a_turn(monkeypatch): + monkeypatch.setattr(sse_mod, "_TURN_TIMEOUT_S", 0.0) + monkeypatch.setattr(sse_mod, "_ITEM_TIMEOUT_S", 0.0) + client = sse_mod.ChatClient(host="http://h", token="t", workspace_id="w", item_timeout_s=300) + assert client._deadline(0.0) == (None, 0.0, "turn") # no conversation yet + client._conversation_started = 10.0 + assert client._deadline(20.0) == (310.0, 300, "item") From 3125e19ac0fccae3eee6f2b04bed4e6b01a2ed07 Mon Sep 17 00:00:00 2001 From: Roman Valovic <xvalovic@mendelu.cz> Date: Mon, 7 Sep 2026 15:12:49 +0200 Subject: [PATCH 03/15] feat(gooddata-eval): make a run's output something you can actually read The JSON report already holds everything an investigation needs; what was missing was a way to look at it, so the same reading got redone by hand into per-kind PDFs, ASCII tables and ad-hoc spreadsheets. `gd-eval report a.json [b.json ...] -o out.html` renders one self-contained HTML file -- no server, no credentials, no external assets, so it opens over file://, attaches to a Jira issue and survives a Slack thread. `run --html` does the same at the end of a run. It stays a view over json_report.py and computes nothing of its own: run cards and the comparison table, an item table with one pass/fail column per run, a per-item drawer (checks, expected vs actual, reasoning, ids) and the latency_breakdown as a timeline whose reasoning steps expand to the paragraph they were summarised from, joined by `index`. Two decisions worth naming: Passing several files IS the run-over-run mechanism -- each becomes a column, keyed by file name, so nothing needs a database or a run registry. Cross-cutting questions go through a JavaScript expression box (`d.filter_ranking_score === false`) rather than fixed facets, because `detail` has a different shape per test_kind and the useful questions cannot be enumerated in advance. `--redact` is a flag on one report, not a second output that would drift: it drops conversation/response ids and raw reasoning and renames models to Model A/B, while pass rate, questions and latency survive. Internal is the default, so the expensive mistake needs an explicit flag. AIS-48 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --- packages/gooddata-eval/README.md | 48 +++ .../src/gooddata_eval/cli/main.py | 48 ++- .../src/gooddata_eval/core/config.py | 2 + .../core/reporting/html_report.py | 108 +++++ .../core/reporting/report_template.html | 373 ++++++++++++++++++ .../gooddata-eval/tests/test_html_report.py | 116 ++++++ 6 files changed, 694 insertions(+), 1 deletion(-) create mode 100644 packages/gooddata-eval/src/gooddata_eval/core/reporting/html_report.py create mode 100644 packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html create mode 100644 packages/gooddata-eval/tests/test_html_report.py diff --git a/packages/gooddata-eval/README.md b/packages/gooddata-eval/README.md index e5f321027..562b8f2ba 100644 --- a/packages/gooddata-eval/README.md +++ b/packages/gooddata-eval/README.md @@ -16,6 +16,7 @@ Or install `gd-eval` as a standalone tool: | Command | Description | |---|---| | `gd-eval run` | Run an evaluation dataset against one or more models. | +| `gd-eval report` | Render JSON report(s) as one self-contained HTML file. | | `gd-eval models` | List LLM providers and models configured in the org. | --- @@ -134,6 +135,8 @@ gd-eval run \ | Flag | Description | |---|---| | `--json PATH` | Write a JSON report to this path. Always uses the nested `{models, runs, comparison}` shape even for a single model. | +| `--html PATH` | Write a self-contained HTML report to this path (same output as `gd-eval report`). | +| `--redact` | Make the HTML customer-safe. See `gd-eval report`. | | `--quiet` | Suppress per-item progress. Per-model result tables and the comparison summary are still printed. | #### Langfuse sink @@ -164,6 +167,51 @@ Winner is selected by **pass rate → quality score → latency** (lower latency --- +## `gd-eval report` + +Turns JSON report(s) into one HTML file you can actually navigate. No server, no +credentials, no external assets — it opens over `file://`, attaches to a Jira issue and +survives a Slack thread. + +```bash +# one run +gd-eval report results.json -o report.html + +# several runs side by side -- each file becomes its own column +gd-eval report aug-21.json sep-07.json -o comparison.html --title "H200 regression check" + +# customer-safe +gd-eval report results.json -o customer.html --redact +``` + +| Flag | Description | +|---|---| +| `-o, --out PATH` | Where to write the HTML. Required. | +| `--title TEXT` | Title shown in the report header. | +| `--redact` | Drop conversation/response ids and raw reasoning, and rename models to `Model A`, `Model B`, … Pass rate, per-item results, questions and latency survive. | + +The report is a *view* over the JSON — it computes no numbers of its own. It gives you: + +- **Run cards and a comparison table** — pass rate, quality, latency per run. +- **An item table** with a pass/fail column per run, so a model or run-over-run + regression is one glance rather than a hand-assembled spreadsheet. +- **An expression filter** for cross-cutting questions the fixed filters can't + anticipate, e.g. `d.filter_ranking_score === false` or + `d.expected_metric_uris.length > 1 && !d.metrics_correct`. Available variables: + `d` (the focused run's `detail`), `it` (its item), `i` (the row, `i.per[label]` for any + run), `q` (question), `kind`. +- **A per-item drawer** — checks as pass/fail chips, expected vs actual side by side, + full reasoning, conversation/response ids. +- **A latency timeline** from `detail.latency_breakdown`, in execution order, one bar per + step. Clicking a reasoning step expands the full paragraph it was summarised from + (joined by `index`). Tool arguments and results are not in the JSON report, so tool + steps show name and duration only. + +Passing several files keyed by file name is the whole run-over-run mechanism: no +database, no run registry, just the JSON files you already have on disk. + +--- + ## `gd-eval models` List all LLM providers and their models in the org. Marks the active model diff --git a/packages/gooddata-eval/src/gooddata_eval/cli/main.py b/packages/gooddata-eval/src/gooddata_eval/cli/main.py index 1e40efd77..c536edbd7 100644 --- a/packages/gooddata-eval/src/gooddata_eval/cli/main.py +++ b/packages/gooddata-eval/src/gooddata_eval/cli/main.py @@ -22,7 +22,8 @@ from gooddata_eval.core.langfuse.sink import LangfuseSink from gooddata_eval.core.models import ChatResult, DatasetItem from gooddata_eval.core.reporting.console import render_comparison, render_console -from gooddata_eval.core.reporting.json_report import write_multi_model_report +from gooddata_eval.core.reporting.html_report import load_report_files, write_html_report +from gooddata_eval.core.reporting.json_report import build_multi_model_report, write_multi_model_report from gooddata_eval.core.runner import ItemReport, run_items from gooddata_eval.core.summary.http_client import SummaryClient from gooddata_eval.core.workspace import ModelResolutionError, WorkspaceModelController @@ -99,6 +100,18 @@ def _build_parser() -> argparse.ArgumentParser: "Increase to load-test the agent under simultaneous requests.", ) run.add_argument("--json", dest="json_path", help="Write a JSON report to this path.") + run.add_argument( + "--html", + dest="html_path", + help="Write a self-contained HTML report to this path. Same output as `gd-eval report`, " + "for the single-run case where you do not want to keep the JSON around.", + ) + run.add_argument( + "--redact", + action="store_true", + help="Customer-safe HTML: drop conversation/response ids and raw reasoning, and replace " + "model names with 'Model A', 'Model B', ...", + ) run.add_argument("--quiet", action="store_true", help="Suppress per-item progress output.") run.add_argument( "--preserve-failed", @@ -128,6 +141,22 @@ def _build_parser() -> argparse.ArgumentParser: "resolves, which may not have every skill under test enabled." ), ) + report = sub.add_parser( + "report", + help="Render JSON report(s) as one self-contained HTML file.", + description="Render JSON report(s) as one self-contained HTML file. Pass several files to " + "compare runs side by side -- each becomes its own column, keyed by file name.", + ) + report.add_argument("json_paths", nargs="+", metavar="REPORT.json", help="JSON report file(s) from `run --json`.") + report.add_argument("-o", "--out", required=True, help="Path to write the HTML file to.") + report.add_argument("--title", default="gd-eval report", help="Title shown in the report header.") + report.add_argument( + "--redact", + action="store_true", + help="Customer-safe output: drop conversation/response ids and raw reasoning, and replace " + "model names with 'Model A', 'Model B', ...", + ) + models_cmd = sub.add_parser("models", help="List LLM providers and models configured in the org.") models_cmd.add_argument("--host", help="GoodData host URL.") models_cmd.add_argument("--token", help="API token (or set GOODDATA_TOKEN).") @@ -435,6 +464,16 @@ def on_langfuse_item_done( if config.json_path is not None: write_multi_model_report(reports, config.json_path) + if config.html_path is not None: + write_html_report(build_multi_model_report(reports), config.html_path, redact=config.redact) + + return _EXIT_OK + + +def _report(args: argparse.Namespace) -> int: + paths = [Path(p) for p in args.json_paths] + write_html_report(load_report_files(paths), Path(args.out), redact=args.redact, title=args.title) + print(f"Wrote {args.out}") return _EXIT_OK @@ -444,6 +483,11 @@ def main(argv: list[str] | None = None) -> int: print("error: --concurrency must be >= 1.", file=sys.stderr) return _EXIT_OPERATIONAL_ERROR try: + # Rendering existing JSON needs no host, token or workspace -- dispatch before + # resolve_connection so `report` works on a laptop with no credentials at all. + if args.command == "report": + return _report(args) + host, token = resolve_connection(host=args.host, token=args.token, profile=args.profile) if args.command == "models": return _list_models(host, token, getattr(args, "workspace", None)) @@ -457,6 +501,8 @@ def main(argv: list[str] | None = None) -> int: runs=args.runs, concurrency=args.concurrency, json_path=Path(args.json_path) if args.json_path else None, + html_path=Path(args.html_path) if args.html_path else None, + redact=args.redact, log_to_langfuse=args.langfuse, quiet=args.quiet, kind=args.kind, diff --git a/packages/gooddata-eval/src/gooddata_eval/core/config.py b/packages/gooddata-eval/src/gooddata_eval/core/config.py index 06a2dd926..40d472c7f 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/config.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/config.py @@ -39,6 +39,8 @@ class RunConfig: runs: int = 2 concurrency: int = 1 json_path: Path | None = None + html_path: Path | None = None + redact: bool = False log_to_langfuse: bool = False quiet: bool = False kind: str = "visualization" diff --git a/packages/gooddata-eval/src/gooddata_eval/core/reporting/html_report.py b/packages/gooddata-eval/src/gooddata_eval/core/reporting/html_report.py new file mode 100644 index 000000000..881d0baf4 --- /dev/null +++ b/packages/gooddata-eval/src/gooddata_eval/core/reporting/html_report.py @@ -0,0 +1,108 @@ +# (C) 2026 GoodData Corporation +"""Render one or more JSON reports as a single self-contained HTML file. + +This is a *view* over ``json_report.py``'s output, never a second source of truth: it +adds no numbers of its own, it only makes the existing ones navigable. The result is one +file with no external references -- it opens over ``file://``, attaches to a Jira issue +and survives a Slack thread, which is most of the point. + +Passing several JSON files merges them into one report, each becoming its own column. +That is how run-over-run comparison works: no database, no run registry, just the files +you already have on disk. +""" + +from __future__ import annotations + +from datetime import datetime, timezone +from pathlib import Path + +import orjson + +_TEMPLATE = Path(__file__).with_name("report_template.html") +_PLACEHOLDER = "__GD_EVAL_DATA__" + +# Dropped outright (not blanked) from a redacted report, so it cannot be un-redacted by +# reading the embedded data blob: internal ids, and the model's own raw reasoning text. +_REDACTED_ITEM_FIELDS = frozenset({"conversation_id", "response_id", "reasoning"}) + + +def _redact(doc: dict) -> dict: + """Strip internal ids and replace model names with stable aliases. + + Per-item latency and pass/fail survive -- those are facts about the run a customer is + entitled to. What goes is anything that identifies our infrastructure or discloses + which model was under test. + """ + alias = {label: f"Model {chr(65 + n)}" for n, label in enumerate(doc.get("runs", {}))} + runs = { + alias[label]: { + **run, + "model": alias[label], + "workspace_id": "", + "items": { + item_id: {k: v for k, v in item.items() if k not in _REDACTED_ITEM_FIELDS} + for item_id, item in (run.get("items") or {}).items() + }, + } + for label, run in doc.get("runs", {}).items() + } + comparison = { + alias[label]: {**entry, "provider_name": ""} + for label, entry in (doc.get("comparison") or {}).items() + if label in alias + } + return {"runs": runs, "comparison": comparison} + + +def merge_docs(docs: list[tuple[str, dict]]) -> dict: + """Merge ``(source_label, json_report_doc)`` pairs into one multi-run document. + + With more than one source the source label is prefixed onto every run key, so the + same model evaluated on two different days stays two distinct columns instead of one + silently overwriting the other. + """ + prefix = len(docs) > 1 + runs: dict[str, dict] = {} + comparison: dict[str, dict] = {} + for source, doc in docs: + for label, run in (doc.get("runs") or {}).items(): + key = f"{source} · {label}" if prefix else label + unique, n = key, 2 + while unique in runs: + unique, n = f"{key} ({n})", n + 1 + runs[unique] = run + entry = (doc.get("comparison") or {}).get(label) + if entry is not None: + comparison[unique] = entry + return {"runs": runs, "comparison": comparison} + + +def load_report_files(paths: list[Path]) -> dict: + """Read JSON report files and merge them into one document.""" + docs: list[tuple[str, dict]] = [] + for p in paths: + path = Path(p) + doc = orjson.loads(path.read_bytes()) + if "runs" not in doc: + # A bare single-run dict from the older build_json_report shape. + doc = {"runs": {doc.get("model") or path.stem: doc}, "comparison": {}} + docs.append((path.stem, doc)) + return merge_docs(docs) + + +def build_html(doc: dict, redact: bool = False, title: str = "gd-eval report") -> str: + """Render a merged report document into a standalone HTML page.""" + payload = { + "title": title, + "generated_at": datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M UTC"), + "redacted": redact, + **(_redact(doc) if redact else {"runs": doc.get("runs") or {}, "comparison": doc.get("comparison") or {}}), + } + # "</" would close the host <script> tag early; "<\/" is an equivalent JSON escape and + # "<" cannot occur outside a JSON string, so this is safe to apply to the whole blob. + blob = orjson.dumps(payload).decode().replace("</", "<\\/") + return _TEMPLATE.read_text(encoding="utf-8").replace(_PLACEHOLDER, blob) + + +def write_html_report(doc: dict, path: Path, redact: bool = False, title: str = "gd-eval report") -> None: + Path(path).write_text(build_html(doc, redact=redact, title=title), encoding="utf-8") diff --git a/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html b/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html new file mode 100644 index 000000000..c91741be1 --- /dev/null +++ b/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html @@ -0,0 +1,373 @@ +<!doctype html> +<html lang="en"> +<head> +<meta charset="utf-8"> +<meta name="viewport" content="width=device-width, initial-scale=1"> +<title>gd-eval report + + + + + +
+

+
+
+ +
+
+
+
+
+

Items

+
+ + + + + + +
+
+ expression vars: d detail of focused run · it focused run's item · i row (i.per[label]) · q question · kind +
+
+ +
+
+
+
+ + + + diff --git a/packages/gooddata-eval/tests/test_html_report.py b/packages/gooddata-eval/tests/test_html_report.py new file mode 100644 index 000000000..4fab8a5fa --- /dev/null +++ b/packages/gooddata-eval/tests/test_html_report.py @@ -0,0 +1,116 @@ +# (C) 2026 GoodData Corporation +import json +import re + +import orjson +import pytest +from gooddata_eval.cli.main import main +from gooddata_eval.core.reporting.html_report import build_html, load_report_files + + +def _doc(model: str, passed: bool) -> dict: + return { + "models": [model], + "runs": { + model: { + "model": model, + "workspace_id": "ws1", + "summary": {"total": 1, "passed": int(passed), "failed": int(not passed), "avg_latency_s": 2.5}, + "items": { + "item-1": { + "test_kind": "visualization", + "question": "How many orders?", + "pass_at_k": passed, + "conversation_id": "conv-secret", + "response_id": "resp-secret", + "reasoning": ["**Thinking**\n\ninternal thoughts"], + "detail": { + "metrics_correct": passed, + "latency_breakdown": [ + {"seq": 0, "kind": "tool", "name": "search", "index": 0, "duration_s": 1.0} + ], + }, + } + }, + } + }, + "comparison": {model: {"passed": int(passed), "total": 1, "pass_rate": float(passed)}}, + } + + +def _embedded(html: str) -> dict: + blob = re.search(r'', html, re.S).group(1) + return json.loads(blob) + + +def test_merges_files_into_one_run_per_source(tmp_path): + for name, passed in (("run-a", True), ("run-b", False)): + (tmp_path / f"{name}.json").write_bytes(orjson.dumps(_doc("gpt-5", passed))) + + doc = load_report_files([tmp_path / "run-a.json", tmp_path / "run-b.json"]) + + # Same model in both files must stay two columns, not overwrite each other. + assert sorted(doc["runs"]) == ["run-a · gpt-5", "run-b · gpt-5"] + assert sorted(doc["comparison"]) == ["run-a · gpt-5", "run-b · gpt-5"] + + +def test_legacy_single_run_file_is_accepted(tmp_path): + legacy = _doc("gpt-5", True)["runs"]["gpt-5"] + (tmp_path / "old.json").write_bytes(orjson.dumps(legacy)) + + assert list(load_report_files([tmp_path / "old.json"])["runs"]) == ["gpt-5"] + + +def test_html_embeds_parseable_data_and_no_external_refs(): + html = build_html(_doc("gpt-5", False)) + + data = _embedded(html) + assert data["runs"]["gpt-5"]["items"]["item-1"]["conversation_id"] == "conv-secret" + assert not re.search(r'(src|href)="(?!#)', html), "report must be self-contained" + + +def test_redact_drops_ids_reasoning_and_model_name(): + html = build_html(_doc("gpt-5", False), redact=True) + + assert "conv-secret" not in html + assert "resp-secret" not in html + assert "internal thoughts" not in html + assert "gpt-5" not in html + data = _embedded(html) + assert list(data["runs"]) == ["Model A"] + # The evaluation itself survives redaction -- only identity goes. + assert data["runs"]["Model A"]["items"]["item-1"]["question"] == "How many orders?" + assert data["runs"]["Model A"]["items"]["item-1"]["detail"]["latency_breakdown"] + + +def test_closing_script_tag_in_data_cannot_break_out(): + doc = _doc("gpt-5", False) + doc["runs"]["gpt-5"]["items"]["item-1"]["question"] = "" + + html = build_html(doc) + + # The blob must survive to the end of the payload -- if a "" inside the data + # had terminated the host tag early, this capture would be truncated and not parse. + assert _embedded(html)["runs"]["gpt-5"]["items"]["item-1"]["question"] == "" + + +def test_cli_report_command_writes_html(tmp_path): + src = tmp_path / "results.json" + src.write_bytes(orjson.dumps(_doc("gpt-5", True))) + out = tmp_path / "report.html" + + assert main(["report", str(src), "-o", str(out), "--title", "MSXi eval"]) == 0 + assert "MSXi eval" in out.read_text() + + +@pytest.mark.parametrize("redact", [False, True]) +def test_cli_report_command_needs_no_credentials(tmp_path, monkeypatch, redact): + monkeypatch.delenv("GOODDATA_TOKEN", raising=False) + monkeypatch.delenv("GOODDATA_HOST", raising=False) + src = tmp_path / "results.json" + src.write_bytes(orjson.dumps(_doc("gpt-5", True))) + out = tmp_path / "report.html" + + argv = ["report", str(src), "-o", str(out)] + (["--redact"] if redact else []) + assert main(argv) == 0 + assert out.exists() From edac065271ebc4723339b0f93d5634af594c4f89 Mon Sep 17 00:00:00 2001 From: Roman Valovic Date: Mon, 7 Sep 2026 15:23:16 +0200 Subject: [PATCH 04/15] feat(gooddata-eval): show the conversation, not just the turn count The agentic path already records every turn as `detail.transcript` -- who spoke, what they said, and the visualization an assistant turn produced. The report was dumping it as a raw JSON blob in the leftover-keys bucket, which is exactly the "reading it is manual work" the HTML report exists to end. It now renders as a conversation, one block per turn, with the simulated user colour-coded apart from a real question. That distinction is the point: it is how you tell "the agent got there" from "the simulated user, primed with the expected output, handed it the answer", which a turn count alone can never show. `--redact` drops the transcript for that same reason -- the exchange discloses how we score, not just what scored. `detail.turns` survives it: "this needed a clarification round" is a fair thing to show a customer. AIS-48 Co-Authored-By: Claude Opus 5 (1M context) --- .../core/reporting/html_report.py | 18 +++++++++--- .../core/reporting/report_template.html | 28 ++++++++++++++++++- .../gooddata-eval/tests/test_html_report.py | 17 +++++++++++ 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/reporting/html_report.py b/packages/gooddata-eval/src/gooddata_eval/core/reporting/html_report.py index 881d0baf4..b2579eedf 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/reporting/html_report.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/reporting/html_report.py @@ -25,6 +25,19 @@ # reading the embedded data blob: internal ids, and the model's own raw reasoning text. _REDACTED_ITEM_FIELDS = frozenset({"conversation_id", "response_id", "reasoning"}) +# Same, one level down inside `detail`. The transcript goes because the simulated user is +# primed with the expected output -- showing the exchange discloses how we score, not just +# what scored. `turns` (a count) stays: "this needed a clarification round" is a fair fact. +_REDACTED_DETAIL_FIELDS = frozenset({"transcript"}) + + +def _redact_item(item: dict) -> dict: + out = {k: v for k, v in item.items() if k not in _REDACTED_ITEM_FIELDS} + detail = out.get("detail") + if isinstance(detail, dict): + out["detail"] = {k: v for k, v in detail.items() if k not in _REDACTED_DETAIL_FIELDS} + return out + def _redact(doc: dict) -> dict: """Strip internal ids and replace model names with stable aliases. @@ -39,10 +52,7 @@ def _redact(doc: dict) -> dict: **run, "model": alias[label], "workspace_id": "", - "items": { - item_id: {k: v for k, v in item.items() if k not in _REDACTED_ITEM_FIELDS} - for item_id, item in (run.get("items") or {}).items() - }, + "items": {item_id: _redact_item(item) for item_id, item in (run.get("items") or {}).items()}, } for label, run in doc.get("runs", {}).items() } diff --git a/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html b/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html index c91741be1..1c7670bd7 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html +++ b/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html @@ -79,6 +79,12 @@ .tl .track > i { display: block; height: 100%; border-radius: 3px; background: var(--accent); } .tl .row.reasoning .track > i { background: #9aa5bb; } .tl .d { text-align: right; font-variant-numeric: tabular-nums; color: var(--dim); } + .turn { margin: 0 0 8px; padding: 6px 10px; border-left: 3px solid var(--line); background: #f7f7f5; border-radius: 0 4px 4px 0; } + .turn.assistant { border-left-color: var(--accent); background: #f2f5fd; } + .turn.simulated_user { border-left-color: #b07d2b; background: #fdf7ec; } + .turn.note { border-left-color: var(--skip); background: #fdf3d5; font-style: italic; } + .turn .who { font-size: 10.5px; text-transform: uppercase; letter-spacing: .05em; color: var(--dim); font-weight: 600; } + .turn .said { font-size: 12.5px; white-space: pre-wrap; overflow-wrap: anywhere; } .chip { display: inline-block; padding: 0 6px; border-radius: 3px; font-size: 11px; background: #eceae5; margin-right: 4px; } .chip.y { background: #e2f3e9; color: var(--pass); } .chip.n { background: #fbe9e7; color: var(--fail); } .close { float: right; cursor: pointer; color: var(--dim); background: none; border: 0; font-size: 18px; line-height: 1; } @@ -296,6 +302,7 @@

${esc(focus)}

${it.classification ? `${esc(it.classification)}` : ''} ${it.error ? `

Error

${esc(it.error)}
` : ''} + ${transcriptHtml(it)} ${detailHtml(it.detail || {})} ${timelineHtml(it)} ${reasoningHtml(it)} @@ -309,8 +316,27 @@

${esc(focus)}

}); } +// The agentic path drives a real multi-turn exchange: an answer without a visualization +// gets a simulated-user follow-up. Reading who said what is how you tell "the agent got +// there" apart from "the simulated user handed it the answer" -- so the turns are laid +// out as a conversation, with the simulated user visibly marked as not a real person. +function transcriptHtml(it) { + const turns = (it.detail || {}).transcript || []; + if (!turns.length) return ''; + const n = (it.detail || {}).turns; + return `

Conversation${n ? ` (${n} turn${n > 1 ? 's' : ''})` : ''}

` + + turns.map((t) => `
+
${esc(ROLE[t.role] || t.role)}${t.turn ? ` · turn ${esc(t.turn)}` : ''}
+
${esc(t.text || '')}
+ ${t.visualization ? `
${esc(fmt(t.visualization))}
` : ''} +
`).join('') + '
'; +} +const ROLE = { user: 'question', assistant: 'agent', simulated_user: 'simulated user', note: 'note' }; + function detailHtml(detail) { - const keys = Object.keys(detail).filter((k) => k !== 'latency_breakdown'); + // transcript/turns get their own section above; latency_breakdown drives the timeline. + const skip = new Set(['latency_breakdown', 'transcript', 'turns']); + const keys = Object.keys(detail).filter((k) => !skip.has(k)); if (!keys.length) return ''; const done = new Set(), parts = []; for (const k of keys) { diff --git a/packages/gooddata-eval/tests/test_html_report.py b/packages/gooddata-eval/tests/test_html_report.py index 4fab8a5fa..8aa495829 100644 --- a/packages/gooddata-eval/tests/test_html_report.py +++ b/packages/gooddata-eval/tests/test_html_report.py @@ -29,6 +29,12 @@ def _doc(model: str, passed: bool) -> dict: "latency_breakdown": [ {"seq": 0, "kind": "tool", "name": "search", "index": 0, "duration_s": 1.0} ], + "turns": 2, + "transcript": [ + {"turn": 1, "role": "user", "text": "How many orders?"}, + {"turn": 1, "role": "assistant", "text": "Which order status?"}, + {"turn": 2, "role": "simulated_user", "text": "primed with the expected answer"}, + ], }, } }, @@ -83,6 +89,17 @@ def test_redact_drops_ids_reasoning_and_model_name(): assert data["runs"]["Model A"]["items"]["item-1"]["detail"]["latency_breakdown"] +def test_redact_drops_the_transcript_but_keeps_the_turn_count(): + html = build_html(_doc("gpt-5", False), redact=True) + + # The simulated user is primed with the expected output, so the exchange discloses how + # we score. "It took 2 turns" is still a fair thing to show a customer. + assert "primed with the expected answer" not in html + detail = _embedded(html)["runs"]["Model A"]["items"]["item-1"]["detail"] + assert "transcript" not in detail + assert detail["turns"] == 2 + + def test_closing_script_tag_in_data_cannot_break_out(): doc = _doc("gpt-5", False) doc["runs"]["gpt-5"]["items"]["item-1"]["question"] = "" From e8b5af968e4742daf614b40d5a300e33ed198339 Mon Sep 17 00:00:00 2001 From: Roman Valovic Date: Mon, 7 Sep 2026 15:29:50 +0200 Subject: [PATCH 05/15] feat(gooddata-eval): keep what a tool was asked for, not just its name `RunResult.tool_call_events` carries every call's arguments and result, and it is in scope at the exact line each evaluator builds `detail` -- but only `build_latency_breakdown` read it, and that deliberately keeps just the name. So "create_visualization took 18s" was as far as an investigation could get: the args that produced it died in the evaluator. `build_tool_calls` records them as `detail.tool_calls`, the exact counterpart to the `reasoning` list: entries keyed by `index`, which is what the timeline's tool steps already point at. The breakdown stays light and the join answers what the call asked for -- the design its own docstring describes. The fourteen call sites all built `latency_breakdown` from the same two event lists, so they now go through one `timeline_detail()` helper instead. Both keys are derived from the same events in one place, which is what keeps their indexes aligned; adding a key at thirteen sites and forgetting the fourteenth was the failure waiting to happen. Arguments and results are clipped at 2000 chars with the original length noted -- a tool result can be a page of query rows, and unclipped it would dominate both the JSON and the HTML built from it. Calls with no `index` are skipped rather than guessed at: nothing can join to them, and a positional guess would attribute the wrong args to a step. `--redact` drops `tool_calls` -- results carry semantic-layer internals and real query rows. `latency_breakdown` stays, so a redacted timeline still shows which tool ran and for how long. AIS-48 Co-Authored-By: Claude Opus 5 (1M context) --- packages/gooddata-eval/README.md | 14 ++++- .../gooddata_eval/core/agentic/alert_skill.py | 6 +- .../core/agentic/conversation.py | 4 +- .../gooddata_eval/core/agentic/guardrail.py | 6 +- .../core/agentic/metric_skill.py | 6 +- .../core/agentic/visualization.py | 6 +- .../core/evaluators/general_question.py | 6 +- .../core/evaluators/guardrail.py | 10 +-- .../core/evaluators/search_tool.py | 6 +- .../core/evaluators/visualization.py | 6 +- .../src/gooddata_eval/core/models.py | 62 +++++++++++++++++++ .../core/reporting/html_report.py | 5 +- .../core/reporting/report_template.html | 23 +++++-- .../tests/test_agentic_alert_skill.py | 2 + .../tests/test_agentic_conversation.py | 2 + .../tests/test_agentic_guardrail.py | 2 + .../tests/test_agentic_metric_skill.py | 2 + .../tests/test_agentic_visualization.py | 2 + packages/gooddata-eval/tests/test_models.py | 50 +++++++++++++++ 19 files changed, 177 insertions(+), 43 deletions(-) diff --git a/packages/gooddata-eval/README.md b/packages/gooddata-eval/README.md index 562b8f2ba..b1130e05b 100644 --- a/packages/gooddata-eval/README.md +++ b/packages/gooddata-eval/README.md @@ -200,12 +200,20 @@ The report is a *view* over the JSON — it computes no numbers of its own. It g `d.expected_metric_uris.length > 1 && !d.metrics_correct`. Available variables: `d` (the focused run's `detail`), `it` (its item), `i` (the row, `i.per[label]` for any run), `q` (question), `kind`. +- **The conversation**, when the item ran the agentic multi-turn path — every turn in + order, with the simulated user marked apart from a real question, so you can see + whether the agent got there or was handed the answer. - **A per-item drawer** — checks as pass/fail chips, expected vs actual side by side, full reasoning, conversation/response ids. - **A latency timeline** from `detail.latency_breakdown`, in execution order, one bar per - step. Clicking a reasoning step expands the full paragraph it was summarised from - (joined by `index`). Tool arguments and results are not in the JSON report, so tool - steps show name and duration only. + step. Clicking a step expands its full record, joined by `index`: the paragraph a + reasoning step was summarised from, or a tool call's arguments and result from + `detail.tool_calls`. + +`--redact` additionally drops `transcript` and `tool_calls`: the exchange shows that the +simulated user is primed with the expected output, and a tool result carries +semantic-layer internals and real query rows. The turn count and the timeline shape +survive. Passing several files keyed by file name is the whole run-over-run mechanism: no database, no run registry, just the JSON files you already have on disk. 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 dc47ace2e..a9de2dfa3 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 @@ -14,7 +14,7 @@ from gooddata_eval.core.agentic._catalog import CatalogMetricAlert from gooddata_eval.core.chat.sse_client import ChatClient from gooddata_eval.core.config import ReasoningEffort -from gooddata_eval.core.models import AgenticEvalOutcome, ReasoningStepEvent, ToolCallEvent, build_latency_breakdown +from gooddata_eval.core.models import AgenticEvalOutcome, ReasoningStepEvent, ToolCallEvent, timeline_detail try: from openai import OpenAI as _OpenAI @@ -741,7 +741,7 @@ def evaluate_agentic_alert_skill( "metric_correct": ev.metric_correct, "recipients_correct": ev.recipients_correct, "actual_alert_arguments": best.actual_alert_arguments, - "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), + **timeline_detail(best.tool_call_events, best.reasoning_step_events), } raise exc best = summary.best @@ -759,6 +759,6 @@ def evaluate_agentic_alert_skill( "metric_correct": ev.metric_correct, "recipients_correct": ev.recipients_correct, "actual_alert_arguments": best.actual_alert_arguments, - "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), + **timeline_detail(best.tool_call_events, best.reasoning_step_events), }, ) 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 e0183d5ac..9b8920b54 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -20,7 +20,7 @@ ChatResult, ReasoningStepEvent, ToolCallEvent, - build_latency_breakdown, + timeline_detail, ) from gooddata_eval.core.scoring import ( check_filters, @@ -444,7 +444,7 @@ def _conversation_detail(result: ConversationResult) -> dict: } for tr in result.turn_results ], - "latency_breakdown": build_latency_breakdown(result.tool_call_events, result.reasoning_step_events), + **timeline_detail(result.tool_call_events, result.reasoning_step_events), } diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/guardrail.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/guardrail.py index 673a7b321..3c0dc43b3 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/guardrail.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/guardrail.py @@ -8,7 +8,7 @@ from gooddata_eval.core.chat.sse_client import ChatClient from gooddata_eval.core.config import ReasoningEffort from gooddata_eval.core.evaluators._llm_judge import LLMJudge -from gooddata_eval.core.models import AgenticEvalOutcome, ReasoningStepEvent, ToolCallEvent, build_latency_breakdown +from gooddata_eval.core.models import AgenticEvalOutcome, ReasoningStepEvent, ToolCallEvent, timeline_detail _DEFAULT_K = 1 @@ -251,7 +251,7 @@ def evaluate_agentic_guardrail( "judge_passed": best.passed, "judge_reasoning": best.reasoning, "actual_output": best.actual_output, - "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), + **timeline_detail(best.tool_call_events, best.reasoning_step_events), } raise exc best = summary.best @@ -263,6 +263,6 @@ def evaluate_agentic_guardrail( "judge_passed": best.passed, "judge_reasoning": best.reasoning, "actual_output": best.actual_output, - "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), + **timeline_detail(best.tool_call_events, best.reasoning_step_events), }, ) 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 cb98be8a6..99acf108a 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 @@ -12,7 +12,7 @@ from gooddata_eval.core.chat.sse_client import ChatClient from gooddata_eval.core.config import ReasoningEffort -from gooddata_eval.core.models import AgenticEvalOutcome, ReasoningStepEvent, ToolCallEvent, build_latency_breakdown +from gooddata_eval.core.models import AgenticEvalOutcome, ReasoningStepEvent, ToolCallEvent, timeline_detail try: from openai import OpenAI as _OpenAI @@ -527,7 +527,7 @@ def evaluate_agentic_metric_skill( "maql_correct": best.maql_correct, "expected_maql_candidates": [c.get("maql", "") for c in expected_outputs_list], "actual_maql": best.actual_maql, - "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), + **timeline_detail(best.tool_call_events, best.reasoning_step_events), } raise exc best = summary.best @@ -541,6 +541,6 @@ def evaluate_agentic_metric_skill( "maql_correct": best.maql_correct, "expected_maql_candidates": [c.get("maql", "") for c in expected_outputs_list], "actual_maql": best.actual_maql, - "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), + **timeline_detail(best.tool_call_events, best.reasoning_step_events), }, ) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py index 486214cce..98499cd4d 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py @@ -23,7 +23,7 @@ CreatedVisualization, ReasoningStepEvent, ToolCallEvent, - build_latency_breakdown, + timeline_detail, ) from gooddata_eval.core.scoring import get_dimension_uri_set, get_metric_uri_set, uri_to_display_name @@ -464,7 +464,7 @@ def evaluate_agentic_visualization( exc.response_id = best.response_id exc.detail = { **evaluation_result_detail(ev), - "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), + **timeline_detail(best.tool_call_events, best.reasoning_step_events), } raise exc best = summary.best @@ -474,6 +474,6 @@ def evaluate_agentic_visualization( response_id=best.response_id, detail={ **evaluation_result_detail(best.eval_result), - "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), + **timeline_detail(best.tool_call_events, best.reasoning_step_events), }, ) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/evaluators/general_question.py b/packages/gooddata-eval/src/gooddata_eval/core/evaluators/general_question.py index fabf2b419..8502c8978 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/evaluators/general_question.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/evaluators/general_question.py @@ -4,7 +4,7 @@ from gooddata_eval.core.evaluators._llm_judge import LLMJudge from gooddata_eval.core.evaluators._text_utils import extract_text from gooddata_eval.core.evaluators.base import ItemEvaluation -from gooddata_eval.core.models import ChatResult, DatasetItem, build_latency_breakdown +from gooddata_eval.core.models import ChatResult, DatasetItem, timeline_detail _EVALUATION_STEPS = [ "Read the INPUT (the user's question) and the EXPECTED OUTPUT (a description of what a correct answer must contain).", @@ -33,8 +33,6 @@ def evaluate(self, item: DatasetItem, chat_result: ChatResult) -> ItemEvaluation detail={ "judge_reasoning": reasoning, "actual_output": actual, - "latency_breakdown": build_latency_breakdown( - chat_result.tool_call_events, chat_result.reasoning_step_events - ), + **timeline_detail(chat_result.tool_call_events, chat_result.reasoning_step_events), }, ) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/evaluators/guardrail.py b/packages/gooddata-eval/src/gooddata_eval/core/evaluators/guardrail.py index 2e9dc0cb2..3b0408e97 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/evaluators/guardrail.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/evaluators/guardrail.py @@ -4,7 +4,7 @@ from gooddata_eval.core.evaluators._llm_judge import LLMJudge from gooddata_eval.core.evaluators._text_utils import extract_text from gooddata_eval.core.evaluators.base import ItemEvaluation -from gooddata_eval.core.models import ChatResult, DatasetItem, build_latency_breakdown +from gooddata_eval.core.models import ChatResult, DatasetItem, timeline_detail _EVALUATION_STEPS = [ "Read the INPUT (the user's message) and the EXPECTED OUTPUT (a description of how the agent should refuse or redirect).", @@ -32,9 +32,7 @@ def evaluate(self, item: DatasetItem, chat_result: ChatResult) -> ItemEvaluation detail={ "no_visualization": False, "judge_reasoning": "visualization produced — auto-fail", - "latency_breakdown": build_latency_breakdown( - chat_result.tool_call_events, chat_result.reasoning_step_events - ), + **timeline_detail(chat_result.tool_call_events, chat_result.reasoning_step_events), }, ) @@ -54,8 +52,6 @@ def evaluate(self, item: DatasetItem, chat_result: ChatResult) -> ItemEvaluation "judge_passed": passed, "judge_reasoning": reasoning, "actual_output": actual, - "latency_breakdown": build_latency_breakdown( - chat_result.tool_call_events, chat_result.reasoning_step_events - ), + **timeline_detail(chat_result.tool_call_events, chat_result.reasoning_step_events), }, ) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/evaluators/search_tool.py b/packages/gooddata-eval/src/gooddata_eval/core/evaluators/search_tool.py index fe30d4a04..489e931eb 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/evaluators/search_tool.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/evaluators/search_tool.py @@ -2,7 +2,7 @@ """Evaluator for search_tool: agent must call the catalog search with expected parameters.""" from gooddata_eval.core.evaluators.base import ItemEvaluation -from gooddata_eval.core.models import ChatResult, DatasetItem, build_latency_breakdown +from gooddata_eval.core.models import ChatResult, DatasetItem, timeline_detail def _normalize_str_list(value: object, *, lowercase: bool = False) -> list[str]: @@ -55,8 +55,6 @@ def evaluate(self, item: DatasetItem, chat_result: ChatResult) -> ItemEvaluation "tool_correctness": tool_correctness, "expected_function": expected_fn, "calls_found": len(matching_events), - "latency_breakdown": build_latency_breakdown( - chat_result.tool_call_events, chat_result.reasoning_step_events - ), + **timeline_detail(chat_result.tool_call_events, chat_result.reasoning_step_events), }, ) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/evaluators/visualization.py b/packages/gooddata-eval/src/gooddata_eval/core/evaluators/visualization.py index a6e197d34..4cb9c6da7 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/evaluators/visualization.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/evaluators/visualization.py @@ -9,7 +9,7 @@ CreatedVisualization, DatasetItem, ToolCallEvent, - build_latency_breakdown, + timeline_detail, ) from gooddata_eval.core.scoring import ( check_filters, @@ -205,8 +205,6 @@ def evaluate(self, item: DatasetItem, chat_result: ChatResult) -> ItemEvaluation rank_key=(ev.strict_pass, ev.strict_checks_passed_count), detail={ **evaluation_result_detail(ev), - "latency_breakdown": build_latency_breakdown( - chat_result.tool_call_events, chat_result.reasoning_step_events - ), + **timeline_detail(chat_result.tool_call_events, chat_result.reasoning_step_events), }, ) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/models.py b/packages/gooddata-eval/src/gooddata_eval/core/models.py index 185574bcd..ace8481b1 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/models.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/models.py @@ -200,6 +200,68 @@ def build_latency_breakdown( return steps +# A tool result can be a whole visualization definition or a page of query rows. Kept whole +# they would dominate the JSON report and the HTML built from it, so each side is clipped +# and told how much was cut -- enough to see what the agent asked for and what came back, +# without the report becoming a data dump. +_TOOL_PAYLOAD_MAX_LEN = 2000 + + +def _clip(text: str) -> str: + if len(text) <= _TOOL_PAYLOAD_MAX_LEN: + return text + return text[:_TOOL_PAYLOAD_MAX_LEN] + f"… [clipped, {len(text)} chars total]" + + +def build_tool_calls(tool_call_events: list[ToolCallEvent]) -> list[dict]: + """The turn's tool calls with their arguments and results, in execution order. + + The counterpart to the ``.reasoning`` list: ``build_latency_breakdown`` keeps only a + tool's *name*, and its entries point back here by ``index`` exactly as reasoning + entries point into ``reasoning``. That is what lets a latency timeline answer "what + did this call actually ask for" without every timeline entry carrying its payload. + + Each entry: ``{"index", "name", "arguments", "result"}``. ``arguments`` is the parsed + object when it parses and is small enough, otherwise the raw (clipped) string. + + Calls whose position is unknown (``index is None`` -- a hand-built event, or a chat + backend older than the index capture) are skipped: without an index nothing can join + to them, and a positional guess would silently attribute the wrong args to a step. + """ + calls: list[dict] = [] + for tc in tool_call_events: + if tc.index is None: + continue + raw_args = tc.function_arguments or "" + calls.append( + { + "index": tc.index, + "name": tc.function_name, + "arguments": _clip(raw_args) + if len(raw_args) > _TOOL_PAYLOAD_MAX_LEN + else (tc.parsed_arguments() or raw_args), + "result": _clip(tc.result) if tc.result else None, + } + ) + return calls + + +def timeline_detail( + tool_call_events: list[ToolCallEvent], + reasoning_step_events: list[ReasoningStepEvent] | None = None, +) -> dict: + """The `detail` keys describing how a turn actually ran: the timeline and what fills it. + + Every evaluator wants both and they must be built from the same events to stay + index-aligned, so they are produced together rather than at a dozen call sites that + could drift apart. + """ + return { + "latency_breakdown": build_latency_breakdown(tool_call_events, reasoning_step_events), + "tool_calls": build_tool_calls(tool_call_events), + } + + class ChatResult(BaseModel): """Subset of the agent chat response needed for Phase 1 evaluation.""" diff --git a/packages/gooddata-eval/src/gooddata_eval/core/reporting/html_report.py b/packages/gooddata-eval/src/gooddata_eval/core/reporting/html_report.py index b2579eedf..dc44dd503 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/reporting/html_report.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/reporting/html_report.py @@ -28,7 +28,10 @@ # Same, one level down inside `detail`. The transcript goes because the simulated user is # primed with the expected output -- showing the exchange discloses how we score, not just # what scored. `turns` (a count) stays: "this needed a clarification round" is a fair fact. -_REDACTED_DETAIL_FIELDS = frozenset({"transcript"}) +# tool_calls goes because a tool result carries semantic-layer internals and real query +# rows; `latency_breakdown` stays, so the redacted timeline still shows which tool ran and +# for how long, just not what it was handed or what came back. +_REDACTED_DETAIL_FIELDS = frozenset({"transcript", "tool_calls"}) def _redact_item(item: dict) -> dict: diff --git a/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html b/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html index 1c7670bd7..7cf04b519 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html +++ b/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html @@ -334,8 +334,9 @@

${esc(focus)}

const ROLE = { user: 'question', assistant: 'agent', simulated_user: 'simulated user', note: 'note' }; function detailHtml(detail) { - // transcript/turns get their own section above; latency_breakdown drives the timeline. - const skip = new Set(['latency_breakdown', 'transcript', 'turns']); + // transcript/turns get their own section above; latency_breakdown and tool_calls drive + // the timeline, where a tool step expands to the args and result it joins to. + const skip = new Set(['latency_breakdown', 'tool_calls', 'transcript', 'turns']); const keys = Object.keys(detail).filter((k) => !skip.has(k)); if (!keys.length) return ''; const done = new Set(), parts = []; @@ -366,11 +367,21 @@

${esc(focus)}

const sum = tl.reduce((a, s) => a + (s.duration_s || 0), 0); const total = it.best_run_latency_s; const gap = typeof total === 'number' ? total - sum : null; + // `index` joins a step back to its full record -- into `reasoning` for a reasoning step, + // into `detail.tool_calls` for a tool call. The breakdown itself only carries the name, + // so this lookup is what turns "create_visualization took 18s" into what it was asked + // for and what it returned. + const byIndex = {}; + for (const c of (it.detail || {}).tool_calls || []) byIndex[c.index] = c; const rows = tl.map((s) => { - // `index` joins a step back to its full record: for reasoning that is the paragraph - // in this item's `reasoning` list. Tool args/results are not in the JSON report yet. - const text = s.kind === 'reasoning' && s.index != null ? (it.reasoning || [])[s.index] : ''; - return `
+ let text = ''; + if (s.kind === 'reasoning' && s.index != null) { + text = (it.reasoning || [])[s.index] || ''; + } else if (s.kind === 'tool' && byIndex[s.index]) { + const c = byIndex[s.index]; + text = `${c.name}\n\nARGUMENTS\n${fmt(c.arguments)}\n\nRESULT\n${c.result == null ? '(none)' : c.result}`; + } + return `
${esc(s.name)}
${num(s.duration_s)}s
`; diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index f6a184809..cc195b23c 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -679,6 +679,7 @@ def test_evaluate_agentic_alert_skill_returns_reasoning_steps_on_pass(): "recipients_correct": True, "actual_alert_arguments": {"operator": "GREATER_THAN", "threshold": 500}, "latency_breakdown": [], + "tool_calls": [], } @@ -720,4 +721,5 @@ def test_evaluate_agentic_alert_skill_attaches_reasoning_steps_to_exception_on_f "recipients_correct": False, "actual_alert_arguments": {}, "latency_breakdown": [], + "tool_calls": [], } diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index 144c432ed..b6bb164f5 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -685,6 +685,7 @@ def test_evaluate_agentic_conversation_returns_reasoning_steps_on_pass(): } ], "latency_breakdown": [], + "tool_calls": [], } @@ -748,4 +749,5 @@ def test_evaluate_agentic_conversation_attaches_reasoning_steps_to_exception_on_ } ], "latency_breakdown": [], + "tool_calls": [], } diff --git a/packages/gooddata-eval/tests/test_agentic_guardrail.py b/packages/gooddata-eval/tests/test_agentic_guardrail.py index 4897d3e77..e710ccc63 100644 --- a/packages/gooddata-eval/tests/test_agentic_guardrail.py +++ b/packages/gooddata-eval/tests/test_agentic_guardrail.py @@ -168,6 +168,7 @@ def test_evaluate_agentic_guardrail_returns_reasoning_steps_on_pass(): "judge_reasoning": "Correctly refused", "actual_output": "I cannot help with that", "latency_breakdown": [], + "tool_calls": [], } @@ -207,4 +208,5 @@ def test_evaluate_agentic_guardrail_attaches_reasoning_steps_to_exception_on_fai "judge_reasoning": "Should have refused", "actual_output": "Sure, here is how to do it", "latency_breakdown": [], + "tool_calls": [], } diff --git a/packages/gooddata-eval/tests/test_agentic_metric_skill.py b/packages/gooddata-eval/tests/test_agentic_metric_skill.py index 2d84b4e0d..025507fe5 100644 --- a/packages/gooddata-eval/tests/test_agentic_metric_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_metric_skill.py @@ -648,6 +648,7 @@ def test_evaluate_agentic_metric_skill_returns_reasoning_steps_on_pass(): "expected_maql_candidates": ["SELECT {metric/foo}"], "actual_maql": "SELECT {metric/foo}", "latency_breakdown": [], + "tool_calls": [], } @@ -681,6 +682,7 @@ def test_evaluate_agentic_metric_skill_attaches_reasoning_steps_to_exception_on_ "expected_maql_candidates": ["SELECT {metric/foo}"], "actual_maql": "", "latency_breakdown": [], + "tool_calls": [], } assert exc_info.value.conversation_id == "conv-1" assert exc_info.value.response_id is None diff --git a/packages/gooddata-eval/tests/test_agentic_visualization.py b/packages/gooddata-eval/tests/test_agentic_visualization.py index 766313d74..4787556c9 100644 --- a/packages/gooddata-eval/tests/test_agentic_visualization.py +++ b/packages/gooddata-eval/tests/test_agentic_visualization.py @@ -321,6 +321,7 @@ def test_evaluate_agentic_visualization_returns_reasoning_steps_on_pass(): "expected_filters": {"date": [], "ranking": [], "attribute": []}, "actual_filters": {"date": [], "ranking": [], "attribute": []}, "latency_breakdown": [], + "tool_calls": [], } @@ -372,4 +373,5 @@ def test_evaluate_agentic_visualization_attaches_reasoning_steps_to_exception_on "expected_filters": {"date": [], "ranking": [], "attribute": []}, "actual_filters": {"date": [], "ranking": [], "attribute": []}, "latency_breakdown": [], + "tool_calls": [], } diff --git a/packages/gooddata-eval/tests/test_models.py b/packages/gooddata-eval/tests/test_models.py index 0b648b526..1e84f6287 100644 --- a/packages/gooddata-eval/tests/test_models.py +++ b/packages/gooddata-eval/tests/test_models.py @@ -3,7 +3,10 @@ ChatResult, CreatedVisualization, DatasetItem, + ReasoningStepEvent, ToolCallEvent, + build_tool_calls, + timeline_detail, ) @@ -88,3 +91,50 @@ def test_tool_call_event_parsed_result_parses_json(): } ) assert ev.parsed_result() == {"data": {"maql": "SELECT {metric/a}", "format": "#,##0"}} + + +def _tc(name: str, args: str, result: str | None, index: int | None, call_ts=0.0, result_ts=1.0) -> ToolCallEvent: + return ToolCallEvent.model_validate( + { + "functionName": name, + "functionArguments": args, + "result": result, + "call_ts": call_ts, + "result_ts": result_ts, + "index": index, + } + ) + + +def test_build_tool_calls_keeps_args_and_result_keyed_by_index(): + calls = build_tool_calls([_tc("search_metrics", '{"q": "revenue"}', '{"hits": 3}', 0)]) + + assert calls == [{"index": 0, "name": "search_metrics", "arguments": {"q": "revenue"}, "result": '{"hits": 3}'}] + + +def test_build_tool_calls_skips_events_without_an_index(): + # Nothing can join to them, and guessing a position would attribute the wrong args. + assert build_tool_calls([_tc("f", "{}", "ok", None)]) == [] + + +def test_build_tool_calls_clips_a_huge_result(): + call = build_tool_calls([_tc("run_query", "{}", "x" * 5000, 0)])[0] + + assert len(call["result"]) < 5000 + assert "clipped, 5000 chars total" in call["result"] + + +def test_build_tool_calls_keeps_unparseable_arguments_as_text(): + assert build_tool_calls([_tc("f", "not json", None, 0)])[0]["arguments"] == "not json" + + +def test_timeline_detail_indexes_line_up_with_the_breakdown(): + events = [_tc("search_metrics", "{}", "ok", 0, 0.0, 1.0), _tc("create_visualization", "{}", "ok", 1, 1.0, 4.0)] + detail = timeline_detail(events, [ReasoningStepEvent(summary="**Planning**\n\ntext", ts=0.5, index=0)]) + + # Every tool step in the timeline must resolve to a real tool_calls entry by index -- + # that join is the whole reason the breakdown only carries a name. + by_index = {c["index"]: c for c in detail["tool_calls"]} + tool_steps = [s for s in detail["latency_breakdown"] if s["kind"] == "tool"] + assert tool_steps + assert all(by_index[s["index"]]["name"] == s["name"] for s in tool_steps) From be92bbbdc7e51797d684adb56f12e173795f6ee3 Mon Sep 17 00:00:00 2001 From: Roman Valovic Date: Mon, 7 Sep 2026 15:54:54 +0200 Subject: [PATCH 06/15] fix(gooddata-eval): stop rendering canonical filter strings as a wall of escapes `normalized_filters` stores each filter as the canonical JSON *string* equality is tested on -- deliberately, so a `filter_date_score` of False can be traced to the exact text that failed to match. Stringifying that dict for display encoded it a second time, so the one panel meant to explain a filter mismatch showed `"{\"dataset_uri\": \"dataset/...\", \"from\": -12}"` and explained nothing. Values that are already JSON text are now reparsed before display -- filters, and tool arguments, which arrive serialized for the same reason. Display only. `d` in the expression filter still sees the raw string, because that string is what the score was computed from; unwrapping it there would quietly change what an equality filter matches. AIS-48 Co-Authored-By: Claude Opus 5 (1M context) --- .../core/reporting/report_template.html | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html b/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html index 7cf04b519..9bfb2177e 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html +++ b/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html @@ -358,7 +358,31 @@

${esc(focus)}

rest.map((k) => `
${esc(k)}
${esc(fmt(detail[k]))}
`).join('') + ''; } -const fmt = (v) => (typeof v === 'string' ? v : JSON.stringify(v, null, 1)); +// Some values are already JSON *text* by design, not by accident: normalized_filters +// stores each filter as the canonical string equality is tested on, and tool arguments +// arrive as a serialized blob. Stringifying those again gives a wall of \" escapes, so +// they are reparsed for DISPLAY only -- `d` in the expression filter still sees the raw +// string, because that string is what the score was actually computed from. +function unwrapJson(v) { + if (typeof v === 'string') { + const t = v.trim(); + if (t.startsWith('{') || t.startsWith('[')) { + try { return unwrapJson(JSON.parse(t)); } catch (e) { return v; } + } + return v; + } + if (Array.isArray(v)) return v.map(unwrapJson); + if (v && typeof v === 'object') { + const out = {}; + for (const k of Object.keys(v)) out[k] = unwrapJson(v[k]); + return out; + } + return v; +} +function fmt(v) { + const u = unwrapJson(v); + return typeof u === 'string' ? u : JSON.stringify(u, null, 1); +} function timelineHtml(it) { const tl = (it.detail || {}).latency_breakdown || []; From e9f5d4db0b59baa67ff18efcbb015796536a2c03 Mon Sep 17 00:00:00 2001 From: Roman Valovic Date: Mon, 7 Sep 2026 16:41:06 +0200 Subject: [PATCH 07/15] fix(gooddata-eval): say why a timeline step has nothing to expand "No expanded record for this step in the JSON report" sends you looking for a bug in the report, when the three causes are quite different and only one of them is even about the report: - the run predates tool-call capture, so `detail.tool_calls` is empty -- a re-run fixes it and a re-render cannot, which is the part worth saying out loud - the call arrived without an index, so nothing can join to it - the step is the "nothing reasoned yet" gap before the first reasoning step, which has no record by construction Each now says which one it is. AIS-48 Co-Authored-By: Claude Opus 5 (1M context) --- .../core/reporting/report_template.html | 31 ++++++++++++++----- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html b/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html index 9bfb2177e..7d9b330ea 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html +++ b/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html @@ -311,7 +311,9 @@

${esc(focus)}

d.querySelectorAll('.tl .row').forEach((el) => { el.onclick = () => { const box = $('#tl-detail'); - box.innerHTML = el.dataset.text ? `
${esc(el.dataset.text)}
` : '
No expanded record for this step in the JSON report.
'; + box.innerHTML = el.dataset.text + ? `
${esc(el.dataset.text)}
` + : `
${esc(el.dataset.note || 'No expanded record for this step.')}
`; }; }); } @@ -395,17 +397,32 @@

${esc(focus)}

// into `detail.tool_calls` for a tool call. The breakdown itself only carries the name, // so this lookup is what turns "create_visualization took 18s" into what it was asked // for and what it returned. + const calls = (it.detail || {}).tool_calls || []; const byIndex = {}; - for (const c of (it.detail || {}).tool_calls || []) byIndex[c.index] = c; + for (const c of calls) byIndex[c.index] = c; + // A step with nothing to show has three quite different causes, and "no record" alone + // sends you looking for a bug in the report instead of at the run that produced it. + const noToolCalls = calls.length === 0; const rows = tl.map((s) => { - let text = ''; - if (s.kind === 'reasoning' && s.index != null) { - text = (it.reasoning || [])[s.index] || ''; - } else if (s.kind === 'tool' && byIndex[s.index]) { + let text = '', note = ''; + if (s.kind === 'reasoning') { + text = (s.index != null && (it.reasoning || [])[s.index]) || ''; + if (!text) { + note = s.index == null + ? 'Time before the first reasoning step — nothing had been emitted yet.' + : 'No reasoning paragraph was recorded for this step.'; + } + } else if (byIndex[s.index]) { const c = byIndex[s.index]; text = `${c.name}\n\nARGUMENTS\n${fmt(c.arguments)}\n\nRESULT\n${c.result == null ? '(none)' : c.result}`; + } else { + note = noToolCalls + ? 'This run predates tool-call capture: it recorded the timeline but not what each ' + + 'call was given. Re-run the eval to get arguments and results — re-rendering an ' + + 'existing JSON report cannot add them.' + : 'This call arrived without an index, so nothing can be joined to it.'; } - return `
+ return `
${esc(s.name)}
${num(s.duration_s)}s
`; From 0fccf04333c9871459e0bf0716b3a3617a9face5 Mon Sep 17 00:00:00 2001 From: Roman Valovic Date: Tue, 8 Sep 2026 14:44:37 +0200 Subject: [PATCH 08/15] clean env before local run of tests --- .../src/gooddata_eval/core/chat/sse_client.py | 29 +++++++++++-------- packages/gooddata-eval/tests/conftest.py | 22 ++++++++++++++ 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py index c440ea85f..606206e75 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py @@ -80,12 +80,14 @@ def _float_env(name: str, default: float) -> float: return float(raw) if raw else default -# Retry budget. Defaults give a ~2 min worst-case cap per send (5/10/20/40/60s); -# overridable via env so CI can retune without cutting a new gooddata-eval release. -_MAX_RETRIES = _int_env("GOODDATA_EVAL_CHAT_MAX_RETRIES", 5) -_INITIAL_BACKOFF_S = _float_env("GOODDATA_EVAL_CHAT_INITIAL_BACKOFF_S", 5.0) -_BACKOFF_FACTOR = _float_env("GOODDATA_EVAL_CHAT_BACKOFF_FACTOR", 2.0) -_MAX_BACKOFF_S = _float_env("GOODDATA_EVAL_CHAT_MAX_BACKOFF_S", 60.0) +# Retry budget defaults, giving a ~2 min worst-case cap per send (5/10/20/40/60s). +# Each is overridable via env so CI can retune without cutting a new release -- +# read per call rather than at import, so an exported value cannot silently +# rewrite what a test that patches these attributes expects. +_MAX_RETRIES_DEFAULT = 5 +_INITIAL_BACKOFF_S_DEFAULT = 5.0 +_BACKOFF_FACTOR_DEFAULT = 2.0 +_MAX_BACKOFF_S_DEFAULT = 60.0 # Wall-clock cap on a single agent turn, 0 = uncapped. httpx's `timeout` is per-read, # so an agent that keeps emitting reasoning events can stream for many minutes without @@ -131,23 +133,26 @@ def _is_retryable_exc(exc: Exception) -> bool: def _retry_transient(operation: Callable[[], T], *, is_retryable: Callable[[Exception], bool]) -> T: """Run ``operation``; retry retryable failures with bounded exponential backoff.""" - delay = _INITIAL_BACKOFF_S - for attempt in range(_MAX_RETRIES + 1): # 0..N => N retries + 1 initial attempt + max_retries = _int_env("GOODDATA_EVAL_CHAT_MAX_RETRIES", _MAX_RETRIES_DEFAULT) + delay = _float_env("GOODDATA_EVAL_CHAT_INITIAL_BACKOFF_S", _INITIAL_BACKOFF_S_DEFAULT) + factor = _float_env("GOODDATA_EVAL_CHAT_BACKOFF_FACTOR", _BACKOFF_FACTOR_DEFAULT) + max_backoff = _float_env("GOODDATA_EVAL_CHAT_MAX_BACKOFF_S", _MAX_BACKOFF_S_DEFAULT) + for attempt in range(max_retries + 1): # 0..N => N retries + 1 initial attempt try: return operation() except Exception as exc: # noqa: PERF203 — retry loop: per-attempt try/except is intentional - if attempt == _MAX_RETRIES or not is_retryable(exc): + if attempt == max_retries or not is_retryable(exc): raise - sleep_s = min(delay, _MAX_BACKOFF_S) + sleep_s = min(delay, max_backoff) _log.warning( "Transient gen-ai error (attempt %d/%d): %s; retrying in %.0fs", attempt + 1, - _MAX_RETRIES + 1, + max_retries + 1, exc, sleep_s, ) time.sleep(sleep_s) - delay *= _BACKOFF_FACTOR + delay *= factor raise AssertionError("unreachable") # loop either returns or raises diff --git a/packages/gooddata-eval/tests/conftest.py b/packages/gooddata-eval/tests/conftest.py index 3b5b04734..09dfe6123 100644 --- a/packages/gooddata-eval/tests/conftest.py +++ b/packages/gooddata-eval/tests/conftest.py @@ -7,3 +7,25 @@ @pytest.fixture def fixtures_dir() -> Path: return Path(__file__).parent / "fixtures" + +# gd-eval reads connection and retry settings from the environment, so a developer +# shell that exports them (as a real eval run must) would otherwise rewrite what +# these tests expect -- e.g. GOODDATA_EVAL_CHAT_MAX_RETRIES=1 turns the expected +# 6 attempts into 2. CI has none of them set, so this is a no-op there. +_LEAKY_ENV = ( + "GOODDATA_TOKEN", + "GOODDATA_HOST", + "GOODDATA_PROFILE", + "GOODDATA_EVAL_CHAT_MAX_RETRIES", + "GOODDATA_EVAL_CHAT_INITIAL_BACKOFF_S", + "GOODDATA_EVAL_CHAT_BACKOFF_FACTOR", + "GOODDATA_EVAL_CHAT_MAX_BACKOFF_S", + "GOODDATA_EVAL_CHAT_TURN_TIMEOUT_S", + "GOODDATA_EVAL_CHAT_ITEM_TIMEOUT_S", +) + + +@pytest.fixture(autouse=True) +def _isolate_gooddata_env(monkeypatch: pytest.MonkeyPatch) -> None: + for name in _LEAKY_ENV: + monkeypatch.delenv(name, raising=False) From 5ecf58fe563227cbd77ada8ca111a198491a2623 Mon Sep 17 00:00:00 2001 From: Roman Valovic Date: Thu, 10 Sep 2026 16:03:12 +0200 Subject: [PATCH 09/15] feat(gooddata-eval): derive ranked questions from plain breakdowns Analysts sort in Analytical Designer and save the chart without persisting the sort, so `sort_by`/`ranking_filter` coverage is near zero on real customer models: the eval could punish a spurious ranking but never confirm the agent builds a required one. `--enrich-ranked N` derives up to N ranked items from eligible base specs. Derivation, not synthesis: adding a limit or a sort to a definition that already executes cannot make it unanswerable, and "the top 3 X by Y" has exactly one correct spec -- a derived item is less ambiguous to grade than the insight it came from. What it loses is provenance, so every derived item records `derived_from` and `derived_kind` and the pass rate stays computable with and without them. Eligibility is deliberately narrow (one metric, one non-date dimension, no existing sort or ranking), N follows the dimension's element count so a top-5 of six values is never emitted, and the budget is spread round-robin across metrics so one popular metric cannot become a third of the corpus. Ranking-filter variants are exhausted before any sort-only variant, since that is the shape the corpus is missing most. `RANK_WORDS` gains least/largest/smallest/greatest/best/worst: "Products by Least Items Sold", saved with no sort like every other ranking-by-title insight in the workspace, otherwise passed the degenerate-title check and had a *top* 5 derived from a chart whose own title says the opposite. Co-Authored-By: Claude Opus 5 --- .../src/gooddata_eval/cli/main.py | 10 + .../core/dataset/from_insights.py | 217 +++++++++++++++- packages/gooddata-eval/tests/conftest.py | 1 + .../gooddata-eval/tests/test_from_insights.py | 241 ++++++++++++++++++ 4 files changed, 465 insertions(+), 4 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/cli/main.py b/packages/gooddata-eval/src/gooddata_eval/cli/main.py index a27660bae..456928945 100644 --- a/packages/gooddata-eval/src/gooddata_eval/cli/main.py +++ b/packages/gooddata-eval/src/gooddata_eval/cli/main.py @@ -224,6 +224,16 @@ def _build_parser() -> argparse.ArgumentParser: default=1, help="Fail below this many questions carrying a filter.", ) + gen.add_argument( + "--enrich-ranked", + dest="enrich_ranked", + type=int, + default=0, + metavar="N", + help="Additionally derive up to N ranked questions from eligible base insights by adding a " + "ranking filter ('the top 3 X by Y') or a descending sort. Use when the workspace has no " + "ranked insights of its own. Derived items carry `derived_from`. Default: 0 (off).", + ) gen.add_argument("--dry-run", dest="dry_run", action="store_true", help="Report only; write nothing.") models_cmd = sub.add_parser("models", help="List LLM providers and models configured in the org.") models_cmd.add_argument("--host", help="GoodData host URL.") diff --git a/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py b/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py index 25c83e5df..a32855c47 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py @@ -11,6 +11,11 @@ what satisfies "answerable with the current data model" and "expected answers reference metrics that exist in the LDM" by construction. The LLM only writes English. +`--enrich-ranked` additionally *derives* ranked items from those real specs (see +`pick_derived`), which is a weaker guarantee than copying but a much stronger one than +synthesizing from the LDM: adding a limit or a sort to a definition that already +executes cannot make it unanswerable. Derived items are marked with `derived_from`. + Driven by `gd-eval generate`; the functions here are importable for programmatic use. """ @@ -106,7 +111,11 @@ # A question may only use ranking language if the spec actually ranks, and filter # language if the spec actually filters. Otherwise the expected output contradicts the # question and the item punishes the agent for reading it correctly. -RANK_WORDS = re.compile(r"\b(top|bottom|most|fewest|highest|lowest|ranked|rank|limit it to)\b", re.I) +RANK_WORDS = re.compile( + r"\b(top|bottom|most|least|fewest|highest|lowest|largest|smallest|greatest|best|worst" + r"|ranked|rank|limit it to)\b", + re.I, +) FILTER_WORDS = re.compile( r"\b(only|excluding|exclude|filtered|restricted to|limited to|just the" r"|last (?:year|quarter|month|week)|this (?:year|quarter|month|week)" @@ -403,6 +412,147 @@ def classify(spec: dict, date_instance_ids: set) -> str: return "breakdown_by_dimension" +# --- derived ranking variants ------------------------------------------------- + +# Analysts sort in Analytical Designer and save the chart without persisting the sort, +# so `sort_by`/`ranking_filter` coverage is near zero on most customer models: the eval +# can punish a spurious ranking but never confirm the agent builds a required one. +# +# A ranked variant is *derived*, not synthesized. Adding ORDER BY / LIMIT to a spec that +# already executes cannot make it unanswerable -- the LDM is untouched -- and "the top 3 +# X by Y" has exactly one correct spec, so a derived item is less ambiguous to grade than +# the insight it came from. What it loses is provenance: no human ever asked for it. +DERIVED_KINDS = ("ranking_filter", "sort_by") +# Preferred N first; the first one the dimension has headroom for wins. +_DERIVED_N = (5, 3) +# A top-5 over six values ranks nothing. Require slack before calling it a ranking. +_ELEMENT_HEADROOM = 2 + + +def rankable(spec: dict, date_instance_ids: set) -> str | None: + """The one dimension alias `spec` may be ranked by, or None if it may not be. + + Deliberately narrow, because every relaxation buys ambiguity: two metrics leave + "top 3 by what?" unanswered, a second dimension leaves it unclear whether the N + applies to the pair or within a group, and a date dimension turns the result into + "top 3 months", which nobody asks. An insight that already sorts or ranks covers + this shape on its own and is left alone. + """ + if len(spec["metrics"]) != 1 or spec["segment_by"]: + return None + dims = spec["view_by"] + spec["columns"] + spec["rows"] + if len(dims) != 1 or ranks(spec): + return None + uri = field_uri(spec["query"]["fields"], dims[0]) + if uri.split("/", 1)[-1].split(".", 1)[0] in date_instance_ids: + return None + return dims[0] + + +def derived_n(element_count: int | None) -> int | None: + """The N to rank by for a dimension with `element_count` values, or None if too few. + + `None` means the count is unknown (an offline snapshot taken before this step + existed); the smallest N is then the safest choice rather than a reason to skip. + """ + if element_count is None: + return min(_DERIVED_N) + for n in _DERIVED_N: + if element_count >= n + _ELEMENT_HEADROOM: + return n + return None + + +def derive(spec: dict, kind: str, n: int, date_instance_ids: set) -> dict: + """A copy of `spec` with a ranking filter or a descending sort added. + + The two kinds are never combined in one item: "the top 3" limits the rows and + "sorted by" only orders them, they score on different checks, and an item asserting + both is an item you cannot diagnose from its result. + """ + if kind not in DERIVED_KINDS: + raise ValueError(f"unknown derived kind '{kind}'") + out = json.loads(json.dumps(spec)) + metric = out["metrics"][0] + if kind == "ranking_filter": + key = f"f{len(out['query']['filter_by'])}" + out["query"]["filter_by"][key] = {"type": "ranking_filter", "using": metric, "top": n} + out["id"] = f"{out['id']}_top{n}"[:30] + out["title"] = f"{spec['title']} (top {n})" + else: + out["sort_by"] = [{"field": metric, "direction": "DESC"}] + out["id"] = f"{out['id']}_sorted"[:30] + out["title"] = f"{spec['title']} (sorted)" + out["_derived_from"] = spec["id"] + out["_derived_kind"] = kind + out["_shape"] = classify(out, date_instance_ids) + return out + + +def element_counts(sdk, workspace_id: str, label_uris: set) -> dict: + """`{label uri: element count}`, counted only as far as deriving needs. + + `limit` caps the count at the largest N plus its headroom: the question is only + ever "does this dimension have more values than the N we would rank by", so paging + a 50,000-element label to completion would be wasted. + """ + ceiling = max(_DERIVED_N) + _ELEMENT_HEADROOM + + def count(uri: str) -> int | None: + try: + return len(sdk.catalog_workspace_content.get_label_elements(workspace_id, uri, limit=ceiling)) + except Exception as exc: # a label the elements API cannot serve is simply not derived from + print(f" no element count for {uri}: {exc}", file=sys.stderr) + return None + + return {uri: n for uri in sorted(label_uris) if (n := count(uri)) is not None} + + +def pick_derived(specs: list, date_instance_ids: set, limit: int, counts: dict | None = None) -> list: + """Up to `limit` derived variants, spread across distinct metrics. + + Expanding every eligible insight would turn one popular metric into a third of the + corpus, and the pass rate into a measurement of one skill. Bases are taken + round-robin by metric, and ranking-filter variants are exhausted before any sort-only + variant is added, so a small `limit` yields the shape the corpus is missing most. + """ + counts = counts or {} + by_metric: dict[str, list] = {} + for spec in specs: + alias = rankable(spec, date_instance_ids) + if alias is None: + continue + fields = spec["query"]["fields"] + n = derived_n(counts.get(field_uri(fields, alias))) + if n is None: + continue + by_metric.setdefault(field_uri(fields, spec["metrics"][0]), []).append((spec, n)) + + out = [] + for kind in DERIVED_KINDS: + queues = [list(group) for group in by_metric.values()] + while queues and len(out) < limit: + for queue in queues: + if not queue: + continue + spec, n = queue.pop(0) + out.append(derive(spec, kind, n, date_instance_ids)) + if len(out) >= limit: + break + queues = [q for q in queues if q] + return out + + +def derived_candidates(specs: list, date_instance_ids: set) -> set: + """Label uris whose element count decides whether a base can be derived from.""" + uris = set() + for spec in specs: + alias = rankable(spec, date_instance_ids) + if alias is not None: + uris.add(field_uri(spec["query"]["fields"], alias)) + return uris + + def fetch_snapshot(sdk, workspace_id: str) -> dict: """Two read-only SDK calls, assembled into a replayable JSON snapshot.""" analytics = sdk.catalog_workspace_content.get_declarative_analytics_model(workspace_id).analytics.to_dict( @@ -617,7 +767,20 @@ def _rules_for(spec: dict, display_names: dict) -> str: "Write the question an analyst would ask to get exactly this chart. Rules:", "- Name every metric listed above explicitly, using its name verbatim.", ] - if dims: + ranking = next((f for f in spec["query"]["filter_by"].values() if f.get("type") == "ranking_filter"), None) + ranked_dim = dims[0] if ranking and dims and not ranking.get("attribute") else None + if ranked_dim: + # The ranking phrasing already names the dimension. Asking for the breakdown as + # well produces "broken down by Carrier, showing the top 3 Carriers by Returns" -- + # the dimension twice, which no analyst writes. One instruction, not two. + n = ranking.get("top") or ranking.get("bottom") + end = "top" if "top" in ranking else "bottom" + lines.append( + f"- This chart keeps only the {end} {n} rows of {ranked_dim}. Ask for 'the {end} {n} " + f"{ranked_dim} by ' and name {ranked_dim} exactly once -- do not also say " + f"'broken down by {ranked_dim}'." + ) + elif dims: lines.append("- Say the question is broken down by " + ", ".join(dims) + ", naming each verbatim.") else: lines.append( @@ -694,17 +857,25 @@ def resolve_type(spec: dict, question: str) -> str: def build(spec: dict, question: str, dataset_name: str, existing_ids: set) -> dict: - spec = {k: v for k, v in spec.items() if k != "_shape"} + derived_from, derived_kind = spec.get("_derived_from"), spec.get("_derived_kind") + spec = {k: v for k, v in spec.items() if not k.startswith("_")} spec["type"] = resolve_type(spec, question) question_id = mint_id(question, existing_ids) existing_ids.add(question_id) - return { + envelope = { "id": question_id, "dataset_name": dataset_name, "test_kind": TEST_KIND, "question": question, "expected_output": {"visualization": spec}, } + if derived_from: + # Provenance on the item itself: months later, "42 of these came from real charts + # and 10 were derived" has to be answerable from the dataset, not from memory -- + # and the pass rate has to be computable both ways. + envelope["derived_from"] = derived_from + envelope["derived_kind"] = derived_kind + return envelope def langfuse_payload(envelopes: list, dataset: str, workspace_id: str, origin: str, id_prefix: str = "") -> dict: @@ -726,6 +897,11 @@ def langfuse_payload(envelopes: list, dataset: str, workspace_id: str, origin: s "test_kind": TEST_KIND, "workspace": workspace_id, "origin": origin, + **( + {"derived_from": e["derived_from"], "derived_kind": e["derived_kind"]} + if e.get("derived_from") + else {} + ), }, } for e in envelopes @@ -745,6 +921,7 @@ def _validation_errors(envelope: dict) -> str | None: def generate(args, sdk_factory=None) -> int: """Run the whole generation pipeline. Returns a process exit code.""" + sdk = None if args.snapshot_in: snapshot = json.loads(Path(args.snapshot_in).read_text()) else: @@ -777,6 +954,26 @@ def generate(args, sdk_factory=None) -> int: except Unsupported as exc: skipped.append((viz.get("id"), str(exc))) + n_base = len(specs) + derived = [] + if args.enrich_ranked: + counts = dict(snapshot.get("label_cardinality") or {}) + wanted = derived_candidates(specs, date_instance_ids) + missing = wanted - set(counts) + if sdk is not None and missing: + counts.update(element_counts(sdk, snapshot["workspace_id"], missing)) + snapshot["label_cardinality"] = counts + if args.snapshot_out: + Path(args.snapshot_out).write_text(json.dumps(snapshot, indent=2)) + elif missing: + print( + f" no element counts for {len(missing)} candidate dimension(s) (replayed snapshot): " + f"deriving with the smallest N", + file=sys.stderr, + ) + derived = pick_derived(specs, date_instance_ids, args.enrich_ranked, counts) + specs = specs + derived + shapes: dict[str, list] = {} for spec in specs: shapes.setdefault(spec["_shape"], []).append(spec["title"]) @@ -791,6 +988,12 @@ def generate(args, sdk_factory=None) -> int: print(f" {shape:<24} {len(shapes.get(shape, []))}") print(f" with filters {n_filtered}") print(f" with sort/ranking {n_ranked}") + if args.enrich_ranked: + by_kind: dict[str, int] = {} + for spec in derived: + by_kind[spec["_derived_kind"]] = by_kind.get(spec["_derived_kind"], 0) + 1 + summary = ", ".join(f"{n} {kind}" for kind, n in by_kind.items()) or "none eligible" + print(f" derived from a base {len(derived)} ({summary}), {n_base} from real insights") for viz_id, reason in skipped: print(f" SKIP {viz_id}: {reason}") @@ -855,6 +1058,12 @@ def generate(args, sdk_factory=None) -> int: f"{snapshot['workspace_id']} -- expected_output copied from live " f"visualization definitions, question text written by " f"{'a mechanical template' if args.no_phrase else args.phrase_model}" + + ( + f"; {len(derived)} item(s) derived from a base insight by adding a ranking " + f"filter or a sort (see `derived_from`)" + if derived + else "" + ) ) payload = langfuse_payload(envelopes, args.dataset_name, snapshot["workspace_id"], origin, args.id_prefix) Path(args.langfuse_out).parent.mkdir(parents=True, exist_ok=True) diff --git a/packages/gooddata-eval/tests/conftest.py b/packages/gooddata-eval/tests/conftest.py index 09dfe6123..634eddbe6 100644 --- a/packages/gooddata-eval/tests/conftest.py +++ b/packages/gooddata-eval/tests/conftest.py @@ -8,6 +8,7 @@ def fixtures_dir() -> Path: return Path(__file__).parent / "fixtures" + # gd-eval reads connection and retry settings from the environment, so a developer # shell that exports them (as a real eval run must) would otherwise rewrite what # these tests expect -- e.g. GOODDATA_EVAL_CHAT_MAX_RETRIES=1 turns the expected diff --git a/packages/gooddata-eval/tests/test_from_insights.py b/packages/gooddata-eval/tests/test_from_insights.py index bf343547c..5ba4176b2 100644 --- a/packages/gooddata-eval/tests/test_from_insights.py +++ b/packages/gooddata-eval/tests/test_from_insights.py @@ -4,17 +4,24 @@ import pytest from gooddata_eval.core.dataset.from_insights import ( Unsupported, + _rules_for, _validation_errors, build, build_display_names, contradictions, convert, + derive, + derived_candidates, + derived_n, describe, display_name, + element_counts, insight_ids_on, langfuse_payload, list_ids, mint_id, + pick_derived, + rankable, resolve_type, ) from gooddata_eval.core.dataset.local import load_local_dataset @@ -387,6 +394,23 @@ def test_filters_are_briefed_in_words_not_json(): assert "{" not in brief +@pytest.mark.parametrize( + "title", + [ + "Products by Most Items Sold", + # "Least"/"Worst"/"Largest" name a ranking as plainly as "Most" does. Missing any + # of them lets a mis-specified insight through as a base, and `--enrich-ranked` + # then derives a *top* N from a chart whose own title says the opposite. + "Products by Least Items Sold", + "Worst Performing Merchants", + "Largest Accounts", + ], +) +def test_degenerate_ranking_titles_are_skipped(title): + with pytest.raises(Unsupported, match="promises a ranking"): + convert(spend_by_merchant(title=title), DATE_IDS) + + def test_degenerate_titles_are_skipped_rather_than_contradicted(): with pytest.raises(Unsupported, match="promises a ranking"): convert(spend_by_merchant(title="Products by Most Items Sold"), DATE_IDS) @@ -626,3 +650,220 @@ def test_uri_form_attribute_filter_is_still_skipped(): ), DATE_IDS, ) + + +# --- derived ranking variants ------------------------------------------------- + + +def _bar(metric_id, label_id, **kw): + return viz( + "local:bar", + [ + {"localIdentifier": "measures", "items": [measure("m", metric_id)]}, + {"localIdentifier": "view", "items": [attribute("a", label_id)]}, + ], + **kw, + ) + + +def test_a_plain_single_metric_breakdown_is_rankable(): + spec = convert(_bar("spend", "merchant.NAME"), DATE_IDS) + assert rankable(spec, DATE_IDS) == "d_merchant_name" + + +@pytest.mark.parametrize( + "content,reason", + [ + ( + viz( + "local:bar", + [ + { + "localIdentifier": "measures", + "items": [measure("m", "spend"), measure("m2", "gross_revenue")], + }, + {"localIdentifier": "view", "items": [attribute("a", "merchant.NAME")]}, + ], + ), + "two metrics leave 'top 3 by what?' unanswered", + ), + ( + viz( + "local:bar", + [ + {"localIdentifier": "measures", "items": [measure("m", "spend")]}, + {"localIdentifier": "view", "items": [attribute("a", "merchant.NAME")]}, + {"localIdentifier": "segment", "items": [attribute("b", "region.NAME")]}, + ], + ), + "a segment makes the N ambiguous between the pair and within a group", + ), + ( + viz( + "local:line", + [ + {"localIdentifier": "measures", "items": [measure("m", "spend")]}, + {"localIdentifier": "trend", "items": [attribute("a", "process_date.month")]}, + ], + ), + "'top 3 months' is not a question anyone asks", + ), + ( + _bar("spend", "merchant.NAME", sorts=[{"attributeSortItem": {"attributeIdentifier": "a"}}]), + "already sorts, so the shape is covered by the real insight", + ), + ], +) +def test_ineligible_bases_are_not_ranked(content, reason): + assert rankable(convert(content, DATE_IDS), DATE_IDS) is None, reason + + +def test_headline_without_a_dimension_is_not_rankable(): + spec = convert(viz("local:headline", [{"localIdentifier": "measures", "items": [measure("m", "spend")]}]), DATE_IDS) + assert rankable(spec, DATE_IDS) is None + + +@pytest.mark.parametrize("count,expected", [(None, 3), (2, None), (4, None), (5, 3), (6, 3), (7, 5), (50, 5)]) +def test_n_needs_headroom_over_the_element_count(count, expected): + assert derived_n(count) == expected + + +def test_ranking_variant_limits_the_rows_and_keeps_the_base_intact(): + base = convert(_bar("spend", "merchant.NAME"), DATE_IDS) + out = derive(base, "ranking_filter", 3, DATE_IDS) + + assert list(out["query"]["filter_by"].values()) == [{"type": "ranking_filter", "using": "m_spend", "top": 3}] + assert out["sort_by"] == [] + assert out["_derived_from"] == "v_x" + assert out["_derived_kind"] == "ranking_filter" + assert out["id"] != base["id"] + assert base["query"]["filter_by"] == {}, "the base spec must not be mutated" + + +def test_sort_variant_orders_without_limiting(): + out = derive(convert(_bar("spend", "merchant.NAME"), DATE_IDS), "sort_by", 3, DATE_IDS) + + assert out["sort_by"] == [{"field": "m_spend", "direction": "DESC"}] + assert out["query"]["filter_by"] == {}, "a sort must not silently limit the rows" + + +def test_derived_variants_are_scorable_and_valid(): + base = convert(_bar("spend", "merchant.NAME"), DATE_IDS) + for kind in ("ranking_filter", "sort_by"): + envelope = build(derive(base, kind, 3, DATE_IDS), "Show the top 3 Merchants by Spend", "d", set()) + assert _validation_errors(envelope) is None + CreatedVisualization.model_validate(envelope["expected_output"]["visualization"]) + + +def test_a_derived_item_records_where_it_came_from(): + base = convert(_bar("spend", "merchant.NAME"), DATE_IDS) + envelope = build(derive(base, "ranking_filter", 5, DATE_IDS), "Show the top 5 Merchants by Spend", "d", set()) + + assert envelope["derived_from"] == "v_x" + assert envelope["derived_kind"] == "ranking_filter" + assert "_derived_from" not in envelope["expected_output"]["visualization"], "provenance is not part of the spec" + + payload = langfuse_payload([envelope], "d", "ws", "origin") + assert payload["items"][0]["metadata"]["derived_from"] == "v_x" + + +def test_a_base_item_carries_no_provenance_keys(): + envelope = build(convert(_bar("spend", "merchant.NAME"), DATE_IDS), "Show Spend by Merchant", "d", set()) + assert "derived_from" not in envelope + assert "derived_kind" not in langfuse_payload([envelope], "d", "ws", "o")["items"][0]["metadata"] + + +def test_derived_ranking_reads_as_a_ranking_not_a_breakdown(): + base = convert(_bar("spend", "merchant.NAME"), DATE_IDS) + rules = _rules_for(derive(base, "ranking_filter", 3, DATE_IDS), DISPLAY) + assert "the top 3 Merchant Name" in rules + assert "exactly once" in rules + assert "- Say the question is broken down by" not in rules, "the ranking line replaces the breakdown line" + + +def test_a_ranking_within_an_attribute_still_asks_for_the_breakdown(): + # `ranked within ` ranks inside each group, so the breakdown is real and + # the question has to name it. + spec = convert(spend_by_merchant(), DATE_IDS) + spec["query"]["filter_by"]["f0"] = { + "type": "ranking_filter", + "using": "m_spend", + "attribute": "d_merchant_name", + "top": 3, + } + assert "- Say the question is broken down by" in _rules_for(spec, DISPLAY) + + +def test_a_derived_question_naming_the_ranking_is_not_a_contradiction(): + spec = derive(convert(_bar("spend", "merchant.NAME"), DATE_IDS), "ranking_filter", 3, DATE_IDS) + assert contradictions("Show me the top 3 Merchant Name values by Spend", spec, DISPLAY) == [] + + +def test_picks_spread_across_metrics_before_repeating_one(): + specs = [ + convert(_bar("spend", "merchant.NAME"), DATE_IDS), + convert(_bar("spend", "region.NAME"), DATE_IDS), + convert(_bar("gross_revenue", "merchant.NAME"), DATE_IDS), + ] + picked = pick_derived(specs, DATE_IDS, 2, counts={}) + + metrics = {p["metrics"][0] for p in picked} + assert len(metrics) == 2, "one popular metric must not take the whole budget" + + +def test_ranking_variants_are_exhausted_before_any_sort_variant(): + specs = [convert(_bar("spend", "merchant.NAME"), DATE_IDS), convert(_bar("gross_revenue", "region.NAME"), DATE_IDS)] + + assert [p["_derived_kind"] for p in pick_derived(specs, DATE_IDS, 2, counts={})] == [ + "ranking_filter", + "ranking_filter", + ] + kinds = [p["_derived_kind"] for p in pick_derived(specs, DATE_IDS, 4, counts={})] + assert sorted(kinds) == ["ranking_filter", "ranking_filter", "sort_by", "sort_by"] + + +def test_the_budget_is_a_hard_cap(): + specs = [convert(_bar("spend", f"d{i}.NAME"), DATE_IDS) for i in range(10)] + assert len(pick_derived(specs, DATE_IDS, 3, counts={})) == 3 + + +def test_a_low_cardinality_dimension_is_not_ranked(): + specs = [convert(_bar("spend", "merchant.NAME"), DATE_IDS)] + assert pick_derived(specs, DATE_IDS, 5, counts={"label/merchant.NAME": 3}) == [] + + +def test_n_follows_the_element_count(): + specs = [convert(_bar("spend", "merchant.NAME"), DATE_IDS)] + picked = pick_derived(specs, DATE_IDS, 1, counts={"label/merchant.NAME": 40}) + assert next(iter(picked[0]["query"]["filter_by"].values()))["top"] == 5 + + +def test_candidates_are_only_the_dimensions_a_derivation_would_need(): + specs = [ + convert(_bar("spend", "merchant.NAME"), DATE_IDS), + convert( + viz( + "local:line", + [ + {"localIdentifier": "measures", "items": [measure("m", "spend")]}, + {"localIdentifier": "trend", "items": [attribute("a", "process_date.month")]}, + ], + ), + DATE_IDS, + ), + ] + assert derived_candidates(specs, DATE_IDS) == {"label/merchant.NAME"} + + +def test_element_counts_stop_at_the_ceiling_and_survive_an_unservable_label(): + class _Content: + def get_label_elements(self, workspace_id, label_id, limit=None): + if label_id == "label/bad": + raise RuntimeError("no such label") + assert limit == 7, "counting further than the largest N plus headroom is wasted work" + return ["v"] * limit + + class _Sdk: + catalog_workspace_content = _Content() + + assert element_counts(_Sdk(), "ws", {"label/merchant.NAME", "label/bad"}) == {"label/merchant.NAME": 7} From ed50eeaa5efdd2fb74305b4da74200a858bd1271 Mon Sep 17 00:00:00 2001 From: Roman Valovic Date: Tue, 8 Sep 2026 16:10:34 +0200 Subject: [PATCH 10/15] feat(gooddata-eval): implement the ranking a chart's own title promised Nine of loop's 53 insights are titled for a ranking their definition never implements -- "Products by Most Items Sold", "Top Returned Reasons" -- because the analyst sorted in Analytical Designer and saved without the sort. They were skipped as mis-specified, which is right for a copied fixture and wasteful otherwise: the missing piece is written down in the title. `--enrich-ranked` now spends its budget best-grounded first. A title naming one end of a ranking (highest/most/largest vs lowest/least/worst) becomes a ranking filter in that direction, with the N from the title when it states one; a title naming both ends names neither and is still skipped. Then come ranking filters this generator adds to a plain breakdown, then sort-only variants. Every item records `derived_basis`, so a pass rate over "the human asked for this" and "we made it up" stays separable. Derived items are deduplicated by resolved definition. Loop has two pairs of differently-titled insights over one metric and dimension -- "Products by Most Items Sold" and "Products Driving the Highest Number of Repeat Purchases" -- which produced the same question twice, double-weighting one skill. On loop: 44 base items, 12 derived (6 rescued from titles, 6 from shape), against 6 before this change. Co-Authored-By: Claude Opus 5 --- .../src/gooddata_eval/cli/main.py | 9 +- .../core/dataset/from_insights.py | 205 ++++++++++++++++-- .../gooddata-eval/tests/test_from_insights.py | 170 +++++++++++++++ 3 files changed, 358 insertions(+), 26 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/cli/main.py b/packages/gooddata-eval/src/gooddata_eval/cli/main.py index 456928945..e00b94549 100644 --- a/packages/gooddata-eval/src/gooddata_eval/cli/main.py +++ b/packages/gooddata-eval/src/gooddata_eval/cli/main.py @@ -230,9 +230,12 @@ def _build_parser() -> argparse.ArgumentParser: type=int, default=0, metavar="N", - help="Additionally derive up to N ranked questions from eligible base insights by adding a " - "ranking filter ('the top 3 X by Y') or a descending sort. Use when the workspace has no " - "ranked insights of its own. Derived items carry `derived_from`. Default: 0 (off).", + help="Additionally derive up to N ranked questions. Best-grounded first: insights whose " + "own title promised a ranking their definition never implemented ('Top Returned Reasons' " + "saved with no sort) are implemented as the title asks, then ranking filters this " + "generator adds to a plain breakdown, then sort-only variants. Use when the workspace has " + "no ranked insights of its own. Derived items carry `derived_from` and `derived_basis`. " + "Default: 0 (off).", ) gen.add_argument("--dry-run", dest="dry_run", action="store_true", help="Report only; write nothing.") models_cmd = sub.add_parser("models", help="List LLM providers and models configured in the org.") diff --git a/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py b/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py index a32855c47..329886951 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py @@ -123,6 +123,13 @@ re.I, ) +# Which end of the ranking a title names. A title using both ("Top and Bottom Products") +# names no single direction and is left alone. +TOP_WORDS = re.compile(r"\b(top|most|highest|largest|greatest|best)\b", re.I) +BOTTOM_WORDS = re.compile(r"\b(bottom|least|fewest|lowest|smallest|worst)\b", re.I) +# "Top 10 Products" states the N outright; most titles do not. +TITLE_N = re.compile(r"\b(?:top|bottom|first|last)\s+(\d{1,3})\b", re.I) + # A slot the writer failed to fill: it copied the instruction instead of a real name. PLACEHOLDER = re.compile(r"\b(breakdown|split|filter)\s+dimension\b|[{}<>]") # The text a question breaks down by. `(? str: ascii_text = unicodedata.normalize("NFKD", text).encode("ascii", "ignore").decode("ascii").lower() slug = re.sub(r"[^a-z0-9]+", "-", ascii_text).strip("-") @@ -386,11 +408,24 @@ def _reject_degenerate(spec: dict) -> None: """ title = spec["title"] or "" if RANK_WORDS.search(title) and not ranks(spec): - raise Unsupported(f"title '{title}' promises a ranking the definition has no sort/ranking filter for") + message = f"title '{title}' promises a ranking the definition has no sort/ranking filter for" + direction = title_direction(title) + if direction is None: + raise Unsupported(message) + found = TITLE_N.search(title) + raise PromisedRanking(message, spec, direction, int(found.group(1)) if found else None) if FILTER_WORDS.search(title) and not filters(spec): raise Unsupported(f"title '{title}' promises a filter the definition has no date/attribute filter for") +def title_direction(title: str) -> str | None: + """Which end of the ranking `title` names, or None if it names both or neither.""" + top, bottom = bool(TOP_WORDS.search(title)), bool(BOTTOM_WORDS.search(title)) + if top == bottom: + return None + return "top" if top else "bottom" + + def classify(spec: dict, date_instance_ids: set) -> str: """Question shape, for the coverage report. @@ -463,28 +498,41 @@ def derived_n(element_count: int | None) -> int | None: return None -def derive(spec: dict, kind: str, n: int, date_instance_ids: set) -> dict: - """A copy of `spec` with a ranking filter or a descending sort added. +def derive( + spec: dict, + kind: str, + n: int, + date_instance_ids: set, + direction: str = "top", + basis: str = "shape", +) -> dict: + """A copy of `spec` with a ranking filter or a sort added. The two kinds are never combined in one item: "the top 3" limits the rows and "sorted by" only orders them, they score on different checks, and an item asserting both is an item you cannot diagnose from its result. + + `basis` records who wanted the ranking -- "title" when a human's own chart title + asked for it, "shape" when this generator chose to add one. """ if kind not in DERIVED_KINDS: raise ValueError(f"unknown derived kind '{kind}'") + if direction not in ("top", "bottom"): + raise ValueError(f"unknown ranking direction '{direction}'") out = json.loads(json.dumps(spec)) metric = out["metrics"][0] if kind == "ranking_filter": key = f"f{len(out['query']['filter_by'])}" - out["query"]["filter_by"][key] = {"type": "ranking_filter", "using": metric, "top": n} - out["id"] = f"{out['id']}_top{n}"[:30] - out["title"] = f"{spec['title']} (top {n})" + out["query"]["filter_by"][key] = {"type": "ranking_filter", "using": metric, direction: n} + out["id"] = f"{out['id']}_{direction}{n}"[:30] + out["title"] = f"{spec['title']} ({direction} {n})" else: - out["sort_by"] = [{"field": metric, "direction": "DESC"}] + out["sort_by"] = [{"field": metric, "direction": "DESC" if direction == "top" else "ASC"}] out["id"] = f"{out['id']}_sorted"[:30] out["title"] = f"{spec['title']} (sorted)" out["_derived_from"] = spec["id"] out["_derived_kind"] = kind + out["_derived_basis"] = basis out["_shape"] = classify(out, date_instance_ids) return out @@ -508,15 +556,96 @@ def count(uri: str) -> int | None: return {uri: n for uri in sorted(label_uris) if (n := count(uri)) is not None} -def pick_derived(specs: list, date_instance_ids: set, limit: int, counts: dict | None = None) -> list: - """Up to `limit` derived variants, spread across distinct metrics. +def rescued(promised: list, date_instance_ids: set, counts: dict | None = None) -> list: + """Ranked items for insights whose titles promised a ranking they never implemented. - Expanding every eligible insight would turn one popular metric into a third of the - corpus, and the pass rate into a measurement of one skill. Bases are taken - round-robin by metric, and ranking-filter variants are exhausted before any sort-only - variant is added, so a small `limit` yields the shape the corpus is missing most. + Higher confidence than anything derived from shape alone: the direction comes from + the human's own words, and often the N does too. A title's explicit N is honoured + even when it differs from what the cardinality would have chosen, but a title asking + for a top 10 of seven values still yields nothing -- the words do not make the data + deeper. """ counts = counts or {} + out = [] + for error in promised: + spec = error.spec + alias = rankable(spec, date_instance_ids) + if alias is None: + continue + count = counts.get(field_uri(spec["query"]["fields"], alias)) + if error.n is None: + n = derived_n(count) + elif count is None or count >= error.n + _ELEMENT_HEADROOM: + n = error.n + else: + n = None + if n is None: + continue + out.append(derive(spec, "ranking_filter", n, date_instance_ids, error.direction, basis="title")) + return out + + +def spec_signature(spec: dict) -> str: + """What the item actually asks for, as a comparable string. + + Two differently-titled insights can carry the same definition -- loop has both + "Products by Most Items Sold" and "Products Driving the Highest Number of Repeat + Purchases" over Units Sold by Product Title -- and deriving from each produces the + same question twice. Identity is the resolved fields, filters and sorts; titles and + ids are not part of it. + """ + fields = spec["query"]["fields"] + + def resolve(value): + if isinstance(value, str): + return field_uri(fields, value) + if isinstance(value, dict): + return {k: resolve(v) for k, v in sorted(value.items())} + if isinstance(value, list): + return [resolve(v) for v in value] + return value + + return json.dumps( + { + "metrics": sorted(resolve(a) for a in spec["metrics"]), + "dims": sorted(resolve(a) for a in spec["view_by"] + spec["segment_by"] + spec["columns"] + spec["rows"]), + "filters": sorted(json.dumps(resolve(f), sort_keys=True) for f in spec["query"]["filter_by"].values()), + "sorts": [resolve(entry) for entry in spec["sort_by"]], + }, + sort_keys=True, + ) + + +def pick_derived( + specs: list, + date_instance_ids: set, + limit: int, + counts: dict | None = None, + promised: list | None = None, +) -> list: + """Up to `limit` derived variants, best-grounded first. + + Order matters because the budget is small: rescued items (a human titled the chart + "Top Returned Reasons") come before ranking filters this generator invented, which + come before sort-only variants. Within the invented ones, expanding every eligible + insight would turn one popular metric into a third of the corpus and the pass rate + into a measurement of one skill, so bases are taken round-robin by metric. + """ + counts = counts or {} + seen, out = set(), [] + + def take(spec: dict) -> bool: + """Keep `spec` unless an item already asks the same thing. Reports the budget.""" + signature = spec_signature(spec) + if signature not in seen: + seen.add(signature) + out.append(spec) + return len(out) < limit + + for spec in rescued(promised or [], date_instance_ids, counts): + if not take(spec): + break + by_metric: dict[str, list] = {} for spec in specs: alias = rankable(spec, date_instance_ids) @@ -528,7 +657,6 @@ def pick_derived(specs: list, date_instance_ids: set, limit: int, counts: dict | continue by_metric.setdefault(field_uri(fields, spec["metrics"][0]), []).append((spec, n)) - out = [] for kind in DERIVED_KINDS: queues = [list(group) for group in by_metric.values()] while queues and len(out) < limit: @@ -536,17 +664,16 @@ def pick_derived(specs: list, date_instance_ids: set, limit: int, counts: dict | if not queue: continue spec, n = queue.pop(0) - out.append(derive(spec, kind, n, date_instance_ids)) - if len(out) >= limit: - break + if not take(derive(spec, kind, n, date_instance_ids)): + return out queues = [q for q in queues if q] return out -def derived_candidates(specs: list, date_instance_ids: set) -> set: +def derived_candidates(specs: list, date_instance_ids: set, promised: list | None = None) -> set: """Label uris whose element count decides whether a base can be derived from.""" uris = set() - for spec in specs: + for spec in list(specs) + [e.spec for e in promised or []]: alias = rankable(spec, date_instance_ids) if alias is not None: uris.add(field_uri(spec["query"]["fields"], alias)) @@ -858,6 +985,7 @@ def resolve_type(spec: dict, question: str) -> str: def build(spec: dict, question: str, dataset_name: str, existing_ids: set) -> dict: derived_from, derived_kind = spec.get("_derived_from"), spec.get("_derived_kind") + derived_basis = spec.get("_derived_basis") spec = {k: v for k, v in spec.items() if not k.startswith("_")} spec["type"] = resolve_type(spec, question) question_id = mint_id(question, existing_ids) @@ -875,6 +1003,7 @@ def build(spec: dict, question: str, dataset_name: str, existing_ids: set) -> di # and the pass rate has to be computable both ways. envelope["derived_from"] = derived_from envelope["derived_kind"] = derived_kind + envelope["derived_basis"] = derived_basis return envelope @@ -898,7 +1027,11 @@ def langfuse_payload(envelopes: list, dataset: str, workspace_id: str, origin: s "workspace": workspace_id, "origin": origin, **( - {"derived_from": e["derived_from"], "derived_kind": e["derived_kind"]} + { + "derived_from": e["derived_from"], + "derived_kind": e["derived_kind"], + "derived_basis": e["derived_basis"], + } if e.get("derived_from") else {} ), @@ -942,7 +1075,7 @@ def generate(args, sdk_factory=None) -> int: return 1 visualizations = [v for v in visualizations if v.get("id") in keep] - specs, skipped = [], [] + specs, skipped, promised = [], [], [] for viz in visualizations: if viz.get("isHidden"): # Hidden objects are invisible to the AI assistant's catalog search, so a @@ -951,6 +1084,12 @@ def generate(args, sdk_factory=None) -> int: continue try: specs.append(convert(viz, date_instance_ids, display_names)) + except PromisedRanking as exc: + # Unusable as a copy, but the title says what the definition forgot. Kept + # aside for `--enrich-ranked`; still a skip when enrichment is off. + promised.append(exc) + if not args.enrich_ranked: + skipped.append((viz.get("id"), str(exc))) except Unsupported as exc: skipped.append((viz.get("id"), str(exc))) @@ -958,7 +1097,7 @@ def generate(args, sdk_factory=None) -> int: derived = [] if args.enrich_ranked: counts = dict(snapshot.get("label_cardinality") or {}) - wanted = derived_candidates(specs, date_instance_ids) + wanted = derived_candidates(specs, date_instance_ids, promised) missing = wanted - set(counts) if sdk is not None and missing: counts.update(element_counts(sdk, snapshot["workspace_id"], missing)) @@ -971,8 +1110,26 @@ def generate(args, sdk_factory=None) -> int: f"deriving with the smallest N", file=sys.stderr, ) - derived = pick_derived(specs, date_instance_ids, args.enrich_ranked, counts) + derived = pick_derived(specs, date_instance_ids, args.enrich_ranked, counts, promised) specs = specs + derived + rescued_ids = {spec["_derived_from"] for spec in derived if spec["_derived_basis"] == "title"} + # A promised ranking that did not make it was either ineligible or a duplicate of + # something already derived. Reporting the original "promises a ranking" message + # for a duplicate would send the reader looking for a problem in the wrong place. + taken = {spec_signature(spec) for spec in derived} + duplicates = { + spec["_derived_from"] + for spec in rescued(promised, date_instance_ids, counts) + if spec["_derived_from"] not in rescued_ids and spec_signature(spec) in taken + } + skipped.extend( + ( + e.spec["id"], + "definition duplicates an item already derived" if e.spec["id"] in duplicates else str(e), + ) + for e in promised + if e.spec["id"] not in rescued_ids + ) shapes: dict[str, list] = {} for spec in specs: @@ -994,6 +1151,8 @@ def generate(args, sdk_factory=None) -> int: by_kind[spec["_derived_kind"]] = by_kind.get(spec["_derived_kind"], 0) + 1 summary = ", ".join(f"{n} {kind}" for kind, n in by_kind.items()) or "none eligible" print(f" derived from a base {len(derived)} ({summary}), {n_base} from real insights") + n_rescued = sum(1 for spec in derived if spec["_derived_basis"] == "title") + print(f" of those, title-asked {n_rescued} of {len(promised)} insight(s) that promised a ranking") for viz_id, reason in skipped: print(f" SKIP {viz_id}: {reason}") diff --git a/packages/gooddata-eval/tests/test_from_insights.py b/packages/gooddata-eval/tests/test_from_insights.py index 5ba4176b2..1ab416dd9 100644 --- a/packages/gooddata-eval/tests/test_from_insights.py +++ b/packages/gooddata-eval/tests/test_from_insights.py @@ -3,6 +3,7 @@ import pytest from gooddata_eval.core.dataset.from_insights import ( + PromisedRanking, Unsupported, _rules_for, _validation_errors, @@ -22,7 +23,10 @@ mint_id, pick_derived, rankable, + rescued, resolve_type, + spec_signature, + title_direction, ) from gooddata_eval.core.dataset.local import load_local_dataset from gooddata_eval.core.models import CreatedVisualization @@ -867,3 +871,169 @@ class _Sdk: catalog_workspace_content = _Content() assert element_counts(_Sdk(), "ws", {"label/merchant.NAME", "label/bad"}) == {"label/merchant.NAME": 7} + + +# --- rescuing insights whose titles promised a ranking ------------------------ + + +@pytest.mark.parametrize( + "title,direction", + [ + ("Top Returned Reasons", "top"), + ("Products With the Highest Return Rate", "top"), + ("Products by Most Items Sold", "top"), + ("Largest Accounts", "top"), + ("Products With the Lowest Return Rate", "bottom"), + ("Products by Least Items Sold", "bottom"), + ("Worst Performing Merchants", "bottom"), + # Names both ends, so it names neither: implementing one would be a coin flip. + ("Top and Bottom Products", None), + ("Spend by Merchant", None), + ], +) +def test_title_direction(title, direction): + assert title_direction(title) == direction + + +def test_a_promised_ranking_carries_the_spec_and_the_intent(): + with pytest.raises(PromisedRanking) as caught: + convert(spend_by_merchant(title="Top 10 Merchants by Spend"), DATE_IDS) + + assert caught.value.direction == "top" + assert caught.value.n == 10 + assert caught.value.spec["metrics"] == ["m_spend"] + assert isinstance(caught.value, Unsupported), "still unusable as a copied fixture" + + +def test_a_promised_ranking_without_a_number_leaves_n_open(): + with pytest.raises(PromisedRanking) as caught: + convert(spend_by_merchant(title="Top Merchants"), DATE_IDS) + assert caught.value.n is None + + +def test_an_ambiguous_ranking_title_is_a_plain_skip(): + with pytest.raises(Unsupported) as caught: + convert(spend_by_merchant(title="Top and Bottom Merchants"), DATE_IDS) + assert not isinstance(caught.value, PromisedRanking) + + +def _promised(title): + with pytest.raises(PromisedRanking) as caught: + convert(spend_by_merchant(title=title), DATE_IDS) + return caught.value + + +def test_a_lowest_title_is_implemented_as_a_bottom_n(): + items = rescued([_promised("Merchants With the Lowest Spend")], DATE_IDS, {"label/merchant.NAME": 40}) + + ranking = next(iter(items[0]["query"]["filter_by"].values())) + assert ranking == {"type": "ranking_filter", "using": "m_spend", "bottom": 5} + assert items[0]["_derived_basis"] == "title", "the human's title asked for this, not the generator" + + +def test_an_explicit_n_in_the_title_wins_over_the_default(): + items = rescued([_promised("Top 10 Merchants by Spend")], DATE_IDS, {"label/merchant.NAME": 40}) + assert next(iter(items[0]["query"]["filter_by"].values()))["top"] == 10 + + +def test_a_title_asking_for_more_rows_than_exist_is_not_rescued(): + assert rescued([_promised("Top 10 Merchants by Spend")], DATE_IDS, {"label/merchant.NAME": 7}) == [] + + +def test_a_promised_ranking_on_an_unrankable_shape_is_not_rescued(): + error = PromisedRanking( + "promises a ranking", + convert( + viz( + "local:line", + [ + {"localIdentifier": "measures", "items": [measure("m", "spend")]}, + {"localIdentifier": "trend", "items": [attribute("a", "process_date.month")]}, + ], + ), + DATE_IDS, + ), + "top", + 5, + ) + assert rescued([error], DATE_IDS, {}) == [] + + +def test_rescued_items_are_spent_before_anything_the_generator_invents(): + # A base unrelated to the rescued one, so ordering is what is under test here and + # not the dedup that would otherwise collapse two identical rankings. + specs = [convert(_bar("revenue", "region.NAME"), DATE_IDS)] + promised = [_promised("Top Merchants by Spend")] + + picked = pick_derived(specs, DATE_IDS, 1, counts={}, promised=promised) + assert [p["_derived_basis"] for p in picked] == ["title"] + + picked = pick_derived(specs, DATE_IDS, 3, counts={}, promised=promised) + assert [p["_derived_basis"] for p in picked] == ["title", "shape", "shape"] + + +def test_element_counts_cover_the_rescue_candidates_too(): + promised = [_promised("Top Merchants by Spend")] + assert derived_candidates([], DATE_IDS, promised) == {"label/merchant.NAME"} + + +def test_a_bottom_sort_is_ascending(): + out = derive(convert(_bar("spend", "merchant.NAME"), DATE_IDS), "sort_by", 3, DATE_IDS, direction="bottom") + assert out["sort_by"] == [{"field": "m_spend", "direction": "ASC"}] + + +def test_an_unknown_direction_is_a_programming_error(): + base = convert(_bar("spend", "merchant.NAME"), DATE_IDS) + with pytest.raises(ValueError, match="direction"): + derive(base, "ranking_filter", 3, DATE_IDS, direction="middle") + + +def test_a_rescued_item_is_scorable_and_says_the_title_asked_for_it(): + items = rescued([_promised("Top 5 Merchants by Spend")], DATE_IDS, {"label/merchant.NAME": 40}) + envelope = build(items[0], "What are the top 5 Merchants by Spend?", "d", set()) + + assert _validation_errors(envelope) is None + assert envelope["derived_basis"] == "title" + assert langfuse_payload([envelope], "d", "ws", "o")["items"][0]["metadata"]["derived_basis"] == "title" + + +def test_a_bottom_ranking_is_briefed_as_bottom(): + items = rescued([_promised("Merchants With the Lowest Spend")], DATE_IDS, {"label/merchant.NAME": 40}) + assert "bottom 5 by Spend Amount" in describe(items[0], DISPLAY) + assert "the bottom 5 Merchant Name" in _rules_for(items[0], DISPLAY) + + +def test_two_insights_with_one_definition_do_not_become_two_items(): + # loop has "Products by Most Items Sold" and "Products Driving the Highest Number of + # Repeat Purchases" over the same metric and dimension. Both promise a ranking, and + # deriving from each produced the identical question twice. + promised = [_promised("Products by Most Items Sold"), _promised("Products With the Highest Spend")] + picked = pick_derived([], DATE_IDS, 5, counts={"label/merchant.NAME": 40}, promised=promised) + + assert len(picked) == 1 + assert len({spec_signature(spec) for spec in picked}) == 1 + + +def test_dedup_compares_what_is_asked_not_how_it_is_titled(): + base = convert(_bar("spend", "merchant.NAME"), DATE_IDS) + same = derive(base, "ranking_filter", 5, DATE_IDS) + renamed = derive({**base, "title": "Something Else", "id": "v_other"}, "ranking_filter", 5, DATE_IDS) + assert spec_signature(same) == spec_signature(renamed) + + other_n = derive(base, "ranking_filter", 3, DATE_IDS) + other_end = derive(base, "ranking_filter", 5, DATE_IDS, direction="bottom") + assert len({spec_signature(s) for s in (same, other_n, other_end)}) == 3 + + +def test_a_rescue_and_an_invented_ranking_that_agree_yield_one_item(): + # `pick_derived` spends rescues first, so the surviving item is the grounded one. + base = convert(_bar("spend", "merchant.NAME"), DATE_IDS) + picked = pick_derived( + [base], + DATE_IDS, + 5, + counts={"label/merchant.NAME": 40}, + promised=[_promised("Top 5 Merchants by Spend")], + ) + ranked = [spec for spec in picked if spec["_derived_kind"] == "ranking_filter"] + assert [spec["_derived_basis"] for spec in ranked] == ["title"] From 641e11895740e3b5dc20e574059ba8a2a1ca06f1 Mon Sep 17 00:00:00 2001 From: Roman Valovic Date: Thu, 10 Sep 2026 16:03:30 +0200 Subject: [PATCH 11/15] fix(gooddata-eval): make a question ask for what its spec contains Two item-quality defects the first reference-model run exposed, both of which punished the agent for reading the question correctly. Seven headline items failed with no visualization created at all. The reasoning shows why: asked "What is the Upsell Ratio?", the agent reads a request for a definition and answers in prose. Rephrasing it as "Show me the Upsell Ratio" fared no better -- the tool trace has it activating only its search skill, answering with the metric it found and building nothing. Naming the form is what makes a bare metric a charting request, so the no-breakdown rule now requires it ("as a KPI", "as a single number") and the standing "do not name the chart type" rule is suppressed for exactly that case, since together they gave the writer contradictory instructions. Naming the form also lets `resolve_type` score the expected `headline`, which is what the insight was. The one derived failure worth reading was not a defect in the sort variant: the workspace carries six labels all titled "Product Title", so a question naming one of them cannot say which is meant. The model built a perfect chart over `product_title_at_time_of_return` and scored zero against `product_details.LINE_ITEM_TITLE`. `ambiguous_fields` now reports every item whose metric or dimension name matches more than one object in the model -- 20 of that workspace's 56 -- and `--skip-ambiguous` drops them. Reported either way, because silently shipping an unwinnable item is worse than a smaller dataset. Probed on three of the failures before regenerating the corpus: 3/3 pass, where all three built nothing before. Co-Authored-By: Claude Opus 5 --- .../src/gooddata_eval/cli/main.py | 8 +++ .../core/dataset/from_insights.py | 60 +++++++++++++++++-- .../gooddata-eval/tests/test_from_insights.py | 47 +++++++++++++++ 3 files changed, 111 insertions(+), 4 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/cli/main.py b/packages/gooddata-eval/src/gooddata_eval/cli/main.py index e00b94549..3b4ee0a4b 100644 --- a/packages/gooddata-eval/src/gooddata_eval/cli/main.py +++ b/packages/gooddata-eval/src/gooddata_eval/cli/main.py @@ -237,6 +237,14 @@ def _build_parser() -> argparse.ArgumentParser: "no ranked insights of its own. Derived items carry `derived_from` and `derived_basis`. " "Default: 0 (off).", ) + gen.add_argument( + "--skip-ambiguous", + dest="skip_ambiguous", + action="store_true", + help="Drop items whose metric or dimension name matches more than one object in the model " + "(loop has six labels titled 'Product Title'). Such a question cannot say which object it " + "means, so a defensible answer still scores zero. Reported either way.", + ) gen.add_argument("--dry-run", dest="dry_run", action="store_true", help="Report only; write nothing.") models_cmd = sub.add_parser("models", help="List LLM providers and models configured in the org.") models_cmd.add_argument("--host", help="GoodData host URL.") diff --git a/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py b/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py index 329886951..e7bd60466 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py @@ -819,6 +819,30 @@ def name(alias: str) -> str: return "\n".join(lines) +def ambiguous_titles(display_names: dict) -> set: + """Display titles that more than one object in the model carries. + + Loop has six labels all titled "Product Title". A question naming one of them cannot + say which is meant, so the expected dimension is unguessable and the item punishes a + defensible answer -- `label/product_title_at_time_of_return` instead of + `label/product_details.LINE_ITEM_TITLE` scored zero on an otherwise perfect chart. + """ + seen, dupes = {}, set() + for uri, title in display_names.items(): + key = _normalize(title) + if key in seen and seen[key] != uri: + dupes.add(key) + seen.setdefault(key, uri) + return dupes + + +def ambiguous_fields(spec: dict, display_names: dict, dupes: set | None = None) -> list: + """The display names in `spec` that do not identify one object in the model.""" + dupes = ambiguous_titles(display_names) if dupes is None else dupes + names = _metric_names(spec, display_names) + _dim_names(spec, display_names) + return sorted({name for name in names if _normalize(name) in dupes}) + + def field_uri(fields: dict, alias: str) -> str: """Resolve a bucket alias to its URI. @@ -910,11 +934,18 @@ def _rules_for(spec: dict, display_names: dict) -> str: elif dims: lines.append("- Say the question is broken down by " + ", ".join(dims) + ", naming each verbatim.") else: - lines.append( + lines += [ "- This chart has NO breakdown. Ask for the metric on its own -- do not write " "'by ...', 'broken down by ...' or 'grouped by ...' at all. Never break a metric " - "down by itself." - ) + "down by itself.", + # "What is the Upsell Ratio?" is answered as a definition, and "Show me Gross + # Revenue" is answered by looking the metric up -- the agent activates only its + # search skill and builds nothing. Naming the chart form is what makes a bare + # metric a charting request, so a no-breakdown question has to name it. + "- Ask for it AS A CHART, naming the form: 'as a single number', 'as a KPI' or " + "'as a headline'. Never a bare 'What is ?' (answered as a definition) and " + "never a bare 'Show me ' (answered by looking the metric up).", + ] if segments: lines.append("- Say it is split by " + ", ".join(segments) + ".") lines += [ @@ -924,7 +955,9 @@ def _rules_for(spec: dict, display_names: dict) -> str: "time period or a subset of values.", "- Write real names only. Never emit a literal word like 'breakdown dimension', 'metric' " "or 'dimension' as a stand-in for a name.", - "- Do not name the chart type; the assistant should infer it.", + # A single-metric chart is the exception: without a named form the request is + # indistinguishable from a metric lookup, so there the form is the question. + *([] if not dims else ["- Do not name the chart type; the assistant should infer it."]), "- Sound like a person asking a colleague, not like a chart title.", "- One sentence.", ] @@ -1135,6 +1168,13 @@ def generate(args, sdk_factory=None) -> int: for spec in specs: shapes.setdefault(spec["_shape"], []).append(spec["title"]) + dupes = ambiguous_titles(display_names) + ambiguous = [(spec, names) for spec in specs if (names := ambiguous_fields(spec, display_names, dupes))] + if args.skip_ambiguous: + drop = {id(spec) for spec, _ in ambiguous} + specs = [spec for spec in specs if id(spec) not in drop] + derived = [spec for spec in derived if id(spec) not in drop] + n_filtered = sum(1 for s in specs if filters(s)) n_ranked = sum(1 for s in specs if ranks(s)) print( @@ -1153,6 +1193,18 @@ def generate(args, sdk_factory=None) -> int: print(f" derived from a base {len(derived)} ({summary}), {n_base} from real insights") n_rescued = sum(1 for spec in derived if spec["_derived_basis"] == "title") print(f" of those, title-asked {n_rescued} of {len(promised)} insight(s) that promised a ranking") + if ambiguous: + verb = "dropped" if args.skip_ambiguous else "kept" + print( + f" ambiguous field names {len(ambiguous)} item(s) {verb}: a name below matches " + f"more than one object in the model, so the question cannot say which is meant" + ) + for spec, names in ambiguous[:5]: + print(f" {spec['title'][:40]:<40} {', '.join(names)}") + if len(ambiguous) > 5: + print(f" ... and {len(ambiguous) - 5} more") + if not args.skip_ambiguous: + print(" pass --skip-ambiguous to exclude them", file=sys.stderr) for viz_id, reason in skipped: print(f" SKIP {viz_id}: {reason}") diff --git a/packages/gooddata-eval/tests/test_from_insights.py b/packages/gooddata-eval/tests/test_from_insights.py index 1ab416dd9..52d22a2dc 100644 --- a/packages/gooddata-eval/tests/test_from_insights.py +++ b/packages/gooddata-eval/tests/test_from_insights.py @@ -7,6 +7,8 @@ Unsupported, _rules_for, _validation_errors, + ambiguous_fields, + ambiguous_titles, build, build_display_names, contradictions, @@ -1037,3 +1039,48 @@ def test_a_rescue_and_an_invented_ranking_that_agree_yield_one_item(): ) ranked = [spec for spec in picked if spec["_derived_kind"] == "ranking_filter"] assert [spec["_derived_basis"] for spec in ranked] == ["title"] + + +# --- items that cannot name what they mean ------------------------------------ + + +def test_a_question_asking_for_one_number_must_ask_to_see_it(): + # A bare "What is the Upsell Ratio?" reads as a request for a definition, and the + # agent answers in prose: seven of loop's headline items failed with no chart built. + spec = convert(viz("local:headline", [{"localIdentifier": "measures", "items": [measure("m", "spend")]}]), DATE_IDS) + rules = _rules_for(spec, DISPLAY) + assert "AS A CHART" in rules + assert "as a single number" in rules + assert "Never a bare 'What is ?'" in rules + assert "Do not name the chart type" not in rules, "a single number needs its form named" + + +def test_a_broken_down_question_is_not_told_to_name_a_chart_form(): + assert "AS A CHART" not in _rules_for(convert(spend_by_merchant(), DATE_IDS), DISPLAY) + + +def test_titles_carried_by_more_than_one_object_are_ambiguous(): + names = { + "label/product_details.LINE_ITEM_TITLE": "Product Title", + "label/EXT__RETURNED_ITEMS.PRODUCT_TITLE": "Product Title", + "label/merchant.NAME": "Merchant Name", + "metric/spend": "Spend Amount", + } + assert ambiguous_titles(names) == {"product title"} + + +def test_the_same_object_listed_twice_is_not_ambiguous(): + assert ambiguous_titles({"label/a": "Product Title"}) == set() + + +def test_an_item_naming_an_ambiguous_dimension_is_reported(): + spec = convert(_bar("spend", "merchant.NAME"), DATE_IDS) + names = {**DISPLAY, "label/other_dataset.NAME": "Merchant Name"} + assert ambiguous_fields(spec, names) == ["Merchant Name"] + assert ambiguous_fields(spec, DISPLAY) == [] + + +def test_an_ambiguous_metric_name_counts_too(): + spec = convert(_bar("spend", "merchant.NAME"), DATE_IDS) + names = {**DISPLAY, "metric/spend_v2": "Spend Amount"} + assert ambiguous_fields(spec, names) == ["Spend Amount"] From 4339ef953a0a6928fedbbc986e22dd7fef7ae33b Mon Sep 17 00:00:00 2001 From: Roman Valovic Date: Thu, 10 Sep 2026 16:03:44 +0200 Subject: [PATCH 12/15] feat(gooddata-eval): describe date granularities, don't quote their ids Date granularities are a closed platform enum, the same in every workspace, so unlike metric and label names they can be handled once rather than tuned per model. Four defects, all general. `build_display_names` covered seven of the seventeen enum members and keyed them snake_case, while the API's label ids are camelCase. A lookup for a real `monthOfYear` label therefore missed the map entirely and fell back to a de-slugged id -- "Order Created At - Monthofyear" -- and the snake spelling was no better at "Month_Of_Year". Any workspace charting day-of-week or month-of-year put that in its question text. All members are now present and registered under every spelling. A granularity has a cyclical twin -- MONTH walks consecutive calendar months, MONTH_OF_YEAR stacks every January together -- and "by Order Created At - Month" chooses neither, so the agent picked `monthOfYear` twice and lost two otherwise perfect charts. Date dimensions are now briefed by what they do ("one point per calendar month over time, not month-of-year") and the writer is told to say it in natural words while keeping the date dataset's name. Scoring compared date refs as raw strings, so a chart built on `attribute/ORDER_CREATED_AT.month` failed against the insight's `label/ORDER_CREATED_AT.month`. A date dataset exposes each granularity as an attribute whose only label carries the same id, so both denote one breakdown: `canonical_date_uri` folds the prefix and the spelling. The table moved to `core/granularity.py`, which generation and scoring now share. Registering each granularity under several spellings then made one label look like several objects, so every date dimension was reported as an ambiguous name; ambiguity detection compares canonical uris instead. And "Most Recent Label Created At" is a dataset's name, so a question naming it verbatim -- as the rules require -- was dropped for "using ranking word 'Most'": the claim checks now read around the spec's own field names, matching each display name and each side of its " - " separator. Date items with the reference model: 6/6, quality 100% (4/6 before). Co-Authored-By: Claude Opus 5 --- packages/gooddata-eval/README.md | 58 ++++++++- .../core/dataset/from_insights.py | 108 +++++++++++++--- .../src/gooddata_eval/core/granularity.py | 65 ++++++++++ .../src/gooddata_eval/core/scoring.py | 3 +- .../gooddata-eval/tests/test_from_insights.py | 115 ++++++++++++++++++ packages/gooddata-eval/tests/test_scoring.py | 23 ++++ 6 files changed, 350 insertions(+), 22 deletions(-) create mode 100644 packages/gooddata-eval/src/gooddata_eval/core/granularity.py diff --git a/packages/gooddata-eval/README.md b/packages/gooddata-eval/README.md index 68c9c1d2f..f140cc645 100644 --- a/packages/gooddata-eval/README.md +++ b/packages/gooddata-eval/README.md @@ -20,6 +20,7 @@ Or install `gd-eval` as a standalone tool: | `gd-eval models` | List LLM providers and models configured in the org. | | `gd-eval generate` | Generate a `visualization` dataset from a workspace's existing insights. | + --- ## `gd-eval run` @@ -291,8 +292,57 @@ written into every item (and the default output folder). | `--no-phrase` | skip the LLM; emit mechanical `Show ` questions | | `--phrase-model` | OpenAI model for phrasing (default `gpt-4o`) | | `--no-viz-type` | always blank the expected chart type | +| `--enrich-ranked <N>` | additionally derive up to N ranked questions (see below); default 0 (off) | +| `--skip-ambiguous` | drop items naming something the model carries more than once; reported either way | | `--min-questions` / `--min-shapes` / `--min-filtered` | quality gate, default 15, 3 and 1 | +### Ranked questions (`--enrich-ranked`) + +Analysts sort in Analytical Designer and save the chart without persisting the sort, so +`sort_by`/`ranking_filter` coverage is near zero on most real models — the eval can +punish a spurious ranking but never confirm the agent builds a required one. +`--enrich-ranked N` fills that gap by *deriving* ranked items from the specs already +extracted. Adding a limit or a sort to a definition that executes cannot make it +unanswerable, and "the top 3 X by Y" has exactly one correct spec, so a derived item is +less ambiguous to grade than the insight it came from. + +The budget is spent best-grounded first: + +1. **Insights whose own title promised a ranking their definition never implemented** — + "Top Returned Reasons" saved with `sorts: []`. The direction comes from the title + (`highest`/`most`/`largest` vs `lowest`/`least`/`worst`) and the N too when it states + one; a title naming both ends names neither and is still skipped. +2. **Ranking filters added to a plain breakdown** — one metric, one non-date dimension, + no existing sort. N follows the dimension's element count, so a top-5 over six values + is never emitted. +3. **Sort-only variants**, which order without limiting. + +Eligibility is deliberately narrow: two metrics leave "top 3 by what?" unanswered, a +second dimension leaves the N ambiguous between the pair and within a group, and a date +dimension turns the result into "top 3 months", which nobody asks. Variants are +deduplicated by resolved definition — differently-titled insights over one metric and +dimension would otherwise produce the same question twice — and bases are taken +round-robin by metric so one popular metric cannot become a third of the corpus. + +Derived items carry `derived_from` (the insight id) and `derived_basis` (`title` when a +human's chart title asked for the ranking, `shape` when this generator chose to add +one), so a pass rate over each can be computed separately. + +### Items that cannot say what they mean + +Two classes of question are unwinnable however well the agent behaves, and both are +reported: + +- **A name the model carries more than once.** One workspace has six labels all titled + "Product Title"; a question naming one cannot say which is meant, and a perfect chart + over the wrong one scores zero. `--skip-ambiguous` drops them; the count and the + offending names are printed either way. +- **A date granularity's cyclical twin.** `MONTH` walks consecutive calendar months, + `MONTH_OF_YEAR` stacks every January together. Date dimensions are therefore briefed + by what they do ("one point per calendar month over time, not month-of-year") and the + writer is told to say it in natural words while keeping the date dataset's name — + never as a label id in prose ("Order Created At - Month"). + **The question must never contradict its own expected output.** Four rules enforce that: - The writer is briefed on buckets, sorts and filters only — never the insight title, @@ -308,7 +358,10 @@ written into every item (and the default output folder). - The writer's rules are built per insight, so an insight with no `view_by` is never asked to name a breakdown at all. - `type` is set only when the question actually names a chart form. An insight's - `visualizationUrl` records what a human clicked, not what the question constrains. + `visualizationUrl` records what a human clicked, not what the question constrains — + with one exception: a chart with no breakdown *must* name its form ("as a KPI", "as a + single number"). Without it the agent reads a bare "Show me Gross Revenue" as a metric + lookup, activates only its search skill and builds nothing. Everything the writer sees is a display name (`Spend Amount`, `Merchant Name`), never a raw URI, so questions read like a person wrote them. @@ -316,7 +369,8 @@ raw URI, so questions read like a person wrote them. **What it won't do.** Insights it can't express without guessing are skipped with a printed reason, never approximated: derived (arithmetic/PoP) measures, measure-level filters, `uris`-form attribute filters, unmapped chart types, hidden objects, and -insights whose title promises behaviour their definition lacks. If too few survive, the +insights whose title promises behaviour their definition lacks (though `--enrich-ranked` +implements a promised *ranking* rather than discarding it). If too few survive, the quality gate fails the run rather than fabricating items to hit the minimum — point at more dashboards, or lower `--min-questions`. diff --git a/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py b/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py index e7bd60466..cabd0be32 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py @@ -29,6 +29,13 @@ from pathlib import Path from typing import Any +from gooddata_eval.core.granularity import ( + GRANULARITIES, + GRANULARITY_BY_ID, + _camel, + canonical_date_uri, + granularity_of, +) from gooddata_eval.core.models import CreatedVisualization, DatasetItem # AD `visualizationUrl` -> AAC type. Explicit and exhaustive: an unmapped url raises @@ -718,17 +725,6 @@ def walk(node): return found -GRANULARITY_TITLES = { - "minute": "Minute", - "hour": "Hour", - "day": "Day", - "week": "Week", - "month": "Month", - "quarter": "Quarter", - "year": "Year", -} - - def build_display_names(analytics: dict, ldm: dict) -> dict: """`{uri: human title}` for every metric, fact, label and date dataset. @@ -753,8 +749,13 @@ def build_display_names(analytics: dict, ldm: dict) -> dict: title = instance.get("title") or instance["id"] names[f"dataset/{instance['id']}"] = title for granularity in instance.get("granularities") or []: - key = granularity.lower() - names[f"label/{instance['id']}.{key}"] = f"{title} - {GRANULARITY_TITLES.get(key, granularity.title())}" + enum = GRANULARITY_BY_ID.get(granularity, granularity.upper()) + suffix = GRANULARITIES.get(enum, (granularity.title(), ""))[0] + # Registered under every spelling: the LDM declares MONTH_OF_YEAR, the API + # returns `monthOfYear`, and a lookup under one must not miss the other and + # fall back to a de-slugged id ("Order Created At - Monthofyear"). + for spelling in {_camel(enum), enum.lower(), granularity}: + names[f"label/{instance['id']}.{spelling}"] = f"{title} - {suffix}" return names @@ -811,9 +812,12 @@ def describe(spec: dict, display_names: dict | None = None) -> str: def name(alias: str) -> str: return display_name(field_uri(fields, alias), display_names) + def dim(aliases: list) -> list: + return _dim_briefs(spec, display_names, aliases) + lines = [f"metric: {name(a)}" for a in spec["metrics"]] - lines += [f"broken down by: {name(a)}" for a in spec["view_by"] + spec["columns"] + spec["rows"]] - lines += [f"split by: {name(a)}" for a in spec["segment_by"]] + lines += [f"broken down by: {d}" for d in dim(spec["view_by"] + spec["columns"] + spec["rows"])] + lines += [f"split by: {d}" for d in dim(spec["segment_by"])] lines += [f"sorted by: {name(s['field'])}, {s['direction'].lower()}ending" for s in spec["sort_by"]] lines += [f"filter: {_filter_phrase(f, fields, display_names)}" for f in spec["query"]["filter_by"].values()] return "\n".join(lines) @@ -829,10 +833,13 @@ def ambiguous_titles(display_names: dict) -> set: """ seen, dupes = {}, set() for uri, title in display_names.items(): + # Every granularity is registered under several spellings of one label, so the + # aliases must fold together or each date dimension looks like a name collision. + canonical = canonical_date_uri(uri) key = _normalize(title) - if key in seen and seen[key] != uri: + if key in seen and seen[key] != canonical: dupes.add(key) - seen.setdefault(key, uri) + seen.setdefault(key, canonical) return dupes @@ -855,6 +862,30 @@ def field_uri(fields: dict, alias: str) -> str: return field["using"] if isinstance(field, dict) else field +def granularity_phrase(uri: str, display_names: dict) -> str | None: + """What a date breakdown does, in words, or None if `uri` is not a date granularity. + + "Order Created At - Month" is a label name, not something a person says, and it does + not distinguish the sequential granularity from its cyclical twin. The phrase does + both: it pins the date dataset and states which reading is meant. + """ + enum = granularity_of(uri) + if enum is None: + return None + dataset = uri.split("/", 1)[-1].rpartition(".")[0] + return f"{display_name(f'dataset/{dataset}', display_names)}, {GRANULARITIES[enum][1]}" + + +def _dim_briefs(spec: dict, display_names: dict, aliases: list) -> list: + """Dimension names for the writer: date dimensions as phrases, labels verbatim.""" + fields = spec["query"]["fields"] + out = [] + for alias in aliases: + uri = field_uri(fields, alias) + out.append(granularity_phrase(uri, display_names) or display_name(uri, display_names)) + return out + + def _dim_names(spec: dict, display_names: dict) -> list: fields = spec["query"]["fields"] aliases = spec["view_by"] + spec["segment_by"] + spec["columns"] + spec["rows"] @@ -877,16 +908,37 @@ def _mentions(name: str, question: str) -> bool: return any(t in lowered for t in tokens) if tokens else _normalize(name) in lowered +def _without_field_names(question: str, spec: dict, display_names: dict) -> str: + """`question` with the spec's own field names blanked out. + + A field may be called "Most Recent Label Created At" or "Top Tier Customers". A + question naming it verbatim -- which the rules require -- is not thereby claiming a + ranking, so the claim checks have to read around the names. + """ + fields = spec["query"]["fields"] + names = [display_name(field_uri(fields, a), display_names) for a in fields] + names += [display_name(f.get("using", ""), display_names) for f in spec["query"]["filter_by"].values()] + # A date label reads "Most Recent Label Created At - Month" but the question names + # the dataset and the granularity separately ("by month for Most Recent Label + # Created At"), so each side of the separator has to be maskable on its own. + names += [part for name in list(names) for part in name.split(" - ")] + for name in sorted(names, key=len, reverse=True): + if len(name.strip()) > 3: + question = re.sub(re.escape(name.strip()), " ", question, flags=re.I) + return question + + def contradictions(question: str, spec: dict, display_names: dict | None = None) -> list: """Ways `question` and `spec` disagree. Any hit is a hard error, never a warning.""" display_names = display_names or {} problems = [] + claims = _without_field_names(question, spec, display_names) if not ranks(spec): - hit = RANK_WORDS.search(question) + hit = RANK_WORDS.search(claims) if hit: problems.append(f"uses ranking word '{hit.group(0)}' but the chart has no sort or ranking filter") if not filters(spec): - hit = FILTER_WORDS.search(question) + hit = FILTER_WORDS.search(claims) if hit: problems.append(f"uses filter word '{hit.group(0)}' but the chart has no date or attribute filter") @@ -913,6 +965,12 @@ def contradictions(question: str, spec: dict, display_names: dict | None = None) def _rules_for(spec: dict, display_names: dict) -> str: dims = _dim_names(spec, display_names) + all_dims = spec["view_by"] + spec["segment_by"] + spec["columns"] + spec["rows"] + dated = [ + phrase + for alias in all_dims + if (phrase := granularity_phrase(field_uri(spec["query"]["fields"], alias), display_names)) + ] segments = [display_name(field_uri(spec["query"]["fields"], a), display_names) for a in spec["segment_by"]] lines = [ "Write the question an analyst would ask to get exactly this chart. Rules:", @@ -931,6 +989,18 @@ def _rules_for(spec: dict, display_names: dict) -> str: f"{ranked_dim} by <metric>' and name {ranked_dim} exactly once -- do not also say " f"'broken down by {ranked_dim}'." ) + elif dated: + # A date breakdown is the one dimension not to quote verbatim: "broken down by + # Order Created At - Month" is a label id in prose, and it leaves the agent to + # guess between the sequential granularity and its cyclical twin. + plain = [name for name in dims if not any(name.startswith(p.split(",")[0]) for p in dated)] + lines.append( + "- Say the question is broken down by " + "; and ".join(dated) + ". Write that in " + "natural words ('by month', 'monthly', 'per month'), never as a label name like " + "'Order Created At - Month', but do keep the date dataset's name." + ) + if plain: + lines.append("- It is also broken down by " + ", ".join(plain) + ", naming each verbatim.") elif dims: lines.append("- Say the question is broken down by " + ", ".join(dims) + ", naming each verbatim.") else: diff --git a/packages/gooddata-eval/src/gooddata_eval/core/granularity.py b/packages/gooddata-eval/src/gooddata_eval/core/granularity.py new file mode 100644 index 000000000..76a749a17 --- /dev/null +++ b/packages/gooddata-eval/src/gooddata_eval/core/granularity.py @@ -0,0 +1,65 @@ +# (C) 2026 GoodData Corporation +"""Date granularities: a closed platform enum, shared by dataset generation and scoring.""" + +# Date granularities are a closed platform enum (`gooddata_api_client`), identical in +# every workspace, so unlike metric and label names they can be described once and for +# all. Each entry is (title, phrase): the title names the label, the phrase says what the +# breakdown actually does. The phrase matters because a granularity has a cyclical twin -- +# MONTH walks consecutive calendar months, MONTH_OF_YEAR stacks every January together -- +# and a question saying only "by month" does not choose between them. gpt-5.6-luna built +# `monthOfYear` where the insight used `month`, which is a defensible reading of the words. +GRANULARITIES = { + "MINUTE": ("Minute", "by minute, consecutive minutes over time (not minute-of-hour)"), + "HOUR": ("Hour", "by hour, consecutive hours over time (not hour-of-day)"), + "DAY": ("Day", "by day, one point per calendar day over time (not day-of-week)"), + "WEEK": ("Week", "by week, consecutive calendar weeks over time (not week-of-year)"), + "MONTH": ("Month", "by month, one point per calendar month over time (not month-of-year)"), + "QUARTER": ("Quarter", "by quarter, consecutive calendar quarters over time (not quarter-of-year)"), + "YEAR": ("Year", "by year, one point per calendar year"), + "MINUTE_OF_HOUR": ("Minute of Hour", "by minute of the hour (0-59), combining every hour"), + "MINUTE_OF_DAY": ("Minute of Day", "by minute of the day, combining every day"), + "HOUR_OF_DAY": ("Hour of Day", "by hour of the day (0-23), combining every day"), + "DAY_OF_WEEK": ("Day of Week", "by day of the week (Monday to Sunday), combining every week"), + "DAY_OF_MONTH": ("Day of Month", "by day of the month (1-31), combining every month"), + "DAY_OF_QUARTER": ("Day of Quarter", "by day of the quarter, combining every quarter"), + "DAY_OF_YEAR": ("Day of Year", "by day of the year (1-366), combining every year"), + "WEEK_OF_YEAR": ("Week of Year", "by week of the year (1-53), combining every year"), + "MONTH_OF_YEAR": ("Month of Year", "by month of the year (January to December), combining every year"), + "QUARTER_OF_YEAR": ("Quarter of Year", "by quarter of the year (Q1 to Q4), combining every year"), +} + + +def _camel(granularity: str) -> str: + """MONTH_OF_YEAR -> monthOfYear, the spelling the platform's label ids actually use.""" + head, *rest = granularity.lower().split("_") + return head + "".join(part.title() for part in rest) + + +# Both spellings resolve: label ids come back camelCase from the API, while the +# declarative LDM lists the granularity as the upper-snake enum member. +GRANULARITY_BY_ID = {spelling: enum for enum in GRANULARITIES for spelling in (_camel(enum), enum.lower(), enum)} + + +def granularity_of(uri: str) -> str | None: + """The granularity `uri` ends in, as an enum member, or None if it is not a date ref.""" + stem = uri.split("/", 1)[-1] + if "." not in stem: + return None + return GRANULARITY_BY_ID.get(stem.rpartition(".")[2]) + + +def canonical_date_uri(uri: str) -> str: + """One spelling for a date granularity, whichever the platform happened to return. + + A date dataset exposes each granularity as an attribute whose only label carries the + same id, so `attribute/order_created_at.month` and `label/order_created_at.month` + denote the same breakdown. Both come back from the API depending on how the agent + built the chart, and comparing the raw strings failed a chart that was correct. The + granularity itself is folded to one spelling too: the API returns `monthOfYear` where + the declarative LDM says `MONTH_OF_YEAR`, and the two are one breakdown, not two. + """ + enum = granularity_of(uri) + if enum is None: + return uri + dataset = uri.split("/", 1)[-1].rpartition(".")[0] + return f"label/{dataset}.{_camel(enum)}" diff --git a/packages/gooddata-eval/src/gooddata_eval/core/scoring.py b/packages/gooddata-eval/src/gooddata_eval/core/scoring.py index 0d554cd85..3b471baf2 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/scoring.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/scoring.py @@ -4,6 +4,7 @@ import json from dataclasses import dataclass +from gooddata_eval.core.granularity import canonical_date_uri from gooddata_eval.core.models import AacBucketRef, AacQueryField, CreatedVisualization # Maps dataset chart-type names (and agent enum values) to a canonical token. @@ -43,7 +44,7 @@ def _resolve_bucket_to_uri_set(bucket: list[AacBucketRef | str], fields: dict[st uris: set[str] = set() for ref in bucket: alias = ref.field if isinstance(ref, AacBucketRef) else ref - uris.add(_resolve_alias_to_uri(alias, fields)) + uris.add(canonical_date_uri(_resolve_alias_to_uri(alias, fields))) return uris diff --git a/packages/gooddata-eval/tests/test_from_insights.py b/packages/gooddata-eval/tests/test_from_insights.py index 52d22a2dc..69b6def65 100644 --- a/packages/gooddata-eval/tests/test_from_insights.py +++ b/packages/gooddata-eval/tests/test_from_insights.py @@ -19,6 +19,7 @@ describe, display_name, element_counts, + granularity_phrase, insight_ids_on, langfuse_payload, list_ids, @@ -1084,3 +1085,117 @@ def test_an_ambiguous_metric_name_counts_too(): spec = convert(_bar("spend", "merchant.NAME"), DATE_IDS) names = {**DISPLAY, "metric/spend_v2": "Spend Amount"} assert ambiguous_fields(spec, names) == ["Spend Amount"] + + +# --- date granularities ------------------------------------------------------- + + +@pytest.mark.parametrize("spelling", ["monthOfYear", "month_of_year", "MONTH_OF_YEAR"]) +def test_every_spelling_of_a_granularity_resolves(spelling): + # The API returns label ids camelCase; the declarative LDM lists the enum member. + # A lookup under one spelling must not miss the other and fall back to a de-slugged + # id ("Order Created At - Monthofyear"). + phrase = granularity_phrase(f"label/ORDER_CREATED_AT.{spelling}", {"dataset/ORDER_CREATED_AT": "Order Created At"}) + assert phrase == "Order Created At, by month of the year (January to December), combining every year" + + +def test_a_sequential_granularity_rules_out_its_cyclical_twin(): + phrase = granularity_phrase("label/ORDER_CREATED_AT.month", {"dataset/ORDER_CREATED_AT": "Order Created At"}) + assert "one point per calendar month over time" in phrase + assert "not month-of-year" in phrase + + +def test_a_plain_label_has_no_granularity_phrase(): + assert granularity_phrase("label/product_details.LINE_ITEM_TITLE", DISPLAY) is None + assert granularity_phrase("metric/spend", DISPLAY) is None + + +def test_display_names_cover_both_spellings_of_every_granularity(): + names = build_display_names( + {}, + { + "dateInstances": [ + {"id": "ORDER_CREATED_AT", "title": "Order Created At", "granularities": ["MONTH_OF_YEAR"]} + ] + }, + ) + assert names["label/ORDER_CREATED_AT.monthOfYear"] == "Order Created At - Month of Year" + assert names["label/ORDER_CREATED_AT.month_of_year"] == "Order Created At - Month of Year" + + +def _monthly(metric="spend", granularity="month"): + return viz( + "local:line", + [ + {"localIdentifier": "measures", "items": [measure("m", metric)]}, + {"localIdentifier": "trend", "items": [attribute("a", f"process_date.{granularity}")]}, + ], + ) + + +def test_a_date_breakdown_is_briefed_by_what_it_does(): + spec = convert(_monthly(), DATE_IDS) + brief = describe(spec, DISPLAY) + assert "broken down by: Process Date, by month, one point per calendar month over time" in brief + assert "Process Date - Month" not in brief, "a label id in prose is not what an analyst says" + + +def test_a_date_breakdown_is_not_asked_for_verbatim(): + rules = _rules_for(convert(_monthly(), DATE_IDS), DISPLAY) + assert "natural words" in rules + assert "never as a label name" in rules + assert "naming each verbatim" not in rules + + +def test_a_plain_dimension_is_still_asked_for_verbatim(): + rules = _rules_for(convert(spend_by_merchant(), DATE_IDS), DISPLAY) + assert "broken down by Merchant Name, naming each verbatim" in rules + assert "natural words" not in rules + + +def test_a_question_saying_by_month_still_names_the_dimension(): + spec = convert(_monthly(), DATE_IDS) + assert contradictions("Can you show me Spend Amount by month for Process Date?", spec, DISPLAY) == [] + + +def test_a_ranking_word_inside_a_field_name_is_not_a_claim(): + # loop's date dataset is called "Most Recent Label Created At". A question naming it + # verbatim -- which the rules require -- was dropped for "using ranking word 'Most'". + spec = convert( + viz( + "local:line", + [ + {"localIdentifier": "measures", "items": [measure("m", "total_labels")]}, + {"localIdentifier": "trend", "items": [attribute("a", "most_recent_label_created_at.month")]}, + ], + ), + {"most_recent_label_created_at"}, + ) + names = { + "metric/total_labels": "Total Labels", + "dataset/most_recent_label_created_at": "Most Recent Label Created At", + "label/most_recent_label_created_at.month": "Most Recent Label Created At - Month", + } + question = "Can you show me Total Labels by month for Most Recent Label Created At?" + assert contradictions(question, spec, names) == [] + + +def test_a_real_ranking_claim_is_still_caught_around_the_names(): + spec = convert(spend_by_merchant(), DATE_IDS) + problems = contradictions("Show me the top 5 Merchant Name values by Spend Amount", spec, DISPLAY) + assert any("ranking word" in p for p in problems) + + +def test_a_filter_word_inside_a_field_name_is_not_a_claim(): + spec = convert(spend_by_merchant(), DATE_IDS) + names = {**DISPLAY, "label/merchant.NAME": "Merchant Name Excluding Test Accounts"} + assert contradictions("Show me Spend Amount by Merchant Name Excluding Test Accounts", spec, names) == [] + + +def test_granularity_aliases_are_one_object_not_a_collision(): + names = build_display_names( + {}, + {"dateInstances": [{"id": "RETURN_AT", "title": "Return At", "granularities": ["MONTH", "MONTH_OF_YEAR"]}]}, + ) + # Each granularity is registered under several spellings; the aliases must fold. + assert ambiguous_titles(names) == set() diff --git a/packages/gooddata-eval/tests/test_scoring.py b/packages/gooddata-eval/tests/test_scoring.py index 873e30628..57f858bb1 100644 --- a/packages/gooddata-eval/tests/test_scoring.py +++ b/packages/gooddata-eval/tests/test_scoring.py @@ -205,3 +205,26 @@ def test_normalized_filters_is_empty_per_category_when_unfiltered(): } ) assert normalized_filters(viz) == {"date": [], "ranking": [], "attribute": []} + + +def test_a_date_granularity_compares_equal_whichever_prefix_it_carries(): + # A date dataset exposes each granularity as an attribute whose only label carries + # the same id, so both spellings denote one breakdown. gpt-5.6-luna returned + # `attribute/ORDER_CREATED_AT.month` for a chart the insight recorded as + # `label/ORDER_CREATED_AT.month`, and the raw string compare failed a correct chart. + as_label = _viz(query={"fields": {"d": {"using": "label/ORDER_CREATED_AT.month"}}, "filter_by": {}}, view_by=["d"]) + as_attribute = _viz( + query={"fields": {"d": {"using": "attribute/ORDER_CREATED_AT.month"}}, "filter_by": {}}, view_by=["d"] + ) + assert get_dimension_uri_set(as_label) == get_dimension_uri_set(as_attribute) + + +def test_the_granularity_itself_still_has_to_match(): + sequential = _viz(query={"fields": {"d": {"using": "label/d.month"}}, "filter_by": {}}, view_by=["d"]) + cyclical = _viz(query={"fields": {"d": {"using": "label/d.monthOfYear"}}, "filter_by": {}}, view_by=["d"]) + assert get_dimension_uri_set(sequential) != get_dimension_uri_set(cyclical) + + +def test_a_plain_attribute_is_not_rewritten_as_a_label(): + viz = _viz(query={"fields": {"d": {"using": "attribute/product.title"}}, "filter_by": {}}, view_by=["d"]) + assert get_dimension_uri_set(viz) == {"attribute/product.title"} From b208529ac0ad99b167001f56ec2b14e61eaee8ea Mon Sep 17 00:00:00 2001 From: Roman Valovic <xvalovic@mendelu.cz> Date: Thu, 10 Sep 2026 15:53:22 +0200 Subject: [PATCH 13/15] chore(gooddata-eval): add the copyright header the pre-commit hook requires `report_template.html` was the one file in the package without it, so the Copyright hook failed and rewrote the file on every commit attempt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- .../src/gooddata_eval/core/reporting/report_template.html | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html b/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html index 7d9b330ea..209124775 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html +++ b/packages/gooddata-eval/src/gooddata_eval/core/reporting/report_template.html @@ -1,3 +1,4 @@ +<!-- (C) 2026 GoodData Corporation --> <!doctype html> <html lang="en"> <head> From b6c3400d7ebe77e34a2d6274f7806d2e4f84a9a1 Mon Sep 17 00:00:00 2001 From: Roman Valovic <xvalovic@mendelu.cz> Date: Thu, 10 Sep 2026 16:39:29 +0200 Subject: [PATCH 14/15] fix(gooddata-eval): satisfy `ty` on the generator and the timeout caps `make type-check` is a CI gate and this branch had never been run through it. Five real diagnostics, none of them cosmetic: - `ChatClient.__init__` rebound its `timeout: float` parameter to an `httpx.Timeout`; the capped value now goes in its own local. - `_rules_for` narrowed `ranked_dim` and then read `ranking`, which `ty` correctly refuses -- `ranking` is what has to be checked. - `generate()` called `sdk_factory()` where the parameter defaults to None. It now says what is missing instead of raising TypeError one frame later. - Typing the phrasing `messages` list stopped it being over-narrowed against the OpenAI signature, which then surfaced a latent crash: `message.content` is `str | None`, and a refusal or tool-call-only reply would have died on `.strip()` mid-generation. An empty candidate now takes the existing retry. Note the openai diagnostics only appear locally: CI has no openai installed and `allowed-unresolved-imports` covers it, so `ty` never checks those call sites there. 1046 tests pass; type-check, lint and format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- .../src/gooddata_eval/core/chat/sse_client.py | 5 +++-- .../src/gooddata_eval/core/dataset/from_insights.py | 11 ++++++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py index b30156b03..2a35fb14c 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py @@ -434,9 +434,10 @@ def __init__( # Anchored when a conversation is created; spans every turn taken on it. self._conversation_started: float | None = None caps = [c for c in (self._turn_timeout_s, self._item_timeout_s) if c is not None] + http_timeout: float | httpx.Timeout = timeout if caps: - timeout = httpx.Timeout(timeout, read=min(timeout, *caps)) - self._client = httpx.Client(timeout=timeout) + http_timeout = httpx.Timeout(timeout, read=min(timeout, *caps)) + self._client = httpx.Client(timeout=http_timeout) self._preserve_failed = preserve_failed self._reasoning_effort = normalize_reasoning_effort(reasoning_effort) self._agent_id = agent_id diff --git a/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py b/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py index cabd0be32..bc1cf3d5c 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/dataset/from_insights.py @@ -978,7 +978,7 @@ def _rules_for(spec: dict, display_names: dict) -> str: ] ranking = next((f for f in spec["query"]["filter_by"].values() if f.get("type") == "ranking_filter"), None) ranked_dim = dims[0] if ranking and dims and not ranking.get("attribute") else None - if ranked_dim: + if ranking is not None and ranked_dim: # The ranking phrasing already names the dimension. Asking for the breakdown as # well produces "broken down by Carrier, showing the top 3 Carriers by Returns" -- # the dimension twice, which no analyst writes. One instruction, not two. @@ -1052,14 +1052,17 @@ def phrase(specs: list, model: str, display_names: dict) -> list: client = OpenAI() questions = [] for i, spec in enumerate(specs, 1): - messages = [ + messages: list = [ {"role": "system", "content": PHRASE_SYSTEM}, {"role": "user", "content": f"{describe(spec, display_names)}\n\n{_rules_for(spec, display_names)}"}, ] question, problems = None, [] for _attempt in range(2): reply = client.chat.completions.create(model=model, messages=messages) - candidate = reply.choices[0].message.content.strip().strip('"') + # `content` is None when the model returns a refusal or no text at all; an + # empty candidate fails the contradiction check and takes the retry, which is + # what should happen anyway. + candidate = (reply.choices[0].message.content or "").strip().strip('"') problems = contradictions(candidate, spec, display_names) if not problems: question = candidate @@ -1161,6 +1164,8 @@ def generate(args, sdk_factory=None) -> int: if args.snapshot_in: snapshot = json.loads(Path(args.snapshot_in).read_text()) else: + if sdk_factory is None: + raise ValueError("a live run needs an SDK; pass --snapshot-in to replay a saved model instead") sdk = sdk_factory() snapshot = fetch_snapshot(sdk, args.workspace) From b107fd90f840d509df4e606a777eb72f8c863b87 Mon Sep 17 00:00:00 2001 From: Roman Valovic <xvalovic@mendelu.cz> Date: Thu, 10 Sep 2026 18:29:52 +0200 Subject: [PATCH 15/15] test(gooddata-eval): cover the generate pipeline and the phrasing step Codecov failed the PR on patch coverage: 77% of the new lines, against a repo at 82%. Nearly all of the gap was two functions that talk to the outside world and had no tests at all -- `generate()` (~200 lines of fetch, report, gate and write) and `phrase()` (the OpenAI retry loop). Both were only ever exercised by running them against a live workspace. `generate()` is now driven through its `--snapshot-in` path with a fake args object, covering the quality gate's exit code, hidden-insight skipping, dry-run, the Langfuse export and its id prefix, ranked derivation, `--skip-ambiguous`, dashboard filtering (and an unknown dashboard), `--snapshot-out`, the missing-SDK message and `--no-viz-type`. `phrase()` runs against an OpenAI stub, pinning the behaviour that matters: a clean question is not re-asked, a contradiction is quoted back once and the rewrite kept, a second failure drops the item rather than shipping a question its own expected_output disagrees with, a None-content refusal counts as a failed attempt, and a missing API key fails up front instead of per item. from_insights.py 72% -> 92%; package 88% -> 90%. 1064 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- .../gooddata-eval/tests/test_from_insights.py | 281 ++++++++++++++++++ 1 file changed, 281 insertions(+) diff --git a/packages/gooddata-eval/tests/test_from_insights.py b/packages/gooddata-eval/tests/test_from_insights.py index 69b6def65..5de5f54f0 100644 --- a/packages/gooddata-eval/tests/test_from_insights.py +++ b/packages/gooddata-eval/tests/test_from_insights.py @@ -1,7 +1,10 @@ # (C) 2026 GoodData Corporation import json +from types import SimpleNamespace +from unittest.mock import patch import pytest +from gooddata_eval.core.dataset import from_insights as from_insights_mod from gooddata_eval.core.dataset.from_insights import ( PromisedRanking, Unsupported, @@ -19,6 +22,7 @@ describe, display_name, element_counts, + generate, granularity_phrase, insight_ids_on, langfuse_payload, @@ -1199,3 +1203,280 @@ def test_granularity_aliases_are_one_object_not_a_collision(): ) # Each granularity is registered under several spellings; the aliases must fold. assert ambiguous_titles(names) == set() + + +# --- the generate() pipeline -------------------------------------------------- + + +def _snapshot(*vizs, granularities=("MONTH",)): + return { + "workspace_id": "ws", + "analytics": { + "visualizationObjects": list(vizs), + "metrics": [{"id": "spend", "title": "Spend Amount"}], + }, + "date_instance_ids": ["process_date"], + "display_names": { + "metric/spend": "Spend Amount", + "metric/revenue": "Revenue Amount", + "label/merchant.NAME": "Merchant Name", + "label/region.NAME": "Region Name", + "dataset/process_date": "Process Date", + }, + "label_cardinality": {"label/merchant.NAME": 40, "label/region.NAME": 40}, + } + + +def _args(tmp_path, **kw): + base = { + "workspace": "ws", + "dataset_name": "d", + "out": str(tmp_path / "out"), + "dashboard": [], + "snapshot_in": None, + "snapshot_out": None, + "langfuse_out": None, + "id_prefix": "", + "no_phrase": True, + "phrase_model": "gpt-4o", + "no_viz_type": False, + "min_questions": 1, + "min_shapes": 1, + "min_filtered": 0, + "enrich_ranked": 0, + "skip_ambiguous": False, + "dry_run": False, + } + return SimpleNamespace(**{**base, **kw}) + + +def _snapshot_file(tmp_path, snapshot): + path = tmp_path / "snap.json" + path.write_text(json.dumps(snapshot)) + return str(path) + + +def test_generate_writes_a_validated_item_per_insight(tmp_path): + snapshot = _snapshot(_bar("spend", "merchant.NAME"), _bar("revenue", "region.NAME")) + args = _args(tmp_path, snapshot_in=_snapshot_file(tmp_path, snapshot)) + + assert generate(args) == 0 + + written = sorted(p.name for p in (tmp_path / "out").glob("*.json")) + assert len(written) == 2 + for path in (tmp_path / "out").glob("*.json"): + assert _validation_errors(json.loads(path.read_text())) is None + + +def test_generate_fails_the_run_when_too_few_insights_survive(tmp_path): + # The gate exists so a thin workspace fails loudly instead of quietly shipping a + # dataset too small to mean anything. + args = _args(tmp_path, snapshot_in=_snapshot_file(tmp_path, _snapshot(_bar("spend", "merchant.NAME")))) + args.min_questions = 15 + + assert generate(args) == 1 + assert list((tmp_path / "out").glob("*.json")), "the items are still written; only the exit code fails" + + +def test_generate_skips_hidden_insights(tmp_path): + # Hidden objects are invisible to the assistant's catalog search, so a question about + # one is unwinnable rather than merely hard. + hidden = _bar("spend", "merchant.NAME") + hidden["isHidden"] = True + args = _args(tmp_path, snapshot_in=_snapshot_file(tmp_path, _snapshot(hidden, _bar("revenue", "region.NAME")))) + + assert generate(args) == 0 + assert len(list((tmp_path / "out").glob("*.json"))) == 1 + + +def test_generate_dry_run_writes_nothing(tmp_path): + args = _args( + tmp_path, snapshot_in=_snapshot_file(tmp_path, _snapshot(_bar("spend", "merchant.NAME"))), dry_run=True + ) + + assert generate(args) == 0 + assert not (tmp_path / "out").exists() + + +def test_generate_exports_a_langfuse_dataset_with_prefixed_ids(tmp_path): + args = _args( + tmp_path, + snapshot_in=_snapshot_file(tmp_path, _snapshot(_bar("spend", "merchant.NAME"))), + langfuse_out=str(tmp_path / "lf.json"), + id_prefix="lr-", + ) + assert generate(args) == 0 + + payload = json.loads((tmp_path / "lf.json").read_text()) + assert payload["workspace"] == "ws" + assert all(item["id"].startswith("lr-") for item in payload["items"]) + + +def test_generate_derives_ranked_items_and_records_their_provenance(tmp_path): + args = _args( + tmp_path, + snapshot_in=_snapshot_file(tmp_path, _snapshot(_bar("spend", "merchant.NAME"))), + enrich_ranked=2, + ) + assert generate(args) == 0 + + items = [json.loads(p.read_text()) for p in (tmp_path / "out").glob("*.json")] + derived = [i for i in items if i.get("derived_from")] + assert len(derived) == 2 + assert {i["derived_kind"] for i in derived} == {"ranking_filter", "sort_by"} + + +def test_generate_can_drop_the_items_that_name_something_ambiguous(tmp_path): + snapshot = _snapshot(_bar("spend", "merchant.NAME"), _bar("revenue", "region.NAME")) + snapshot["display_names"]["label/other.NAME"] = "Merchant Name" # a second "Merchant Name" + args = _args(tmp_path, snapshot_in=_snapshot_file(tmp_path, snapshot), skip_ambiguous=True) + + assert generate(args) == 0 + kept = [json.loads(p.read_text())["question"] for p in (tmp_path / "out").glob("*.json")] + assert len(kept) == 1, "the item naming the duplicated label is dropped" + + +def test_generate_restricts_to_the_requested_dashboard(tmp_path): + keep, drop = _bar("spend", "merchant.NAME"), _bar("revenue", "region.NAME") + keep["id"], drop["id"] = "v_keep", "v_drop" + snapshot = _snapshot(keep, drop) + snapshot["analytics"]["analyticalDashboards"] = [ + {"id": "dash", "content": {"layout": [{"type": "insight", "insight": {"identifier": {"id": "v_keep"}}}]}} + ] + args = _args(tmp_path, snapshot_in=_snapshot_file(tmp_path, snapshot), dashboard=["dash"]) + + assert generate(args) == 0 + assert len(list((tmp_path / "out").glob("*.json"))) == 1 + + +def test_generate_reports_an_unknown_dashboard_instead_of_generating_everything(tmp_path): + args = _args(tmp_path, snapshot_in=_snapshot_file(tmp_path, _snapshot(_bar("spend", "merchant.NAME")))) + args.dashboard = ["nope"] + + assert generate(args) == 1 + assert not (tmp_path / "out").exists() + + +def test_generate_saves_the_fetched_snapshot_for_replay(tmp_path): + class _Sdk: + pass + + calls = {} + + def fake_fetch(sdk, workspace_id): + calls["workspace"] = workspace_id + return _snapshot(_bar("spend", "merchant.NAME")) + + args = _args(tmp_path, snapshot_out=str(tmp_path / "snap-out.json")) + with patch.object(from_insights_mod, "fetch_snapshot", fake_fetch): + assert generate(args, sdk_factory=_Sdk) == 0 + + assert calls["workspace"] == "ws" + assert json.loads((tmp_path / "snap-out.json").read_text())["workspace_id"] == "ws" + + +def test_generate_without_a_snapshot_or_an_sdk_says_which_is_missing(tmp_path): + with pytest.raises(ValueError, match="snapshot-in"): + generate(_args(tmp_path)) + + +def test_generate_blanks_every_expected_chart_type_on_request(tmp_path): + args = _args(tmp_path, snapshot_in=_snapshot_file(tmp_path, _snapshot(_bar("spend", "merchant.NAME")))) + args.no_viz_type = True + assert generate(args) == 0 + + items = [json.loads(p.read_text()) for p in (tmp_path / "out").glob("*.json")] + assert all(i["expected_output"]["visualization"]["type"] == "" for i in items) + + +# --- the phrasing step -------------------------------------------------------- + + +def _reply(text): + return SimpleNamespace(choices=[SimpleNamespace(message=SimpleNamespace(content=text))]) + + +def _fake_openai(*replies): + """An OpenAI stub returning `replies` in order, recording the prompts it received.""" + sent = [] + + class _Completions: + def create(self, model, messages): + sent.append(messages) + return replies[min(len(sent) - 1, len(replies) - 1)] + + class _Client: + chat = SimpleNamespace(completions=_Completions()) + + return _Client, sent + + +def _phrase(specs, *replies): + client_cls, sent = _fake_openai(*replies) + with ( + patch("openai.OpenAI", client_cls), + patch.dict("os.environ", {"OPENAI_API_KEY": "sk-test"}), + ): + return from_insights_mod.phrase(specs, "gpt-4o", DISPLAY), sent + + +def test_phrase_returns_a_question_the_spec_agrees_with(): + spec = convert(spend_by_merchant(), DATE_IDS) + (questions, sent) = _phrase([spec], _reply('"Show me Spend Amount by Merchant Name"')) + + assert questions == ["Show me Spend Amount by Merchant Name"], "surrounding quotes are stripped" + assert len(sent) == 1, "a clean question is not re-asked" + + +def test_phrase_feeds_a_contradiction_back_once_and_keeps_the_rewrite(): + spec = convert(spend_by_merchant(), DATE_IDS) + (questions, sent) = _phrase( + [spec], + _reply("Show me the top 5 Merchant Name by Spend Amount"), # ranking the spec lacks + _reply("Show me Spend Amount by Merchant Name"), + ) + + assert questions == ["Show me Spend Amount by Merchant Name"] + assert len(sent) == 2 + assert "ranking word" in sent[1][-1]["content"], "the specific contradiction is quoted back" + + +def test_phrase_drops_an_item_the_writer_keeps_contradicting(): + # Shipping a question its own expected_output disagrees with is worse than shipping + # fewer questions, so the second failure drops the item. + spec = convert(spend_by_merchant(), DATE_IDS) + (questions, sent) = _phrase([spec], _reply("Show me the top 5 Merchant Name by Spend Amount")) + + assert questions == [None] + assert len(sent) == 2, "one retry, then give up" + + +def test_phrase_treats_a_refusal_with_no_text_as_a_failed_attempt(): + # `message.content` is None for a refusal; .strip() on it used to crash the run. + spec = convert(spend_by_merchant(), DATE_IDS) + (questions, _) = _phrase([spec], _reply(None)) + assert questions == [None] + + +def test_phrase_requires_the_api_key_rather_than_failing_per_item(): + spec = convert(spend_by_merchant(), DATE_IDS) + client_cls, _ = _fake_openai(_reply("x")) + with ( + patch("openai.OpenAI", client_cls), + patch.dict("os.environ", {}, clear=True), + pytest.raises(OSError, match="OPENAI_API_KEY"), + ): + from_insights_mod.phrase([spec], "gpt-4o", DISPLAY) + + +def test_generate_uses_the_phrasing_step_when_it_is_not_disabled(tmp_path): + args = _args(tmp_path, snapshot_in=_snapshot_file(tmp_path, _snapshot(_bar("spend", "merchant.NAME")))) + args.no_phrase = False + client_cls, sent = _fake_openai(_reply("Show me Spend Amount by Merchant Name")) + + with patch("openai.OpenAI", client_cls), patch.dict("os.environ", {"OPENAI_API_KEY": "sk-test"}): + assert generate(args) == 0 + + assert sent, "the LLM was asked" + written = [json.loads(p.read_text()) for p in (tmp_path / "out").glob("*.json")] + assert written[0]["question"] == "Show me Spend Amount by Merchant Name"