Skip to content

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

Merged
DIodide merged 8 commits into
stagingfrom
yubi_dev
Aug 23, 2026
Merged

TIG-254: Move selected tagger into backend code (corrected)#34
DIodide merged 8 commits into
stagingfrom
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).

@yubimamiya

yubimamiya commented Aug 3, 2026

Copy link
Copy Markdown
Author

I have reverted the --force and instead updated the database (enums and tables) with explicit USING cast. I ran the migration successfully on my local container of the database. My apps/database/drizzle/0002_update_enums.sql file for the migration is hidden from the GitHub repo because it is in .gitignore. Please let me know if you would like me to publish this file to the GitHub repo for reference of the USING cast. I will take a look at review comment #4 about the unreviewed events next.

@DIodide

DIodide commented Aug 3, 2026

Copy link
Copy Markdown
Member

Thanks @yubimamiya — glad the USING-cast migration ran cleanly locally, and good to see the staging merge absorbed the history rewrite without issues.

Yes, please commit the migration file. A migration only does its job if every environment runs the same SQL — kept local-only, staging/prod would still have to improvise the enum conversion by hand. The reason it's hidden is a pre-existing drizzle/ entry in the root .gitignore (line 24), which contradicts the repo's documented db:generate/db:migrate workflow — please remove that line as part of this PR, then commit the whole apps/database/drizzle/ folder: 0002_update_enums.sql, any earlier migrations that exist only on your machine (0000/0001), and the meta/ journal files drizzle-kit generated. The journal has to stay consistent with the SQL files, so commit the folder as a unit.

Heads-up: the changes you described aren't on the pushed branch yet. At the current head (321b604):

  • apps/database/package.json still has "db:push": "drizzle-kit push --force" — the revert must still be local.
  • apps/web still fails tsc --noEmit with the same 2 errors in src/actions/users.ts (35,7 / 203,9), and the 11 frontend files from the review still use the old enum literals.

So the remaining checklist to get this over the line:

  1. Push the --force revert + un-gitignore and commit the migration folder
  2. Frontend enum alignment (review items 1–2) — the offer stands to take this as a separate PR onto your branch if you'd rather focus on the pipeline; just say the word
  3. Item 4 (needs_review events publishing with is_public = true) — which you're already on

Once those land I'll re-run the typecheck and flip the review.

@yubimamiya

Copy link
Copy Markdown
Author

Hi! My bad, I missed that --force in that drizzle-kit push. I have updated the package.json file and added all of the sql migration files. I re-ran the migration, so 004_update_database.sql is the most updated file. Let me know if you have any other questions!

@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.

Re-reviewed at b2ec222. Good progress on the migration front — --force is reverted, the full drizzle/ folder + meta journal is committed, and the event_tag USING-cast mapping is complete and correct (I checked all 18 old values). Not there yet though; here's what's still open, plus two new issues in the committed migrations.

New issues in the migration files

1. org_category mapping is incomplete — the cast will fail on real data

In 0004_update_database.sql (and its duplicate 0002), the organizations.category CASE maps only academic, performance, cultural, athletic. Four other renamed values fall through the ELSE unchanged and don't exist in the new enum, so the cast throws:

old value falls through as in new enum?
social social ❌ (new is social event)
religious religious ❌ (new is religion)
political political ❌ (new is politics)
service service ❌ (new is community service)

Any DB with an org in one of those categories fails the migration mid-chain. It likely passed locally because your orgs happened to be in the mapped categories (or the table was freshly seeded with new values).

2. The chain commits the trial-and-error history, and 0003 can't replay over data

The journal is: 0002 (old→new), 0003 (revert to old enums), 0004 (old→new again, byte-identical to 0002). Two problems:

  • 0003 casts columns back to the old enum with a direct USING "tag"::event_tag — but after 0002, rows hold new values like 'free food', which the old enum doesn't contain. On any database with rows in event_tags/user_interests/organizations, 0003 throws and the chain wedges halfway.
  • 0002 and 0004 being identical means fresh environments run the same conversion twice with a full revert in between — harmless only when tables are empty.

