Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes FastAPI deployment issues by moving pytest import under conditional execution and adjusting production worker count. The changes enable proper language detection in search by adding i18n middleware and passing language context through the search pipeline.
Key Changes
- Moved pytest import to conditional block to avoid import errors in production
- Reduced production workers from 50 to 4 for better resource management
- Added i18n middleware to FastAPI for language detection via headers, cookies, and query parameters
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| openlibrary/asgi_app.py | Moved pytest import under CI check, added CORS and i18n middleware, changed health endpoint path |
| openlibrary/fastapi/search.py | Passed request language to search function |
| openlibrary/plugins/worksearch/code.py | Added lang parameter to async_work_search and passes it to WorkSearchScheme |
| openlibrary/plugins/worksearch/schemes/init.py | Added lang attribute and constructor to SearchScheme base class |
| openlibrary/plugins/worksearch/schemes/works.py | Changed from web.ctx.lang to self.lang for language detection |
| compose.production.yaml | Reduced Gunicorn workers from 50 to 4 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Lgtm! We tested on prod and it handled the traffic correctly. We fixed some errors related to web.ctx.lang, and tweaks the worker count because having 4 fast api workers with our usual 50 normal web works was causing the servers to be overloaded. We're still monitoring to see if during high traffic events our current setup might still struggle, but so far it's looking ok.
Note need to open a PR for haproxy changes.
We don't have pytest on prod so we need to move the import under the conditional.
Also I realize 50 workers is way too high. So set it to 4 for production. In theory each worker should be able to handle hundreds of requests.
This also adds support for i18n, which was breaking.
Technical
Testing
Screenshot
Stakeholders