Skip to content

replaces on_event with lifespan event handlers#87

Open
anthonyduong9 wants to merge 4 commits intomainfrom
replace-on_event-with-lifespan-event-handlers
Open

replaces on_event with lifespan event handlers#87
anthonyduong9 wants to merge 4 commits intomainfrom
replace-on_event-with-lifespan-event-handlers

Conversation

@anthonyduong9
Copy link
Contributor

@anthonyduong9 anthonyduong9 commented Apr 28, 2025

Problem

  • When inference tests are run, the DeprecationWarning
    on_event is deprecated, use lifespan event handlers instead. shows up in the logs.
  • In autointerp, we suppress the error with # type: ignore[deprecated].

Fix

  • Removing on_event functions.
  • Creating lifespan() functions and passing them to FastAPI.lifespan.

Testing

  • From apps/inference, I ran poetry run python start.py, and made several requests to POST /tokenize.
  • From apps/autointerp, I ran poetry run uvicorn server:app --host 0.0.0.0 --port 5003 --workers 1 and made requests to each endpoint besides for POST /score/embedding.

Fixes #70

@codecov
Copy link

codecov bot commented Apr 28, 2025

Codecov Report

❌ Patch coverage is 65.11628% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 7.92%. Comparing base (e8ddd08) to head (9cd85c0).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
apps/inference/neuronpedia_inference/server.py 71.42% 8 Missing ⚠️
apps/autointerp/neuronpedia_autointerp/logging.py 0.00% 7 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main     #87      +/-   ##
========================================
+ Coverage   7.86%   7.92%   +0.06%     
========================================
  Files        124     125       +1     
  Lines      17470   17505      +35     
  Branches     381     381              
========================================
+ Hits        1374    1388      +14     
- Misses     16085   16106      +21     
  Partials      11      11              
Flag Coverage Δ
autointerp 80.00% <0.00%> (-20.00%) ⬇️
inference 36.79% <77.77%> (+0.01%) ⬆️
webapp 5.18% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@anthonyduong9 anthonyduong9 force-pushed the replace-on_event-with-lifespan-event-handlers branch 3 times, most recently from c7a29ea to 688e29b Compare May 3, 2025 05:20
@anthonyduong9 anthonyduong9 marked this pull request as ready for review May 3, 2025 07:15
@anthonyduong9 anthonyduong9 force-pushed the replace-on_event-with-lifespan-event-handlers branch from 688e29b to 127cc59 Compare May 7, 2025 01:23
@hijohnnylin
Copy link
Owner

Sorry for the delay in getting around to this.

The cost-benefit doesn't work out currnetly for this change: it eliminates a deprecation warning in the code, but it will be a heavy lift to test this in production (Kubernetes/GoogleCloud is slow, plus resolving issues could take a significant amount of time debugging).

@anthonyduong9 anthonyduong9 force-pushed the replace-on_event-with-lifespan-event-handlers branch from 127cc59 to cccbc17 Compare August 14, 2025 23:46
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.

[inference][autointerp] Update fastAPI startup to use lifespan instead of on_event

2 participants