docs(feedback): spec set for Feedback Aggregation & Clustering System#2422
Open
blarghmatey wants to merge 7 commits into
Open
docs(feedback): spec set for Feedback Aggregation & Clustering System#2422blarghmatey wants to merge 7 commits into
blarghmatey wants to merge 7 commits into
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a build-oriented spec set for the “Feedback Aggregation & Clustering System”, defining the event contract, the tfact_feedback dimensional model, an MVP Zendesk ingestion plan, an ML clustering/sentiment approach, a Dagster asset shape, and Superset-first consumption/UX guidance.
Changes:
- Added a spec index and phased roadmap for the feedback aggregation initiative.
- Defined a common feedback event contract + migration-proof business keys and a
tfact_feedbackKimball model. - Described MVP Zendesk dbt model DAG plus an additive Dagster ML asset plan and consumption surfaces.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/design/README_feedback_aggregation.md | Indexes the spec set, key decisions, and phasing for implementation. |
| docs/design/feedback_dimensional_model.md | Defines the tfact_feedback dimensional model and related conformed/new dimensions. |
| docs/design/feedback_event_contract_spec.md | Specifies the source-agnostic event contract and stable business-key strategy. |
| docs/design/feedback_zendesk_mvp_spec.md | Provides a Zendesk-only MVP dbt model plan grounded in repo conventions/macros. |
| docs/design/feedback_ml_approach.md | Specifies embedding, clustering, category discovery, and sentiment approach as additive consumers. |
| docs/design/feedback_dagster_asset_spec.md | Specifies a Dagster ML code location/asset graph to generate embeddings/clusters/labels. |
| docs/design/feedback_consumption_ux_spec.md | Specifies Superset-first consumption, personas, and access-control expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+164
to
+168
| `feedback_pk = generate_surrogate_key([feedback_source, source_natural_key])` where `source_natural_key` is a | ||
| **stable business identifier from the source system**, never a warehouse/Airbyte/Postgres row PK: | ||
|
|
||
| | Source | source_natural_key | | ||
| |--------|--------------------| |
Member
Author
There was a problem hiding this comment.
Fixed in affb4d7. Aligned §2 and §6 of this doc to the event-contract terminology: feedback_source/source_natural_key → source_slug/source_record_ref. This doc was recovered from an earlier draft that predated the contract naming.
Comment on lines
+200
to
+201
| Clustering/labeling jobs (Dagster asset) read `int__feedback__unioned` (redacted) → write `feedback_embeddings` | ||
| + `dim_feedback_category` + `category_fk`/`sentiment_fk` back onto the fact (late-arriving update). |
Member
Author
There was a problem hiding this comment.
Fixed in affb4d7. Removed the stray leading + (diff artifact) and reflowed the build-path sentence.
Advance the Feedback Aggregation & Clustering System project through its spec phase. The dimensional-model design (previously drafted but never committed) is recovered and joined by concrete, repo-grounded specs that resolve the open items it handed to downstream tasks. Why: the RFC (mitodl/hq#12210) settled the two architecture decisions (contract-first interim landing; one common tfact_feedback). This turns that direction into build-ready specs so implementation can start without re-deriving source columns, dbt conventions, or the ML/orchestration shape. Adds under docs/design/: - README_feedback_aggregation.md — spec index + key decisions + phasing - feedback_dimensional_model.md — recovered schema design (tfact_feedback) - feedback_event_contract_spec.md — common event contract + business keys - feedback_zendesk_mvp_spec.md — build-ready Zendesk MVP dbt models/columns - feedback_ml_approach.md — embedding/clustering/category/sentiment - feedback_dagster_asset_spec.md — batch ML asset (clones student_risk_probability) - feedback_consumption_ux_spec.md — audiences, Superset surfaces, access control Design/tracking only; no pipeline code. MVP ships the fact first; ML is an additive consumer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RJXxq9oEabCbJiXFzSTqkY
Evaluate in-engine AI functions and a DataFrame AI framework against the original local-torch embedding plan. Decisive repo fact: production runs Trino on Starburst Galaxy (StarRocks is not deployed), whose in-SQL AI functions (generate_embedding → Iceberg ARRAY(DOUBLE), public preview) can embed inside the engine we already run, on AWS Bedrock in-account. New default: Starburst AI generate_embedding as a trino_only dbt model — eliminates the net-new torch/sentence-transformers service and removes the third-party-egress objection (Bedrock-in-account over redacted text). Only clustering + LLM cluster-labeling remain a (now lightweight) Python asset, since neither Trino nor DuckDB does density clustering and we want HDBSCAN's noise class. Fenic and local self-hosted embeddings kept as fallbacks; StarRocks vector index folded into the Phase-3 data-bus note. Adds adr_embedding_compute_strategy.md; revises ml_approach §B, dagster_asset_spec header, and the README index/decisions accordingly. Schema, contract, and business keys unchanged (compute location was always meant to be swappable — vectors in a sidecar, category/sentiment late-arriving). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RJXxq9oEabCbJiXFzSTqkY
…arRocks) Strategic direction is to retire Trino for StarRocks (transforms + data bus + OLAP serving), so no Galaxy-only functionality may sit on the critical path. This withdraws rev.1's Starburst generate_embedding recommendation: it is a Galaxy-proprietary SQL function in exactly the layer being migrated, and StarRocks has no embedding-generation equivalent to migrate it to. New decision: keep all AI/embedding compute engine-external (Dagster/Python) reading/writing open Iceberg, and generate embeddings by calling AWS Bedrock (amazon.titan-embed-text-v2:0) DIRECTLY via boto3 — same in-account/PII-safe Bedrock substrate Starburst's function merely wraps, minus the lock-in and minus torch. Vectors land in an Iceberg ARRAY<float> sidecar that StarRocks later reads to build an HNSW index (a load, not a re-embed), making StarRocks ANN the intended serving tier. Sentiment stays the engine-agnostic CSAT-seed + embedding-kNN path (no analyze_sentiment); clustering stays Python HDBSCAN. dbt models stay portable (no trino_only AI models). Fenic (cloud-egress, K-means) and local sentence-transformers kept as fallbacks. Schema/contract/business-keys unchanged — compute location was always meant to be swappable.
…mework Verified against fenic source (main): Fenic is Apache-2.0 and its shipped embedding providers are OpenAI/Cohere/Google (Developer+Vertex) — genuinely open source and provider-agnostic, so it's a first-class fit for the portable engine-external layer (not the downgraded footnote rev.2 made it). Correction to the record: there is NO BedrockEmbeddingModel config class yet — "Bedrock" in config.py is only an OpenRouter LLM-routing target (not embeddings), and ROADMAP lists Bedrock as planned. So native in-account Bedrock embeddings via Fenic are roadmap-not-shipped. Decision: use Fenic as the engine-external framework (embed + classify/ sentiment + labeling; batching/caching/lineage free), with the embedding PROVIDER as a PII-policy choice decoupled from the framework — in-account Bedrock (via boto3 today, or contribute the provider; Apache-2.0 allows it) preferred, or a Fenic-native managed provider over redacted text if policy allows. Clustering stays our own sklearn UMAP+HDBSCAN (Fenic is K-means only, no noise class). Vectors → open Iceberg ARRAY<float>; StarRocks HNSW is the intended serving tier (load, not re-embed). No engine-native AI SQL functions; no trino_only AI models. Schema/contract/business-keys unchanged.
…rness) Owner direction: Bedrock/in-account is not a requirement — choose the embedding model on task effectiveness (clustering + retrieval on our own feedback corpus). Egress of Presidio-redacted text to a managed provider is acceptable; redaction stays as data-minimization regardless. Adds §B.1: an effectiveness-driven selection method — use MTEB to narrow, then benchmark the shortlist on a labeled Zendesk sample by clustering agreement (ARI/NMI vs. ticket_tags) + silhouette + human coherence, sweeping Matryoshka dims and cost. Candidate shortlist: Fenic-native managed (gemini-embedding-001, Cohere embed-v4, OpenAI text-embedding-3-large) and self-hosted open (Qwen3-Embedding, BGE-M3) if we host for effectiveness. No hardcoded winner; choice reversible via model_version. Also upgrades LLM semantic-normalization-before-embedding from "rejected default" to a first-class eval arm in §C — 2026 support-ticket-clustering evidence reports it is the single largest lever on cluster quality for short/noisy text; gate adoption by measured lift vs. per-record LLM cost. Syncs ADR + dagster_asset_spec + README notes to rev.4.
- feedback_zendesk_mvp_spec §8: compound uniqueness test referenced source_slug, which is not a fact column; use ['feedback_source_fk', 'source_record_id'] (both exist; same business grain since feedback_source_fk = surrogate_key([source_slug])). Drop the stray "source_slug equivalent" from the not_null list. (sentry) - feedback_dimensional_model §2/§6: align business-key terminology to the rest of the spec set — feedback_source/source_natural_key → source_slug/source_record_ref. (Copilot) - feedback_dimensional_model §8: remove stray leading "+" diff artifact that rendered oddly in the build-path prose. (Copilot) - feedback_dagster_asset_spec §5: type the PrivateAttr in the LLMClientFactory example (_client: Anthropic | None) per Pydantic v2 + repo convention, so the client-caching pattern is correct. (sentry)
The consumption surface was written Superset-first; broaden it to a real per-audience choice, since the model is surface-agnostic (every surface just reads the modeled tables). Add Marimo notebook-as-webapp as the interactive/ curation/prototyping surface and net-new app as the write-back/embedding option, each grounded in tooling the platform already operates: - Superset is managed as code in src/ol_superset/ with programmatic RLS (apply_rls.py) — lowest-effort MVP dashboards. - images/marimo-jupyterlab/ already runs marimo with Keycloak OIDC + per-user Trino token + IRSA, so a deployed notebook-as-webapp reuses that plumbing and can push row-level authz to the engine (Lakekeeper/Cedar) instead of the app. Recommend phased/audience-matched: Superset for MVP trend/cluster dashboards, a Marimo webapp for cluster exploration + the category-curation loop and to prototype the UX, net-new app only when write-back/product-embedding justifies it. Choice stays reversible. Updates §2/§3/§4/§5 + README.
blarghmatey
force-pushed
the
worktree-feedback-aggregation-spec
branch
from
July 11, 2026 00:55
cb23bf4 to
9b0a34f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Advances the Feedback Aggregation & Clustering System through its spec phase. The architecture RFC (mitodl/hq#12210, supersedes #10793) settled the two decisions — contract-first interim landing and one common
tfact_feedback. This PR turns that direction into build-ready, repo-grounded specs so implementation can start without re-deriving source columns, dbt conventions, or the ML/orchestration shape.The dimensional-model design existed only as an uncommitted draft in an unrelated worktree; it is recovered here and joined by five new specs.
Contents (
docs/design/)README_feedback_aggregation.mdfeedback_dimensional_model.mdtfact_feedbackschema (recovered)feedback_event_contract_spec.mdfeedback_zendesk_mvp_spec.mdfeedback_ml_approach.mdfeedback_dagster_asset_spec.mdstudent_risk_probability; Vault LLM resourcefeedback_consumption_ux_spec.mdKey decisions
tfact_feedbackmirroringtfact_discussion_events, with sparse nullable conformed FKs; explicit stablefeedback_pk(diverges from precedent) for migration + late-arriving category/sentiment.Scope
Design/tracking only — no pipeline code. MVP ships the fact first; the ML Dagster asset is a separate, additive follow-up.
🤖 Generated with Claude Code