-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
POC for FastAPI on the search.json endpoint #11249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
05b0121 to
5ff5280
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This POC introduces a minimal FastAPI-based search endpoint and supporting async Solr plumbing to evaluate performance.
- Add an ASGI app with a FastAPI route for /search.json, backed by new async search functions.
- Switch Solr HTTP calls to httpx with an async bridge to allow sync and async callers.
- Add docker-compose services and a startup script for the new FastAPI app.
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| openlibrary/utils/solr.py | Convert raw_request to async using httpx and adapt select to use async bridge; minor escaping cleanup. |
| openlibrary/utils/async_utils.py | Introduce AsyncBridge running a background event loop to call async from sync code. |
| openlibrary/plugins/worksearch/subjects.py | Type casts for facet iteration to match expected runtime structure. |
| openlibrary/plugins/worksearch/code.py | Split query prep/processing, add async variants, switch Solr calls to httpx, tweak types. |
| openlibrary/fastapi/search.py | New FastAPI router exposing /search.json, calling async_work_search. |
| openlibrary/asgi_app.py | New ASGI app bootstrapping legacy config side-effects and mounting FastAPI routes. |
| docker/ol-web-fastapi-start.sh | Entrypoint script to run Gunicorn/Uvicorn serving the ASGI app. |
| compose.yaml | Add fast_web service for the FastAPI app in local compose. |
| compose.staging.yaml | Add fast_web service for staging. |
| compose.production.yaml | Add fast_web profiles for production deployment. |
| compose.override.yaml | Build-time override for fast_web during local dev. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
cdrini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woop woop I think we're at a spot where we can test this on prod! Some of the solr wrapping methods have exploded in number, but they were even before hand messy. We'll need to sort that out at some point 😬 Otherwise looking great!
| # facet: bool = Query(False, description="Whether to return facets."), | ||
| spellcheck_count: int | None = Query( | ||
| 3, description="The number of spellcheck suggestions." | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this endpoint also supports other arguments, eg title, author etc. We might need some sort of kwargs style thing to get all the url parameters and forward it along.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openlibrary/openlibrary/plugins/worksearch/schemes/works.py
Lines 45 to 106 in a556ee6
| all_fields = frozenset( | |
| { | |
| "key", | |
| "redirects", | |
| "title", | |
| "subtitle", | |
| "alternative_title", | |
| "alternative_subtitle", | |
| "cover_i", | |
| "ebook_access", | |
| "ebook_provider", | |
| "edition_count", | |
| "edition_key", | |
| "format", | |
| "by_statement", | |
| "publish_date", | |
| "lccn", | |
| "lexile", | |
| "ia", | |
| "oclc", | |
| "isbn", | |
| "contributor", | |
| "publish_place", | |
| "publisher", | |
| "first_sentence", | |
| "author_key", | |
| "author_name", | |
| "author_alternative_name", | |
| "subject", | |
| "person", | |
| "place", | |
| "time", | |
| "has_fulltext", | |
| "title_suggest", | |
| "publish_year", | |
| "language", | |
| "number_of_pages_median", | |
| "ia_count", | |
| "publisher_facet", | |
| "author_facet", | |
| "first_publish_year", | |
| "ratings_count", | |
| "readinglog_count", | |
| "want_to_read_count", | |
| "currently_reading_count", | |
| "already_read_count", | |
| # Subjects | |
| "subject_key", | |
| "person_key", | |
| "place_key", | |
| "time_key", | |
| # Classifications | |
| "lcc", | |
| "ddc", | |
| "lcc_sort", | |
| "ddc_sort", | |
| "osp_count", | |
| # Trending | |
| "trending_score_hourly_sum", | |
| "trending_z_score", | |
| } | |
| ) |
Co-authored-by: Drini Cami <[email protected]>
|
Ok! This looks good for testing! Merging up now. |
Building on (and simplifying) #11181
Creates a very minimal search endpoint we can use to test performance of FastAPI for our use case.
We chose this endpoint because it was relatively simple to isolate and is the 2nd highest traffic endpoint. Partials is the highest traffic endpoint but is much more complicated and touches more code.
This was a pretty big change going through several iterations and lots of discussion over video calls so we don't have it well documented here.
Technical
Testing
Screenshot
Stakeholders