Skip to content

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

Open
yubimamiya wants to merge 4 commits into
mainfrom
yubi_dev
Open

TIG-254: Move selected tagger into backend code (corrected)#34
yubimamiya wants to merge 4 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).

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

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