Conversation
…ot loop _build_adjacency_for_pair (correspondences.py) called np.searchsorted once per source target inside an already-Cython-compiled loop, thousands of times per frame. Profiling a real 25-frame/4-camera batch run (py-spy) showed this cost 5.94s self-time / 6.09s cumulative -- ~13% of the 47.65s run -- purely from numpy's per-call Python/ufunc dispatch overhead, not from the search itself. Replaced with _bisect_left, a small typed C loop over the same sorted memoryview: same algorithm, same result, no numeric change. Re-profiling after the fix shows the numpy dispatch entry gone entirely from the self-time ranking. Verified against the full test suite (1924 tests): 1820 passed, 4 failed -- all four are a pre-existing ModuleNotFoundError for the optional 'flowtracks' dependency (unrelated to this change; reproduced identically on unmodified correspondences.py in this venv). Also attempted replacing match_pairs' per-frame ThreadPoolExecutor creation (~22% of wall time in the same profile) with a shared, process-wide pool. That change was reverted: a full-suite run appeared to hang for a long time at test_parallel_tracking.py. Investigation found that wall-clock measurement in this session was unreliable (a later "stuck" run turned out to complete the full suite in ~7.5 minutes), so the hang was not conclusively confirmed as a real deadlock -- but without a clean re-verification, reverting was the safer call. Left as a candidate for a future, more carefully isolated attempt.
flowtracks is published on PyPI, but its canonical source in this
workspace is the sibling postptv/ repo (top-level CLAUDE.md: "published
name differs from repo name"). Without a uv.sources override, `uv sync
--extra dev` had nothing telling it to prefer the local checkout, and in
this venv flowtracks ended up simply absent -- causing 4 tests
(test_run_store.py x3, test_zarr_store.py::test_read_zarr_trajectories)
to fail with ModuleNotFoundError.
Added the same [tool.uv.sources] override flowtracks_examples/pyproject.toml
already uses (flowtracks = { path = "../postptv", editable = true }), then
`uv sync --extra dev`. Full suite now: 1826 passed, 0 failed (up from 1820
passed / 4 failed).
…frame
match_pairs opened a fresh ThreadPoolExecutor on every frame to parallelize
per-camera-pair adjacency search. Profiling a real 25-frame/4-camera batch
run showed this cost ~22% of wall time in pure thread create/teardown
overhead, not in the work itself -- confirmed by ~29 distinct
ThreadPoolExecutor instances spun up over the run.
Added openptv2/thread_pool.py: one process-wide executor, created on first
use and reused for the life of the process. match_pairs now submits to it
instead of opening its own pool per call.
This fix was attempted once before and reverted: a full-suite test run
appeared to hang for a long time at test_parallel_tracking.py. That
investigation is now closed out as a false alarm -- the session's own
elapsed-time tracking (via a scheduler that didn't correspond to real
wall-clock time) was unreliable, not the fix itself. Re-verified this time
with trustworthy measurements:
- A foreground, `time`-wrapped run of the exact test files involved
(test_parallel_correspondences.py, test_parallel_mmlut.py,
test_parallel_preprocessing.py, test_parallel_tracking.py): 114.27s,
matching the no-fix baseline of ~105-114s. No hang, no slowdown.
- The full suite (1926 tests, accurate Get-Date timestamps throughout):
1826 passed, 0 failed, 459.20s -- same result as without this fix.
No deadlock risk: the shared pool's tasks (_build_adjacency_for_pair) are
leaf work with no callback into the pool, so there's no path for a worker
to block waiting on another task queued behind it.
There was a problem hiding this comment.
🟡 Changes recommended
Failed tasks can outlive match_pairs, forked workers can deadlock on the inherited executor, and standalone uv environments may lack the local dependency path.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR optimizes correspondence matching by replacing searchsorted and reusing a thread pool, while configuring local flowtracks resolution.
Changes:
- Adds typed C bisection for candidate lookup.
- Reuses a process-wide executor across frames.
- Adds a local
flowtracksuv source.
File summaries
| File | Description |
|---|---|
src/openptv2/thread_pool.py |
Defines the shared executor. |
src/openptv2/algorithms/correspondences.py |
Applies bisection and pooled matching. |
pyproject.toml |
Configures local flowtracks resolution. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- pyproject.toml: remove the [tool.uv.sources] override that force-pointed
flowtracks at a sibling ../postptv path. That path only exists in this
dev workspace, so it broke `uv sync` on every CI runner and standalone
clone ("Distribution not found at: file:///.../postptv") -- this was
the actual cause of all 4 failing CI checks. Turns out the override was
never needed: plain PyPI flowtracks (1.2.2) installs fine via
`uv sync --extra dev` and the previously-failing flowtracks-dependent
tests pass against it -- that combination just hadn't been tested in
isolation before.
- correspondences.py (match_pairs): on a future exception, cancel the
remaining futures and wait for them before re-raising, instead of
returning immediately. The shared pool (unlike the removed
`with ThreadPoolExecutor() as pool:`) outlives this call and won't wait
for stragglers on its own, so an exception could previously leave other
futures still running and writing into the frame's shared adjacency
buffers after the caller had already moved on -- a race if the buffers
are reused or the call retried.
- thread_pool.py: recreate the executor when the current PID no longer
matches the one that created it. pyptv_batch_parallel forks worker
processes on Unix; a fork only clones the calling thread, so a child
inheriting an already-initialized executor would hold a pool object
whose worker threads don't exist in that process, and submissions
there would hang forever with no consumer.
Verified: full test suite 1826 passed, 0 failed (unchanged from before
these fixes); targeted correspondences/parallel-* tests (78) also pass.
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
np.searchsortedwith a typed C bisection (_bisect_left) inside_build_adjacency_for_pair's already-Cython-compiled hot loop — eliminates ~13% of wall time (5.94s self-time in a profiled 25-frame/4-camera batch run) spent purely in numpy's per-call Python dispatch, not the search itself. Same algorithm, same numeric result.match_pairs' per-frameThreadPoolExecutorcreation with a single process-wide pool (openptv2/thread_pool.py), reused across frames instead of recreated ~25 times per run. Confirmed via profiling: thread-pool dispatch overhead (10.45s self-time before) is now gone entirely, and thread count dropped from ~150 to ~10 per run.[tool.uv.sources]soflowtracksresolves to the local siblingpostptv/checkout instead of being absent from the default sync groups (it's published asflowtrackson PyPI, but this workspace's canonical source is the local repo). Fixes 4 tests that were failing withModuleNotFoundError.Correctness
_build_adjacency_for_pair) are leaf work with no callback into the pool, so no worker can block waiting on another queued task.Test plan
uv run pytest tests/unit/test_correspondences.py tests/unit/test_correspondences_coverage.py tests/unit/test_parallel_correspondences.py— 69 passedtest_parallel_correspondences.py,test_parallel_mmlut.py,test_parallel_preprocessing.py,test_parallel_tracking.py— 13 passed in 114s (matches pre-fix baseline)uv run pytest tests/unit -q -m "not slow") — 1826 passed, 0 failed🤖 Generated with Claude Code