Suggested fix for both: squash to a single clean migration — keep one copy of the conversion SQL (with the four missing org_category mappings added) as 0002, delete 0003/0004 and their snapshots, and regenerate meta/_journal.json so the chain is 0000 → 0001 → 0002. Then the replay is deterministic on empty and populated databases.

Still open from the previous review

  • Build is still broken — re-ran tsc --noEmit on apps/web at b2ec222: same 2 errors in src/actions/users.ts (35,7 / 203,9). The 9 frontend files listed in the original review still use the old enum literals. This is the main blocker.
  • Item 4insert_event still hardcodes is_public = true, so needs_review events (fallback date, no location) go straight to the public feed. You mentioned you were looking at this one.
  • Smaller items from before, all unchanged: duplicate _resolve_model_path in extractor.py, hyphen-stripping in clean_body_text ("3-5pm""35pm"), unpinned scikit-learn against the committed pickle, unused pandas in requirements.

The offer still stands to take the frontend enum alignment as a PR onto your branch — it's mechanical but touches a lot of files, and it's the last big blocker. Just say the word.

@DIodide

DIodide commented Aug 23, 2026

Copy link
Copy Markdown
Member

@yubimamiya I've pushed the remaining fixes directly to this branch so we can get it over the line — please review the three new commits when you get a chance and flag anything you'd do differently:

  • a9df97b — frontend enum alignment. All 11 apps/web files now use the new event_tag/org_category values (filters, settings, onboarding mapping, create-event form, org pages, color/palette maps — the 6 brand-new tags got colors and palettes too). users.ts now derives its types from the schema (typeof eventTagEnum.enumValues) so the two can't drift again. tsc --noEmit passes in apps/web.

  • 7fc5ee6 — migration squash. Kept your USING-cast conversion as a single 0002, removed the 0003/0004 revert-and-reapply pair (their snapshots were identical to 0002's), and regenerated the journal. Two data-level fixes on top:

    1. Added the four missing org_category mappings (social, religious, political, service) — they fell through the ELSE into values the new enum doesn't have.
    2. Deduped the workshop+academicacademics merge before the cast — an event (or user) holding both old tags produced duplicate composite-PK rows when the index rebuilt.

    Verified by replaying 0000→0001→0002 in one transaction on a scratch pgvector container seeded with every old enum value, including the collision pair: applies cleanly, 18 tags → 17 (merge deduped), all 10 org categories convert, needs_review lands in pipeline_log_status.

    Also fixed seed.ts, which lost its sql import in the staging-merge conflict resolution (it wouldn't have run), and gave apps/database the Bun types its Bun.file usage needs — tsc passes there now too.

  • 409e912 — pipeline. needs_review events now insert with is_public = false so fallback-dated, location-less events stay out of the public feed until someone confirms them (resolving review item 4 — shout if you intended otherwise). Removed the duplicate _resolve_model_path (kept your repo-root version, folded both docstrings), stopped clean_body_text stripping hyphens ("3-5pm""35pm"), cached the tag-embedding matrix instead of refetching per email, let FileNotFoundError propagate out of extract_event, pinned scikit-learn==1.6.1 (the version your model was pickled with — re-export before bumping), and dropped unused pandas.

One deploy note for whoever runs this against an existing local DB: the compose file now uses pgvector/pgvector:pg17, so older local containers created from postgres:* images need a docker compose up -d to pick up the new image (the data volume carries over) before the migration can CREATE EXTENSION vector.

Everything green on my end: web + database typecheck, biome, py_compile, and the full migration replay above. @yubimamiya once you've looked it over and you're happy, I think this is ready.

@DIodide
DIodide dismissed their stale review August 23, 2026 15:15

All requested changes have been addressed in a9df97b / 7fc5ee6 / 409e912 (pushed by reviewer); awaiting author sign-off on those commits.

@DIodide
DIodide changed the base branch from main to staging August 23, 2026 20:08
@DIodide
DIodide merged commit cb12a8a into staging Aug 23, 2026
1 check failed
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.

3 participants