Stitch multi_stop_search segments lazily on dask backends#3666
Stitch multi_stop_search segments lazily on dask backends#3666brendancol wants to merge 4 commits into
Conversation
multi_stop_search allocated a full-size np.full path array and computed every segment's full-size lazy result to numpy inside the segment loop, so peak memory scaled with the grid instead of the chunks. a_star_search's dask path is sparse on purpose; this brings the multi-stop wrapper in line with it: - dask segments are combined lazily with da.where, keeping the surface's chunking (cupy blocks included for dask+cupy) - per-segment goal costs come from _cost_at_pixel, which computes only the block containing the goal pixel; _optimize_waypoint_order uses the same helper instead of materializing a full array per waypoint pair - numpy and cupy paths are unchanged On a 2000x2000 dask raster (250x250 chunks) peak memory for a 3-waypoint route drops from 133.8 MB (4.18x the full array) to 11.0 MB, with no full-size allocations. Adds regression tests mirroring test_dask_no_large_numpy_arrays for multi_stop_search and optimize_order, and an asv peakmem benchmark (MultiStopSearchDaskMemory) so the memory contract is tracked. Claude-Session: https://claude.ai/code/session_0155N4QGamQVxgpAAPbpQNq4
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Stitch multi_stop_search segments lazily on dask backends
Blockers (must fix before merge)
None found. The claimed memory behavior was verified by running the issue repro against this branch (peak 133.8 MB to 11.0 MB on a 2000x2000 / 250x250-chunk raster, no full-size np.full calls), and the full pathfinding suite passes on numpy, dask+numpy, cupy, and dask+cupy on a GPU host.
Suggestions (should fix, not blocking)
-
xrspatial/pathfinding.py(dask branch of the segment loop): each segment adds ada.wherelayer over every chunk, so a route with hundreds of waypoints on a finely chunked grid builds a task graph of roughlyn_waypoints x n_chunkselementwise tasks._MAX_WAYPOINTScaps this at 1000 and the eager alternative OOMs, so it is a sane tradeoff, but a short comment in the code (or a note in the docstring) would save the next person from rediscovering it. -
multi_stop_searchdocstring:a_star_searchdocuments its dask strategy in a backend table, but the multi-stop docstring says nothing about dask inputs now staying lazy. One sentence ("dask inputs are routed sparsely and stitched lazily; the full grid is never materialized") would document the new contract this PR's tests enforce.
Nits (optional improvements)
-
xrspatial/tests/test_pathfinding.py: thetracking_fullhelper is now copy-pasted three times (a_star test plus the two new multi-stop tests). A small module-level helper or fixture would keep the three guards in sync if the byte threshold ever changes.
What looks good
- The junction-pixel reasoning (no masking needed because the overwriting value is identical, and snap is rejected for dask) is written down where the code does it, not just in the PR description.
_cost_at_pixelhandles the 0-d cupy case for dask+cupy, and_optimize_waypoint_orderreuses it, sooptimize_order=Trueis covered by the same fix rather than left as a known hole.- The peakmem benchmark uses a large grid with nearby waypoints, so it stays fast while making an eager-stitching regression show up as a step change.
- Known conflict with #3648 (benchmarks sweep touching the same file) is called out in the PR body instead of left for the merger to find.
Remaining known cost (tracked, not for this PR)
With a dask friction surface, every a_star_search call still does a full nanmin pass to compute f_min, so an N-waypoint route scans the friction array N-1 times. Issue #3660 records this as out of scope here.
Checklist
- Algorithm matches the documented sparse-dask design
- All implemented backends produce consistent results (parity tests executed, including GPU)
- NaN handling is correct (NaN + offset stays NaN;
isfinitegates the overlay) - Edge cases are covered by tests (optimize_order path has its own no-materialization guard)
- Dask chunk boundaries handled correctly (segments inherit the surface's chunking)
- No premature materialization in the changed paths; single-pixel reads compute one block
- Benchmark exists (
MultiStopSearchDaskMemory.peakmem_multi_stop_search) - README feature matrix: no new function, no backend support change, not needed
- Docstrings: no public signature change; see suggestion about documenting the dask contract
…e np.full tracking helper (#3660) Claude-Session: https://claude.ai/code/session_0155N4QGamQVxgpAAPbpQNq4
brendancol
left a comment
There was a problem hiding this comment.
PR Review follow-up: Stitch multi_stop_search segments lazily on dask backends
Re-review after commit 8e22f15, which addressed the earlier pass:
- Graph-growth suggestion: fixed. The dask stitch loop now carries a comment explaining the
n_waypoints x n_chunkstask growth and why_MAX_WAYPOINTSbounds it. - Docstring suggestion: fixed.
multi_stop_searchnow states that dask surfaces are routed sparsely and stitched lazily, with peak memory scaling with chunk size and explored corridor. - Test-helper nit: fixed. The three
np.fullguards now share_tracking_np_full, and the pre-existing a_star test was switched to it as well, so the guards stay in sync.
Suite re-run after the changes: 59 passed (numpy, dask+numpy, cupy, dask+cupy on a GPU host). flake8 finding count unchanged from base. No new findings; nothing blocking from my side.
… 1 MEDIUM -> #3661) Claude-Session: https://claude.ai/code/session_0155N4QGamQVxgpAAPbpQNq4
…e-pathfinding-2026-07-08-01 # Conflicts: # xrspatial/pathfinding.py
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Stitch multi_stop_search segments lazily on dask backends (follow-up: conflict resolution)
This is a follow-up after merging origin/main to clear the CONFLICTING state. Pushed as 5f0d0639.
Conflict resolution
Only xrspatial/pathfinding.py had a real content conflict, inside the TSP cost-matrix loop in _optimize_waypoint_order. Two sibling changes overlapped there:
- This PR's lazy read:
_cost_at_pixel(seg.data, goal_py, goal_px), which computes only the block holding the goal pixel instead of materializing the whole segment. #3667on main: reuse of symmetric costs so the reverse A* search is skipped whensnapis off, computed through a_pair_costhelper that still materialized the segment via_segment_to_numpy.
I kept main's symmetric-reuse loop and moved the lazy single-pixel read into _pair_cost (pathfinding.py:1360-1388). The two optimizations now compose: on dask, the cost matrix is built without materializing any segment, and the reverse direction is reused when snap is off. Behavior on numpy is unchanged since both sides read the same requested goal pixel.
test_pathfinding.py auto-merged (golden-value tests from #3655/#3663 and the cupy/dask fix from #3637 came in without conflict). The three sweep-state CSVs text-merged; the performance CSV was untouched by this branch, so no row surgery was needed.
Lazy-stitch behavior
The core change of this PR (lines 1538-1638) merged with no conflict. The dask path still builds path_data as a chunked da.where overlay, reads segment goal costs one pixel at a time, and never allocates the full grid. The graph-token reset (path_agg.name = None) from #3652/#3656 is intact.
Test result
PYTHONPATH=<wt> python -m pytest xrspatial/tests/test_pathfinding.py -x -q -> 86 passed in 4.74s (CUDA+cupy present, so GPU and dask+cupy lanes ran).
Checklist
- Merge conflict resolved integrating both sibling changes, not one side
- Lazy dask stitching preserved
- Symmetric-cost reuse from #3667 preserved
- Dask graph-token reset preserved
- All pathfinding tests pass across backends
Closes #3660.
multi_stop_searchon a dask-backed surface allocated a full-sizenp.fullpath array and computed each segment's full-size lazy result to numpy inside the segment loop, so peak memory scaled with the grid rather than the chunks. The sparse design ofa_star_search's dask path (and itstest_dask_no_large_numpy_arraysguard) was being thrown away one call up the stack.Changes:
da.where, keeping the surface's chunking; dask+cupy keeps cupy blocks throughout._cost_at_pixelhelper that computes only the block containing the goal pixel._optimize_waypoint_orderuses the same helper, sooptimize_order=Trueno longer materializes a full h x w array per waypoint pair.Measured on a 2000x2000 dask raster with 250x250 chunks, 3 waypoints (repro script from the issue):
The remaining 11 MB is A* frontier-dict overhead, which scales with the explored corridor.
Backend coverage: numpy and cupy untouched; dask+numpy and dask+cupy verified against numpy results locally (GPU host, cupy tests executed, not skipped).
Tests and benchmarks:
test_multi_stop_dask_no_large_numpy_arraysandtest_multi_stop_optimize_order_dask_no_large_numpy_arrays: samenp.full-tracking guard as the existing a_star test, plus parity checks against the numpy backend.MultiStopSearchDaskMemoryasv benchmark (peakmem_): large grid (128 MB) with nearby waypoints, so a regression back to eager stitching shows as a step change in peak memory while the benchmark stays under a second.Test plan:
pytest xrspatial/tests/test_pathfinding.py(59 passed, includes cupy and dask+cupy on this host)Note: #3648 (benchmarks sweep) touches the same benchmark file; if it merges first this branch needs a trivial conflict resolution in the import block.
https://claude.ai/code/session_0155N4QGamQVxgpAAPbpQNq4