diff --git a/api/analyses/drill_down/types.py b/api/analyses/drill_down/types.py index ae03c98..f759735 100644 --- a/api/analyses/drill_down/types.py +++ b/api/analyses/drill_down/types.py @@ -2,9 +2,12 @@ from __future__ import annotations +import logging from dataclasses import dataclass, field from typing import Any +logger = logging.getLogger("nvi.analyses.drill_down.types") + DEFAULT_DIMENSIONS: list[str] = ["A2", "A3", "A11", "A12", "A13", "A14"] DEFAULT_METRIC: str = "loan_default_rate" DEFAULT_MAX_ITERS: int = 3 @@ -43,14 +46,49 @@ class DrillDownArgs: @classmethod def from_raw(cls, raw: dict[str, Any], default_question: str) -> "DrillDownArgs": + raw_dims = raw.get("dimensions") or list(DEFAULT_DIMENSIONS) return cls( question=raw.get("question") or default_question, metric=raw.get("metric") or DEFAULT_METRIC, - dimensions=raw.get("dimensions") or list(DEFAULT_DIMENSIONS), + dimensions=_filter_to_valid_columns(raw_dims), max_iters=int(raw.get("max_iters") or DEFAULT_MAX_ITERS), ) +def _filter_to_valid_columns(dims: list[str]) -> list[str]: + """Drop any candidate dimension that isn't a real column ref. The + planner sometimes conflates metric names (e.g. 'unemployment_rate_96') + with column names — they look the same shape but only one can be + grouped by. We drop the bad ones here so `decide_next` never picks + them and the composer never sees them. Falls back to DEFAULT_DIMENSIONS + if everything got filtered out (better than running with no candidates).""" + # Import lazily — recon access from a types module would create a cycle + # if recon is being rebuilt; deferring it sidesteps that. + from api.recon import load_recon + + recon = load_recon() + kept: list[str] = [] + dropped: list[tuple[str, str]] = [] + for d in dims: + try: + recon.resolve_column(d) + kept.append(d) + except ValueError as e: + dropped.append((d, str(e))) + if dropped: + logger.warning( + "drill_down dimensions filtered (%d kept, %d dropped): %s", + len(kept), len(dropped), + ", ".join(f"{d!r} ({reason})" for d, reason in dropped), + ) + if not kept: + logger.warning( + "all proposed dimensions invalid; falling back to DEFAULT_DIMENSIONS" + ) + return list(DEFAULT_DIMENSIONS) + return kept + + @dataclass class Slice: """One iteration's slice: the chosen dimension, the SQL it produced, diff --git a/api/prompts/planner.system.txt b/api/prompts/planner.system.txt index 7aa734b..fa2ea1e 100644 --- a/api/prompts/planner.system.txt +++ b/api/prompts/planner.system.txt @@ -25,3 +25,8 @@ Rules: - Most questions are single-step. Only chain analyses when the second genuinely needs the first's output. - Comparing two periods is ONE `compare_periods` step, not two `direct_answer` steps. - A step's `fallback` is optional. Include one when the primary analysis is speculative (e.g. an exploratory `drill_down` that might find nothing useful) and there's a safer Analysis that can still produce an answer. The fallback runs only if the primary errors or yields no finding. Fallbacks must not themselves have fallbacks. + +Distinguishing metrics from dimensions: +- A `metric` is a name from the metric catalog above — what gets aggregated. Example: `loan_default_rate`. +- A `dimension` (in drill_down args) is a raw COLUMN name from a warehouse table — what you group by. Example: `A2` (district name), `status` (loan status), `frequency` (statement frequency). NEVER put a metric name in the dimensions list. +- If unsure whether a name is a metric or a column, check the metric catalog: anything listed there is a metric, not a dimension.