Skip to content

Curated insights (1/5): chart-generator package + registry - #762

Open
01painadam wants to merge 2 commits into
mainfrom
feat/curated-insights-registry
Open

Curated insights (1/5): chart-generator package + registry#762
01painadam wants to merge 2 commits into
mainfrom
feat/curated-insights-registry

Conversation

@01painadam

Copy link
Copy Markdown
Collaborator

What & why

Deterministic "curated" insights (POST /api/analyze → no LLM) existed only for Tree Cover Loss and Integrated Alerts; every other dataset returned an empty insight. This is the groundwork to expand them to all 12 catalog datasets (the rest land in the branches stacked on top of this one).

src/api/services/charts.py (one file, 2 generators, linear can_handle scan) becomes src/api/services/charts/:

  • base.py — the ChartGenerator seam + column_to_rows.
  • one module per dataset (TCL + Integrated Alerts moved verbatim here).
  • registry.py — an explicit dataset_id → generator map that raises on duplicate registration, plus DATASETS_WITHOUT_CURATED_INSIGHTS, a coverage ratchet.

AnalyzeService now does a registry lookup instead of a first-can_handle-wins scan. The analyze router rejects unknown dataset_ids with a 422 instead of letting a doomed job fail downstream in the analytics handler.

Coverage ratchet: a unit test asserts every catalog dataset is either registered or in the explicit exclusion set — so a future catalog YAML added without a curated generator fails CI loudly. This PR leaves the 10 not-yet-implemented datasets in the exclusion set; later branches shrink it to empty.

Tests

  • tests/unit/api/services/charts/ (mirror-path): per-generator + registry ratchet/duplicate tests.
  • tests/api/test_analyze.py: auth + unknown-dataset 422.
  • Full unit tier green; pre-commit (ruff + mypy) clean.

Contract

No behaviour change to existing TCL/Integrated Alerts output. No AgentState / stream / FE-type changes.

🤖 Generated with Claude Code

01painadam and others added 2 commits July 22, 2026 16:07
src/api/services/charts.py becomes src/api/services/charts/ with one module
per dataset, a base module for the ChartGenerator seam, and an explicit
dataset-id -> generator registry that raises on duplicate registration.
AnalyzeService switches from a first-can_handle-wins scan to a registry
lookup. DATASETS_WITHOUT_CURATED_INSIGHTS is a coverage ratchet enforced by
a unit test: every catalog dataset needs a generator or a deliberate
exclusion entry. Tests move to the mirror-path tests/unit/api/services/charts/.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An unknown dataset_id previously produced a pending job that failed
downstream in the analytics handler; fail loudly at the API edge with a
422 instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@yellowcap

Copy link
Copy Markdown
Collaborator

One concern I have here is about the background tasks. The api response cycle kicks off a "Background task" using FastAPI's BackgroundTasks.add_task(). It runs after the HTTP response is already sent to the client. The request handler returns immediately — the client doesn't block waiting for analysis to finish.

This is not a huge risk right now because if an insight fails its not that bad. But it won't scale if we have a lot of users and a lot of containers. If a container is restarted, this is completely lost.

The cycle as in this PR implementation is

  POST /api/analyze                                                                                                                                                                                                                          
    │                                                                                                                                                                                                                                        
    ├─ 1. Validate dataset_id                                                                                                                                                                                                                
    ├─ 2. INSERT JobOrm (status=pending)                                                                                                                                                                                                     
    ├─ 3. Return 200 { status: "pending", id: "..." }  ← response sent here                                                                                                                                                                  
    │                                                                                                                                                                                                                                        
    └─ (after response is sent)                                                                                                                                                                                                              
        ├─ BackgroundTask starts                                                                                                                                                                                                             
        ├─ Pull data from analytics API                                                                                                                                                                                                      
        ├─ Generate charts                                                                                                                                                                                                                   
        ├─ Persist InsightOrm + InsightChartOrm                                                                                                                                                                                              
        ├─ INSERT JobResourceOrm linking job → insight                                                                                                                                                                                       
        └─ UPDATE JobOrm (status=completed)                                                                                                                                                                                                  

I would propsoe to make this api call wait for the analytics-api to finish and do the whole ORM cycle in one go.

@01painadam

Copy link
Copy Markdown
Collaborator Author

Good catch @yellowcap

The background task spread the cycle over four separate transactions with the response already gone, so a container restart mid-task stranded the job in pending/running (which the FE polls forever) or left an orphaned InsightOrm with no resource pointing at it.

Addressed in #779 (stacked on this branch, no overlap with 2/5–5/5), which does exactly what you propose. The request waits for the analytics API and the whole ORM cycle happens in one go:

POST /api/analyze
  ├─ 1. Validate dataset_id                    (422 if unknown — unchanged)
  ├─ 2. Pull analytics data + generate charts  (nothing persisted yet)
  ├─ 3. ONE transaction: JobOrm (already terminal) + InsightOrm
  │      + InsightChartOrm + JobResourceOrm
  └─ 4. Return 200 { status: "completed", resources: [...] }

Either the full cycle exists or nothing does — there's no intermediate state left to strand. A crash mid-analysis persists nothing and the client sees the error instead of a forever-pending job.

A few decisions worth you looking at on that branch:

  • Timeout: the analytics pull (its polling loop can run tens of seconds) is capped with asyncio.timeout(50s), intended to sit under the gateway idle timeout; hitting it records a failed job. If you know the actual LB idle timeout offhand, happy to tune the constant.
  • Upstream failure → 200 + status: "failed": it's a job outcome the schema and FE already model. A persistence failure propagates as a 500 instead — the transaction rolled back, so a retry starts clean.
  • Contract unchanged: same JobResponse shape, just terminal on arrival; GET /api/jobs/{id} still works, and the existing FE poll loop simply ends on its first iteration.

On the scaling point: synchronous execution fixes durability, not throughput (many users × slow pulls = many held connections). The durable answer is a queue (e.g. pg-backed jobs claimed by a worker), which I've left out of scope here; the JobResponse + GET /api/jobs/{id} contract is kept precisely so that can land later without another FE change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants