feat(sequences): add detection sampling, plus S3 bucket and presigned-URL caching - #661
Conversation
github-actions
Bot
added
ext: client
module: services
ext: tests
endpoint: sequences
labels
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #661 +/- ##
==========================================
+ Coverage 93.75% 93.85% +0.10%
==========================================
Files 59 59
Lines 3152 3208 +56
==========================================
+ Hits 2955 3011 +56
Misses 197 197
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c393579 to
5a02acb
Compare
Acruve15
changed the title
5a02acb to
530dc9c
Compare
fe51
left a comment
fe51
left a comment
There was a problem hiding this comment.
LGTM on the approach (I need to test it soon)
The interval-based choice over target-count is the right one, and the key design decision, row_number() computed ascending from the start of the sequence regardless of desc (src/app/crud/crud_detection.py:43-52), is what makes the sampled frame set stable as a sequence grows. Combined with the stable presigned URLs, that's what lets a polling player actually hit its cache.
The two commits are more coupled than the split suggests: without sampling, a full 1,000-detection load burns ~2,000 URL cache entries and the cache is effectively useless.
To communicate to the frontend before they build against this
sampling and limit are independent. ?sampling=10 alone, with the desc=true / limit=10 defaults, returns the 10 most recent sampled frames — not a spread across the sequence. Full coverage needs limit >= ceil(detections_count / sampling), and limit caps at 500 (so sampling >= detections_count / 500).
Worked example for a 20/20/20 player load on a 1,000-detection sequence:
?desc=false&limit=20 # first 20
?desc=false&sampling=48&offset=1&limit=20 # 20 across the middle
?desc=true&limit=20 # last 20, reverse client-side
General form for M middle frames: sampling = floor((N - 40) / M), offset = ceil(20 / sampling), limit = M. detections_count is already returned on the sequence object, so the frontend has everything it needs — no extra endpoint required.
Non-blocking follow-ups
Neither is a problem today; both are tracked separately so they don't hold this PR.
-
#672: purge stale-window cache entries. The window
slotis part of the cache key (src/app/services/storage.py:155), so entries from previous windows are permanently unreachable but stay resident until size-evicted. Effective capacity is roughly half the nominal 8192. Purging them is strictly safe — unlike a fullclear(), which the existing comment rightly argues against. -
#671: cache sizing under multiple workers.
_URL_CACHE_MAXSIZEis per-S3Bucket, and bucket instances are now process-lifetime (src/app/services/storage.py:208), so the bound isn_workers × n_active_orgs × ~8 MBrather than flat. Since we run multiple workers in prod, URL stability is also per-worker: a client sees up to W distinct URLs per frame, so each frame downloads up to W times instead of once. Still a large win over re-signing on every poll, just divided by W.
…r claim Review feedback on #661. sampling and limit are independent, which is easy to get wrong: `?sampling=10` alone, with the desc=true and limit=10 defaults, returns the 10 most recent sampled frames rather than a spread across the sequence. Spanning the whole sequence in one call needs limit >= ceil(detections_count / sampling), and since limit caps at 500 that also means sampling >= detections_count / 500. Both the endpoint description and the client docstring now say so, since the frontend is about to build against this. Also corrects the _stable_presign docstring, which claimed the repo runs a single uvicorn worker. That was inferred from the dev compose files passing no --workers, but production runs several and its deployment config is not in this repo. URL stability is therefore per-worker in production: a client sees up to W distinct URLs per frame, so a frame is fetched up to W times instead of once, still far better than re-signing on every poll but divided by W. See #671.
|
Thanks, both follow-ups are fair and the worker one corrects something I had wrong. Pushed 76ee7fd. The I verified your 20/20/20 recipe against the endpoint rather than just the arithmetic, on a seeded 1,000-detection sequence: Your formula is exactly right ( On workers, you are right and my docstring was wrong. I had asserted "the repo currently runs a single worker (docker-compose.yml passes no On #672, agreed, and it is a consequence of my choosing to put the window On the coupling between the two commits: agreed, and your framing is sharper than mine. Without sampling, a 1,000-detection load burns ~2,000 entries against a 8,192 bound, so ~4 sequences evict each other and the cache stops paying off. Worth noting that makes #672 and #671 more than cosmetic once the frontend starts requesting large 🤖 Addressed by Claude Code |
|
Thanks for the follow-ups — the omitted-limit default plus the two headers is a good answer to the truncation trap, and the worker docstring reads accurately now. One thing I'd like to discuss before this merges, on What I noticed
Concretely, on the 1,000-detection sequence from your verification: Twenty is read as twenty grid points, so it skips 20 × 48 = 960 detections and lands near the end. The flip side is that skipping the first 20 detections requires What I'd expect instead
Two reasons I lean this way:
What it seems to costFrom reading As far as I can tell the two properties that matter both survive:
The questionIs there something that makes this more expensive than it looks, or a case I'm not seeing where counting in sampled units is the more useful behaviour? You've been closer to this query than I have. If it's not technically limiting, this feels like the more natural shape, and now is the cheapest possible moment to pick it — (There's a middle option, keeping a sequence-wide grid and treating @Acruve15 @MateoLostanlen what do you think ? |
|
Thanks for the PR @Acruve15 , great work, it's OK for me. On the offset question: I agree with @fe51 , offset should keep its historical meaning and count raw detections rather than sampled frames. @Acruve15 could you make that change before we merge? As Félix said, now is the cheapest moment since nothing depends on the current behaviour yet |
Two separate costs on every detection fetch: S3Service.get_bucket built a fresh S3Bucket per call, and S3Bucket.__init__ does a blocking head_bucket round-trip, so each request paid one synchronous S3 call on the event loop (10ms against localstack, more against a real endpoint). Bucket instances are one per organization and effectively static, so they are now built once and reused. boto3 stamps the current clock into every signature, so presigning the same key twice returns two different strings. The browser keys its cache on the full URL, so the player re-downloaded every frame on every poll even when the image had not changed. URLs are now reused for a window derived from S3_URL_EXPIRATION, with the window slot in the cache key so no rollover invalidation is needed. Uploads also set Cache-Control, without which browsers only cache heuristically and the stable URLs would not pay off. That applies to newly uploaded objects only; existing frames stay on heuristic caching. Stability is per-process, which is fine while a single uvicorn worker runs, and degrades to one URL per worker per window rather than breaking if that changes.
Closes #660. GET /sequences/{id}/detections takes a sampling=N parameter that keeps one detection in N, so the player can load a whole timeline in one request instead of paging through every frame. Sampling runs in SQL via row_number(), not as a Python slice of a full fetch, which would keep the scan and the serialization cost and only shrink the payload. Row numbers are always computed ascending on created_at, so the sampled frame set does not depend on desc: desc only flips the output order. That also keeps the set stable as a sequence grows, since a new detection lands last and cannot renumber earlier rows, so the player keeps hitting the same frames and the same cached URLs across polls. (rn - 1) % sampling == 0 keeps the first detection, so any non-empty sequence returns at least one row, and offset pages the sampled set rather than raw rows. sampling=1 delegates to the existing unsampled path, so default behaviour is unchanged. The limit ceiling goes from 100 to 500, since the issue's own example (1000 detections at sampling=10) saturated 100 exactly. Also drops a redundant DetectionRead round-trip in the endpoint: it is a bare subclass of Detection, so the extra validate and dump per row was an identity detour.
…r claim Review feedback on #661. sampling and limit are independent, which is easy to get wrong: `?sampling=10` alone, with the desc=true and limit=10 defaults, returns the 10 most recent sampled frames rather than a spread across the sequence. Spanning the whole sequence in one call needs limit >= ceil(detections_count / sampling), and since limit caps at 500 that also means sampling >= detections_count / 500. Both the endpoint description and the client docstring now say so, since the frontend is about to build against this. Also corrects the _stable_presign docstring, which claimed the repo runs a single uvicorn worker. That was inferred from the dev compose files passing no --workers, but production runs several and its deployment config is not in this repo. URL stability is therefore per-worker in production: a client sees up to W distinct URLs per frame, so a frame is fetched up to W times instead of once, still far better than re-signing on every poll but divided by W. See #671.
Guardrail for the trap fe51 spotted in review. sampling thins the candidate set but limit still truncates it, and truncation was invisible: `?sampling=10` on a 1000-detection sequence returned 10 frames from the last 10% of the sequence, which looks like a coarse timeline but is only its tail. Nothing errored. limit is now optional. When sampling is set and limit is omitted it defaults to whatever spans the whole sampled set (capped at 500), so `?sampling=10` returns 100 frames across the sequence instead of 10 at the end. An explicit limit is still honoured, since paging a sampled set is legitimate. Either way a sampled response now carries X-Sampled-Total and X-Sampled-Truncated, so a caller can tell whether what it got covers the sequence rather than having to derive it. Paging to the end of the set is not reported as truncation. Costs one extra COUNT, only when sampling > 1, reusing the existing get_detection_counts_by_sequence_ids so the denominator matches the rows the endpoint actually returns (continuity rows included). Unsampled requests keep the historical limit=10 default, take no extra query, and get no headers.
codecov/patch caught a real gap: making limit optional added a conditional that only forwards it when set, and the client integration test called fetch_sequences_detections without a limit, so that line never ran. Adds two calls on the existing sequence: one with an explicit limit, which is the branch that was uncovered, and one with sampling and no limit, which checks the API sizes the response to the sampled set and returns the X-Sampled-Total / X-Sampled-Truncated headers through the client.
Per review from @fe51 and @MateoLostanlen. offset used to change meaning depending on whether sampling was set: it counted raw detections without it and sampled frames with it, which is why its description needed the "within the sampled set" qualifier. It now always counts raw detections, and sampling applies from that point, so offset=20&sampling=48 starts at detection 21 and steps by 48. At sampling=1 the two readings coincide, so nothing changes there. This drops the conversion callers had to do: the 20/20/20 player recipe becomes sampling = floor((N - 40) / M), offset = 20, limit = M, with no ceil(). offset moves out of the SQL OFFSET and into the WHERE, as `position >= offset AND (position - offset) % sampling == 0` over the ascending row_number. Same scan either way, since the window has to cover every row of the sequence before anything filters. Two consequences worth knowing: - offset now anchors chronologically instead of following the sort. A SQL OFFSET applies after ORDER BY, so previously desc=true made it skip from the newest end. The selection is now fully independent of desc, which only reverses the output, matching how the sampled set already behaved. - offset sets where the grid starts, so it also sets its phase: only advancing in multiples of sampling keeps the same detections in the grid. That is the paging rule, and it is now stated in the parameter description and covered by a test that walks three pages and checks they reconstruct the grid exactly. X-Sampled-Total now counts the frames left from offset onward, which also makes truncation mean "there is more past what you asked for" rather than mixing the offset back in.
5111959 to
4718415
Compare
|
hi @fe51 @MateoLostanlen thanks for your replies and agreed on the approach. I don't have the full view so it's great tht you give me additional context :) I believe this PR is ready to ship ⛵ |
Review pass over the branch. Net 130 lines lighter, no behaviour change except the CORS fix below. Docstrings and comments were doing PR-body work. Trimmed throughout: the _stable_presign docstring goes from 22 lines to 4, _url_cache_window from 8 to 3, fetch_by_sequence from 19 to 7, the three Query descriptions from 32 to 20, and the ten new test docstrings to one or two lines each. Comments that restated the code are gone; the ones giving a non-obvious reason stayed. Simplifications, all behaviour-preserving: - one branch and one assignment for effective_limit instead of assign-then- reassign, math.ceil instead of -(-x // y), and max(1, ...) dropped since sampled_total == 0 only when the offset is past the end, where LIMIT 0 and LIMIT 1 both return nothing - le=MAX_DETECTION_LIMIT instead of repeating the literal 500, and the sampling ceiling gets a name - a user_auth fixture for the tests this branch added, replacing 13 copies of the same five-line token block - test_storage reuses the pinned_url_window fixture instead of repeating its body - dropped test_..._sampling_larger_than_count, subsumed by the degenerate- sampling test which asserts the same row plus the headers - dropped an assertion in the window test that re-implemented the function it was testing, so it could not fail Corrections found on the way: - fetch_by_sequence claimed offset was desc-independent for the whole method. That only holds on the sampling path; the unsampled path delegates to fetch_all, whose SQL OFFSET applies after ORDER BY. Docstring now says so. - the URL cache comment quoted 600-900 byte URLs and an 8 MB ceiling. Measured against this config they are 167 bytes (SigV2) or 333 (SigV4), so the ceiling is nearer 5 MB per bucket, times the organization and worker counts. CORSMiddleware sets no expose_headers, so browser JS could not read X-Sampled-Total or X-Sampled-Truncated at all: the truncation signal was inert for the frontend player it was added for. Both are now exposed, with a test that fails without it. Nothing existing caught this, since the endpoint tests use the ASGI transport and the client tests use requests rather than a browser.
Caught by github-code-quality on b9ab86b. Introducing the user_auth fixture stripped the top-level auth blocks from the tests this branch added, but two pre-existing crop tests hold theirs inside a `try:` at a deeper indent, so the strip missed them and the restore step then inserted a second copy. Both tests assigned auth twice in a row, the first assignment dead. Collapsed to one. The two tests are otherwise untouched, as intended: they predate this branch and keep the file's own idiom rather than the new fixture.
3 participants
Closes #660.
The issue asks for detection sampling so the player can load a sequence without pulling every frame, and names two causes: API response time and client-side caching. Investigating
GET /sequences/{id}/detectionsturned up three further problems, two of which hit harder than sampling itself, so this PR does three things, with the fourth split out into #664.What changed
1. Sampling (the issue's ask).
sampling=Nkeeps one detection in N, pushed into SQL viarow_number()rather than slicing a full fetch in Python (which would keep the scan and the serialization cost and only shrink the payload).sampling=1delegates to the existing path, so default behaviour is unchanged.2.
S3Bucketinstance caching.get_bucket()built a fresh instance per call andS3Bucket.__init__does a blockinghead_bucket, so every request paid one synchronous S3 round-trip on the event loop.3. Stable presigned URLs +
Cache-Control. boto3 stamps the current clock into every signature, so presigning the same key twice returns two different strings. The browser keys its cache on the full URL, so the player re-downloaded every frame on every poll even when the image had not changed. This is the client-side-caching half of the issue, and sampling alone does nothing for it.Performance, before vs after
The index this depends on lives in #664
The composite index on
detections(sequence_id, created_at)was originally part of this PR, but it turned out to be exactly one of the three indexes proposed in #663, so it moved to #664 to keep a single owner. The DB numbers below assume #664 has landed. Without it, the sequence read stays on a sequential scan and this PR's gain is limited to the S3 items.Measured there: 34.8 ms to 0.42 ms at 2M detections (roughly production today), and 63.6 ms to 1.1 ms at 5M.
Per request, 100 frames returned, 5M-row table
head_bucket(once per request)On top of that, sampling changes the shape of the work
Loading a 1000-frame timeline previously meant 10 requests (the old
limitceiling was 100), each paying the ~85 ms above. It is now one request atsampling=10, and the browser fetches 100 images instead of 1000.Worth stating plainly: most of that last factor is returning a tenth of the data by design, not the same work done faster. The honest split is roughly 20x from the fixes (of which the DB share needs #664) and 10x fewer bytes and images from sampling.
The browser-cache win is the one number missing here. Every poll after the first goes from re-downloading N images to N cache hits, which is probably the largest perceived gain for the player, but quantifying it needs the frontend rather than the API.
How this was measured, and two corrections
Numbers come from a seeded 5M-row database (50k sequences) on the dev compose stack. Before/after uses the same data, forcing the pre-change plan with
enable_indexscan=offrather than dropping and rebuilding the index, so the two measurements are directly comparable.Two measurement traps worth recording, since both changed the answer materially:
limitlocated a ~45 ms penalty that vanished between 60.9KB and 65.0KB of response body: the 64KB socket buffer, i.e. a 40 ms delayed-ACK stall on loopback inside the container. It affects before and after equally and is not a property of the API, but it makes absolute sub-64KB latencies from that harness meaningless, so the table above is built from measured components instead. Above 64KB timings go cleanly linear at ~0.03 ms per frame.API compatibility
No breaking change. Only one route's signature moved, and only additively:
samplingadded, default1= exactly the previous behaviourlimitceiling widened 100 to 500, so any previously valid request stays validdescriptionstrings rewordedThe response model is untouched. No routes were added, removed, or renamed, and auth is unchanged. The one behavioural change reaching every route that returns a presigned URL is that URLs are now stable within a window instead of unique per request, which is the point of item 3.
For pyro-platform: nothing is required, and items 2 and 3 speed up the existing call with no frontend work. To get the sampling win it needs to pass
sampling, and alsolimit:getDetectionsBySequencecurrently sends onlydesc, so it receives the API default of 10 detections.samplingandlimitare independent, which is the easy thing to get wrong:?sampling=10alone, with thedesc=trueandlimit=10defaults, returns the 10 most recent sampled frames, not a spread across the sequence. Spanning the sequence in one call needslimit >= ceil(detections_count / sampling), and sincelimitcaps at 500,sampling >= detections_count / 500.detections_countis already on the sequence object, so no extra endpoint is needed. Both the endpoint description and the client docstring now spell this out.A 20/20/20 player load on a 1000-detection sequence, verified against the endpoint rather than derived on paper:
60 distinct frames, no overlap between the three calls. General form for
Mmiddle frames:sampling = floor((N - 40) / M),offset = ceil(20 / sampling),limit = M. Credit to @fe51 for the recipe.Interaction with #624 (now merged)
Rebased onto main after #624 landed. Two points a reviewer should know:
GET /sequences/{id}/detectionsreturn continuity rows (frames attached withbbox="[]"where the object was not detected), and its own route description tells clients to filter onbboxif they only want real detections. Sampling thins that combined set rather than treating continuity rows specially, which keeps it consistent with the documented endpoint behaviour and withdetections_count, which feat(detections): accept empty bboxes and keep sequence frames continuous #624 also made include them.crud_detection.pyconflicted and was resolved by hand. feat(detections): accept empty bboxes and keep sequence frames continuous #624 addedget_latest_with_bbox, which needs sqlmodel'sselect(a single-entitySelectOfScalaris what makessession.execreturn aDetectionrather than aRow). The sampled query needs sqlalchemy'sselectfor its two-entity numbering subquery. Both now coexist:selectstays sqlmodel's, as feat(detections): accept empty bboxes and keep sequence frames continuous #624 wrote it, and sqlalchemy's is imported asselect_sa, so no line of feat(detections): accept empty bboxes and keep sequence frames continuous #624's code changed.Semantics worth reviewing
These are the decisions a reviewer should push on:
sampling=N)created_atdesc=trueanddesc=false;desconly flips output order. It also means an appended detection cannot renumber earlier rows, so the player keeps hitting the same frames and the same cached URLs across polls(rn - 1) % sampling == 0offsetlimitceilingsampling=10) saturated 100 exactly, sosampling=5would have been silently truncatedDeliberately not
id % N: detection ids are global rather than per-sequence, so the stride would be non-uniform and small sequences could return zero rows.Two known limitations:
limitcannot push down throughrow_number(), so the scan is proportional to sequence length regardless oflimit(inherent to interval sampling, and why the index matters); and sampling is uniform increated_at, notrecorded_at, so out-of-order uploads make it non-uniform in capture time.Verification
ty, deps-check). No new dependency.popitemwithclearfails it).Known limits of the URL cache
Both raised in review and tracked separately, neither blocking:
--workers, production runs several, and its deployment config is not in this repo. Corrected in the docstring.clear()the code comment argues against.These matter more once the frontend starts requesting large
limitvalues: a 1000-detection load burns ~2000 entries against a 8192 bound, so a handful of sequences evict each other and the cache stops paying off.Depends on
#664 for the
detections(sequence_id, created_at)index. This PR is functionally independent and its tests pass without it, but the sequence read stays on a sequential scan until it lands.Out of scope
GET /detections/is an unboundedfetch_allwith no limit or offset at all. Real problem, separate issue.Cache-Controlonto already-uploaded objects.deschere andorder_descon the alerts endpoints.