Skip to content

TIG-254: Move selected tagger into backend code (corrected) - #34

Open
yubimamiya wants to merge 2 commits into
mainfrom
yubi_dev
Open

TIG-254: Move selected tagger into backend code (corrected)#34
yubimamiya wants to merge 2 commits into
mainfrom
yubi_dev

Conversation

@yubimamiya

Copy link
Copy Markdown

Remove all secrets
Update extraction pipeline in backend to use cosine similarity embedding-based event tagging approach
Update data tables with event tag embeddings and import machine learning model for event classification
Technical documentation: https://docs.google.com/document/d/1OGwtYvPLSNfOmgwFi0mSn0AWvuWeSvUu1-j1I0XKq00/edit?usp=sharing

@linear

linear Bot commented Jul 26, 2026

Copy link
Copy Markdown

TIG-254

@DIodide
DIodide self-requested a review July 26, 2026 01:36

@DIodide DIodide left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — the core approach is solid and the data work is genuinely clean: all 22 tag embeddings are present, correctly 1536-dim, and match the new enum exactly. seed.ts is fully consistent with the renamed enums too.

Holding off on approval though, because the enum rename isn't propagated to the frontend and that breaks the build. Details below.

Important

This branch's history was rewritten. A credential was committed in the first commit; I rebuilt both commits without it and force-pushed. The resulting tree is byte-identical to what you had (git diff between old and new head is empty) and your authorship is preserved — but please run git fetch origin && git reset --hard origin/yubi_dev before your next push, otherwise you'll restore the old history. The credential itself should be treated as burned and rotated regardless, since this repo is public.


Blocking

1. Breaks the TypeScript build

Typechecked both sides in clean worktrees:

tsc --noEmit in apps/web
main (c6f8a4c) 0 errors
this branch 2 errors
src/actions/users.ts(35,7):  error TS2769: No overload matches this call.
src/actions/users.ts(203,9): error TS2769: No overload matches this call.
  Type '"academic"' is not assignable to ... Did you mean '"academics"'?

2. The enum rename isn't propagated to apps/web

eventTagEnum and orgCategoryEnum were rewritten (free-foodfree food, academicacademics, culturalculture, art split into visual arts/performing arts, workshop/speaker dropped, plus 6 new values). 11 files still use the old literals and none are touched by this PR:

  • src/actions/users.ts
  • src/app/(app)/events/create/create-event-form.tsx
  • src/app/(app)/explore/explore-client.tsx
  • src/app/(app)/map/_lib/map-helpers.ts
  • src/app/(app)/orgs/create/page.tsx
  • src/app/(app)/orgs/orgs-client.tsx
  • src/app/(app)/settings/settings-client.tsx
  • src/app/(onboarding)/onboarding/page.tsx
  • src/components/events/event-card.tsx
  • src/components/events/event-cover-art.tsx
  • src/components/events/event-filters.tsx

Beyond the compile error, this breaks at runtime:

  • onboarding/page.tsx maps Research: "academic", Sustainability: "outdoor" and inserts them — Postgres will reject values no longer in the enum. (Worth noting the new enum has proper research and sustainability values now, so these mappings get simpler.)
  • Filter chips in event-filters.tsx / settings-client.tsx would match zero events.
  • Color and icon maps keyed on "free-food" etc. silently fall through, and the 6 new tags have no styling at all.

Suggestion: rather than re-listing the values by hand, derive them from the schema (typeof eventTagEnum.enumValues[number]) in users.ts so this can't drift silently again — the two hardcoded unions there are what actually broke.

3. Enum migration is destructive, with --force

db:push was changed to drizzle-kit push --force, which suppresses the data-loss confirmation — on a change that removes in-use enum values, and with no migration files in the repo (apps/database/drizzle/ doesn't exist). Existing event_tags.tag and organizations.category rows hold the old values.

This needs a real generated migration with an explicit USING cast that maps old → new values. Could we revert the --force here?


Important

4. Events lose date and location but still publish

The ML path sets datetime_str="" and location_name="". The orchestrator's fallback to the email's send time and the new needs_review status are a nice touch — but that flag only lands in pipeline_logs, and insert_event hardcodes is_public = true. So every ingested event goes live publicly with the email's send date as the event date and no location.

Was that the intent for this stage, or should needs_review events be held back (e.g. is_public = false) until someone confirms them?

5. _resolve_model_path is defined twice

extractor.py defines it at two places; the second silently overrides the first, so the cwd-anchoring comment above the first is dead code and the two docstrings contradict each other. The surviving parents[4] resolves to the repo root — fine in a checkout, but breaks if only backends/fastapi is copied into an image.

6. clean_body_text strips every hyphen

.replace("-", "") turns "3-5pm" into "35pm" and "e-mail" into "email". That corrupts both the embedded text and the user-visible description (which is cleaned_body[:2000]). Suggest dropping the hyphen from that replace.

7. assign_tags hits the DB per email

db.get_tag_embeddings() runs on every message — full table fetch plus a numpy rebuild each time. Worth caching the normalized matrix at module level.

8. assign_tags always assigns the top tag

The top-ranked tag is appended unconditionally, before the threshold loop — so an event gets a tag even at ~0 similarity. Intentional?

9. Blanket except Exception in extract_event

A missing/incompatible model file becomes a silent None per email, so a misdeployed model drops 100% of traffic with only log noise. Consider letting config-level errors (e.g. FileNotFoundError from _get_classifier) propagate.


Minor

  • scikit-learn is unpinned but has to stay compatible with the committed pickle (sklearn.linear_model._logistic) — sklearn pickles are version-sensitive, so please pin it.
  • pandas is added to requirements.txt but isn't imported anywhere in backends/.
  • Leftover markers: // YUBI has modified event tags, // YUBI ADD FUNCTION TO GET EMBEDDINGS..., and a comment duplicated verbatim in seed.ts (// 2. Only declare tagEmbeddingData once...).
  • Missing trailing newlines in extractor.py, db.py, orchestrator.py.
  • seed.ts imports from both ./schema and ./schema/index — worth merging.

Happy to take the frontend enum alignment off your plate as a separate PR if that's easier — just let me know. The main things I'd want your call on are #3 (migration strategy) and #4 (whether unreviewed events should publish).

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