feat(db): improve GET /annotate/all performance#1
Closed
labradorite-dev wants to merge 17 commits intodevfrom
Closed
feat(db): improve GET /annotate/all performance#1labradorite-dev wants to merge 17 commits intodevfrom
GET /annotate/all performance#1labradorite-dev wants to merge 17 commits intodevfrom
Conversation
…-Data-Accessibility-Project#566) - Add pytest-benchmark~=4.0 dev dependency - Add ContextVar-based timing collector (timing.py) for zero-cost production instrumentation via _phase() context managers - Instrument extract_and_format_get_annotation_result() with per-phase timing: format_s, agency_suggestions_s, location_suggestions_s, name_suggestions_s, batch_info_s - Instrument GetNextURLForAllAnnotationQueryBuilder.run() with main_query_s timing - Add benchmark test suite under tests/automated/integration/benchmark/ with HTTP round-trip and per-phase breakdown tests - Add GHA workflow (.github/workflows/benchmark.yml) that runs on workflow_dispatch or PR to dev, uploads JSON artifact per commit SHA - Add README with Mermaid sequence diagram of the measured call chain
…olice-Data-Accessibility-Project#566) Add scale_seed.py with create_scale_data() that seeds 10k URLs plus geographic hierarchy, agencies, name/agency/location suggestions to exercise the query planner under realistic load. Add scale_seeder fixture (module-scoped) and two new benchmark tests that mirror the existing small-data pair, printing per-phase averages for direct comparison.
…olice-Data-Accessibility-Project#566) The sequential agency_auto_suggestions / add_location_suggestion loops (4 DB calls × 5k iterations each) blew through the 300s pytest timeout during fixture setup. Replace with 3 round-trips per suggestion type: initiate_task → bulk_insert subtasks (return_ids) → bulk_insert suggestions.
…ata-Accessibility-Project#566) SuggestionModel.robo_confidence is typed int; Pydantic rejects float values with fractional parts (e.g. 0.9). Use 1.0 which coerces cleanly.
…ags (Police-Data-Accessibility-Project#566) Convert both annotation views from regular views to MATERIALIZED VIEWs with unique indexes on url_id. Add CONCURRENTLY refresh calls to refresh_materialized_views(). Drops main_query_s from ~177ms to ~0.64ms at 10k-URL scale (~276x improvement).
…test (Police-Data-Accessibility-Project#566) With materialized views, URLs added between refreshes have no row in the view. Inner joins excluded them entirely, making new URLs invisible to annotators. Switch to LEFT OUTER JOINs so URLs without a view row still appear with NULL/0 counts. The sort test explicitly verifies annotation-count-driven ordering, so it needs a manual refresh_materialized_views() call after data setup to reflect counts before querying.
…ect#566) - Migration: Union[str, None] -> Optional[str], add docstrings to upgrade/downgrade - async_.py: add missing newline at end of file
… heads (Police-Data-Accessibility-Project#566) Upstream dev merged 1fb2286a016c (add_internet_archive_to_batch_strategy) which also chains from 759ce7d0772b, creating two Alembic heads. Update our migration's down_revision to sit after upstream's.
fa0df7f to
c3d6469
Compare
GET /annotate/all performance
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improves
GET /annotate/allperformance.Background Context
Issue _____ describes improving performance of an endpoint. However, before improving performance I first wanted to measure current performance to drive the decision with data. As such I established a benchmark using
pytest-benchmark. To back the benchmark with scale, it seeds 10k URLs to populate some relatively realistic annotation data. The most recent benchmark can be seen in the new GHA I added (see in the PR checks).The benchmark determined that
main_query_swas by far the largest portion of wall-clock time in this endpoint and so I focused my efforts on improving that performance. I considered a number of different solutions but by far the most effective is to convert theurl_annotation_count_viewandurl_annotation_flagsfrom regular PostgreSQL views to materialized views with unique indexes onurl_id.Performance Improvement
main_query_s@ 10k URLsFrom what I can tell, both views are used solely as internal query infrastructure to determine which URL gets served to an annotator next, via sorting. This does come at a cost though, which is a tradeoff for staleness.
Staleness Tradeoff
I added the materialized views to the existing
RefreshMaterializedViewsOperatorschedule which is once per day. In the worst case, URL ranking could be up to 24 hours stale — e.g. a URL that just received its 10th annotation may still be ranked highly when it should be deprioritized. My personal viewpoint is that this is acceptable because:If fresher data is a product priority, I'd recommend a more aggressive refresh rate (such as hourly) for the materialized view.
Alternative Long Term Solution
I would be remiss to not include the other, much larger refactor I considered which is to implement proper CQRS. The core of the issue here is that data is prioritized for writes, NOT reads. Then, at query time we pay the price and have to stitch all the data together. Materialized views shift that data stitching slightly left (once a day in a separate job), but a potentially more "complete" fix would be to add a new, fully denormalized table like this:
Then at each time we write a new annotation to be consumed by annotators, we also populate this table correctly. Then, at query time we have a primary key lookup (extremely cheap):
SELECT * FROM annotation_queue ORDER BY priority_rank LIMIT 1.If we are interested, I do think this could be a new issue if read performance will continue to be prioritized for the app.
Test plan
test_benchmark_annotate_all_*)alembic upgrade head)CONCURRENTLYrefresh works (unique indexes in place as prerequisite)