perf: cut startup job discovery from ~20h to ~2h on NFS - #11
Conversation
Startup did two sequential passes over 143k video groups, one NFS round trip at a time (~10h each on this archive): - select_best_variant stat'ed every rendition file even when the filename resolution already decided the winner; now stats only on ties, making the job-list build metadata-free for typical 5-rendition groups - the job-list build and the two-phase filter now run their per-group checks in a 64-thread pool (STARTUP_STAT_THREADS) since stat/exists release the GIL and the work is pure NFS latency - new phase_needs() fuses needs_transcription + needs_translation into one stat per artifact (5 round trips max instead of ~11 per video); equivalence with the separate checks is pinned by an exhaustive test over all artifact presence/staleness combinations Measured on the production archive: pass 1 ~3200 groups/s (was ~1-4/s), pass 2 ~19-26 jobs/s (was ~4/s).
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesArchive job evaluation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/python/tools/archive_transcriber.py`:
- Around line 1009-1049: Update build_job and the should_skip interaction so
OSError from any per-group filesystem stat is contained to that group instead of
propagating through executor.map and aborting discovery. Catch the expected stat
failure around the should_skip call (or within should_skip if appropriate), log
the affected best_path, and return None so the existing discovery loop continues
processing remaining groups.
- Around line 2070-2092: Update the job-filtering block around check_job and
ThreadPoolExecutor to handle Ctrl+C without waiting for running phase_needs
calls or the executor context manager to drain. Replace the blocking
executor.map/context-manager pattern with the repository’s cancellation-friendly
pattern used by discover_video_jobs, while preserving job ordering and
transcription_jobs/translation_jobs population.
- Around line 1030-1049: The parallel job-building flow using ThreadPoolExecutor
and executor.map must remain interruptible during Ctrl+C. Replace the unbounded
map submission with bounded in-flight futures or explicitly cancel pending
futures on interruption, and ensure executor shutdown does not wait for queued
NFS stat/exists work before propagating the interrupt; preserve the existing job
collection and progress logging behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe84832a-6443-4b1a-b88e-64ea0cb5ddbb
📒 Files selected for processing (2)
src/python/tools/archive_transcriber.pytests/test_archive_transcriber.py
- build_job catches OSError so one transient NFS failure skips that group instead of aborting the whole discovery run - both startup thread pools cancel their queued backlog on interrupt or error instead of draining all remaining groups before exiting
1 participant
Summary
Every service (re)start did two sequential passes over all 143,384 video groups, one NFS round trip at a time — ~10 hours each (measured in the July 10/12 startup logs) before any GPU work began.
Changes
select_best_variantskips stats when the filename decides: it stat'ed every rendition file for a size tiebreak even when resolutions already ranked them; now it stats only genuine ties, making pass 1 metadata-free for typical_1080p…_180pgroups.STARTUP_STAT_THREADS):stat/existsrelease the GIL and the work is pure NFS latency. 64 threads is deliberate — the NFS server also serves production playback.phase_needs()fusesneeds_transcription+needs_translationinto one stat per artifact (≤5 round trips per video instead of ~11). The separate functions remain for other call sites.Test plan
uv run pytest— 102 passed; new exhaustive equivalence test pinsphase_needsto the separate checks across every artifact presence/staleness combinationNote: a stray
architecture_diagram.pngwas accidentally committed and removed in the follow-up commit — squash-merge keeps it out of main entirely.