0
mirror of https://github.com/bbenchoff/OrthoRoute.git synced 2026-08-19 05:33:48 +00:00
Files
OrthoRoute/FINDINGS.md
Brian Benchoff 8e5226c4a3 [phase-8] Finalize FINDINGS.md; document skipped portal extraction
Extraction 4 (escape/portal planning) skipped per the plan's bail-out
clause: ~40 router methods entangled with accounting/hotsets/via ownership
overlapping the existing pad_escape_planner.py — moving them would be a
rewrite, not a relocation. FINDINGS.md records the full per-phase log,
before/after counts, and unanticipated findings.
2026-07-30 13:33:40 -07:00

16 KiB
Raw Permalink Blame History

Hardening audit findings — branch hardening/audit-2026-07

Summary

All phases complete (Phase 8: 3 of 4 extractions; the fourth skipped deliberately).

Metric Before After
Unit+integration suite 342 passed / 9 skipped 345 passed / 9 skipped (+3 new strict-mode tests)
Engine smoke suite 80 passed 80 passed
Removed tests none (no test referenced deleted code)
cuda_dijkstra.py 5,928 lines 3,922
unified_pathfinder.py 11,367 lines ~9,220 (three collaborator modules extracted)
persistent_kernel.py 702 lines ~500 (dead kernel variant removed; occupancy sizing added)

New modules: pathfinder/cuda_common.py (shared device preamble), shared/profiling.py (ORTHO_PROFILE instrumentation), manhattan/geometry_emitter.py, manhattan/via_accounting.py, manhattan/hotset_policy.py. New env vars: ORTHO_PROFILE=1, ORTHO_STRICT=1. No routing-semantics changes anywhere.

Implementation log for the July 2026 audit hardening task. One section per phase. Ground rules observed: no routing-semantics changes, tests are the contract, one commit per phase.

Environment: macOS (Darwin), Python 3.12 in an isolated venv with pytest numpy scipy psutil only. No CUDA hardware — all GPU-path changes are read-verified only, per the task's ground rule 1. The Darwin requirements marker installs mlx, not cupy, so the plan's note that "cupy will fail to build" did not apply; cupy is simply skipped.

Phase 0 — Baseline

  • python -m pytest tests/ -q --ignore=tests/regression --ignore=tests/test_engine_smoke.py342 passed, 9 skipped
  • python -m pytest tests/test_engine_smoke.py -q80 passed

Exactly matches the expected baseline in the task description. The suites are fully green with only pytest numpy scipy psutil installed (no PyQt6, no mlx, no cupy).

Pre-existing working-tree change carried into this branch: one .gitignore line adding /notes/ (gitignored session-documentation folder, requested by the maintainer).

Phase 1 — Cooperative launch occupancy query

Replaced the hardcoded num_blocks = 80 in launch_persistent_kernel() with _cooperative_grid_size(kernel, threads_per_block):

  • SM count from cp.cuda.Device().attributes['MultiProcessorCount'].
  • Max resident blocks/SM for this kernel from cp.cuda.driver.occupancyMaxActiveBlocksPerMultiprocessor(kernel.kernel.ptr, 256, 0) (the compiled Function behind a RawKernel is kernel.kernel; its .ptr fetch is guarded by AttributeError).
  • Grid = sm_count * blocks_per_sm, clamped to [1, 512].
  • Any query failure → conservative fallback of 32 blocks with a one-line WARNING naming the failed query; chosen config logged once at DEBUG.
  • Result cached in a module dict keyed by (function pointer, block size) — computed once per process.

CuPy import remains lazy/guarded (module-level try/except ImportError was already present); code is unreachable without CuPy. Only one launch site exists (cuda_dijkstra.py:5754), unchanged. Tests: 342/9 + 80, green.

Phase 2 — Exception discipline in the GPU fast path

In _route_all's handler around find_path_fullgraph_gpu_seeds:

  • Added module-level _is_cuda_infrastructure_error() — true when the exception type's module starts with cupy (covers cupy.cuda.runtime.CUDARuntimeError, cupy.cuda.memory.OutOfMemoryError, and cupy_backends.* driver errors). No new top-level cupy import.
  • CUDA infrastructure errors: first occurrence per process logs an actionable WARNING (names the exception, notes the ~10x CPU fallback and the cooperative-launch/SM-count suspicion); repeats log at DEBUG with a running count. Per-run counter self._gpu_fastpath_cuda_failures is reported in the existing [GPU-STATS] end-of-run summary.
  • Non-CUDA exceptions: unchanged WARNING, plus the full traceback now logged at DEBUG (exc_info=True) so real bugs aren't invisible.
  • Fallback behavior and the expected no-path None flow are untouched.

Tests: 342/9 + 80, green.

Phase 3 — Documentation drift

Documentation-only changes (no code):

  • Core-loop step (d) and negotiation STEP 3 now name the full-graph GPU supersource label-correcting search as the primary path, ROI heap Dijkstra as the CPU fallback.
  • Layer counts: header now states 32-layer support and the 32-layer flagship board up front, marks the remaining 18-layer material as examples, generalized "all 18 layers" and "B.Cu (L17)".
  • RESULTS block replaced with pointers to docs/optimization/ and tests/regression/golden_metrics.json (both verified to exist).
  • "Dijkstra" prose for the GPU solver corrected to frontier/queue label-correcting (Bellman-Ford family) in the module docstring, in the cuda_dijkstra.py header (with an explicit "module name is historical" note — no file/class renames), and in the README's two "parallel Dijkstra" mentions.
  • Beyond the plan's list, two sections that directly contradicted the corrected claims were also fixed (still prose-only): "GPU SUPPORT (currently disabled)" — it is the primary runtime path — and two "(TO BE IMPLEMENTED)" tags on portal machinery that has long been implemented (the listed-but-nonexistent methods _route_with_portals, _emit_portal_geometry, _retarget_failed_portals, _gpu_roi_near_far_sssp_with_metrics were removed from the method list).

Tests: 342/9 + 80, green.

Phase 4 — Solver graveyard purge

cuda_dijkstra.py: 5,928 → 3,922 lines. persistent_kernel.py: 702 → 501 (net of the Phase 1 addition). Every deletion was preceded by a repo-wide grep (tests, benchmarks, scripts, main.py, and getattr-style dynamic dispatch checked); the unit suite ran green after each cut.

Important correction to the audit's dead listfind_paths_on_rois is NOT dead: the live entry point find_path_roi_gpu wraps its single ROI into a batch and calls it. That keeps the whole near-far batch pipeline alive: _prepare_batch, _normalize_batch, _run_near_far, the wavefront/compaction/persistent/delta-stepping helpers, _reconstruct_paths, and the K_pool pool arrays (dist_val_pool, dist_stamp_pool, near_bits_pool, …). All kept.

Deleted (no reference anywhere outside the dead cluster itself; internal call chains removed callers-before-callees):

  • find_path_singlefind_path_batch (only caller of it)
  • route_batch_persistent
  • find_path_multisource_multisink_gpu_prepare_batch_multisource, _run_near_far_multisink_relax_near_bucket_gpu, _advance_threshold, _split_near_far_buckets (multisink loop was their only caller)
  • find_paths_bidirectional_batchfind_path_bidirectional_transpose_csr, _unpack_frontier, _expand_frontier_single
  • Bonus dead helpers found during verification (not in the audit list, provably uncalled): _slice_per_roi, _relax_edges_parallel
  • Dead kernels whose last user was deleted (or never existed): relax_kernel, procedural_neighbor_kernel, persistent_kernel_stamped (~535 lines), accountant_kernel; in persistent_kernel.py, the never-compiled PERSISTENT_SSSP_KERNEL_CODE bit-packed variant (~240 lines; only the QUEUE variant is used by create_persistent_kernel)
  • Orphaned instance attribute _persistent_kernel_version (no reader, getattr included)

Kept and worth noting:

  • _fallback_cpu_dijkstra (now cuda_dijkstra.py:3247) also has zero callers, but it was not on the audit's list and is the only CPU-correctness fallback living inside the GPU solver class, so it was left in place. Candidate for a future cut — or for re-wiring.
  • USE_DELTA_STEPPING/USE_PERSISTENT_KERNEL paths inside _run_near_far are disabled by config flags but reachable; untouched.

Removed tests: none — no test referenced any deleted symbol (test counts unchanged). Kept — referenced by tooling: none (benchmarks/ and scripts/ reference no dead entry point).

Preamble dedup: new pathfinder/cuda_common.py exports DEVICE_PRELUDE (atomicMinFloat, f2u, pack_key, atomicMinDistanceKey). Prepended to wavefront_kernel, active_list_kernel, persistent_kernel (cuda_dijkstra.py) and PERSISTENT_QUEUE_SSSP_KERNEL_CODE (persistent_kernel.py); local copies removed. Verified by token-level diff of assembled source vs. original: wavefront_kernel is token-identical; the other three gain only the (previously absent, unused) helper functions and lose nothing — the five original copies differed only in whitespace, parameter names, and __forceinline__ qualifiers. Note: via_kernels.py contains no copies of these functions (audit said it did; it does not).

Tests after phase: 342 passed / 9 skipped + 80 passed — identical to baseline.

Phase 5 — Hygiene: constants, pins, cost budget

  1. Backtrace status codes: BACKTRACE_OK/TRUNCATED/OUT_OF_RANGE/SELF_LOOP module constants in cuda_dijkstra.py, used in _backtrace_fullgraph_path's status check. Truncation (length hit capacity) now logs its own distinct message; out-of-range and self-loop parents get specific messages too. Kernel source unchanged (constants documented as "keep in sync with the kernel source").
  2. Dependency pins: requirements.txt floor raised cupy>=10.0.0cupy>=13 with a note pointing at cupy-cuda12x>=13 wheels; setup.py gpu extra aligned (cupy>=13). requirements-kicad.txt already pinned cupy-cuda12x>=13.4,<14 — the authoritative KiCad-env pin was already correct; README's CUDA 12 text needed no change.
  3. Float32 cost budget: comment block in PathFinderConfig next to the cleanup penalties documenting the magnitudes (base 0.4, pres_fac ≤1024, owner 25×pres_fac, cleanup 1e6, quantum ~0.06 at 1e6). New warn_if_penalties_exceed_float32_budget() (warning only, never raises) checks the cleanup/reservation penalties and owner/path-node × peak-pres_fac products against 2^23 × grid_pitch; wired into PathFinderRouter.__init__ next to the existing config logging. Defaults are all inside the budget, so no warning fires out of the box.

Tests: 342/9 + 80, green.

Phase 6 — Per-net phase instrumentation

New orthoroute/shared/profiling.py, gated on ORTHO_PROFILE=1 (read once at import into a module bool):

  • profile_span(label) — context manager; when enabled it opens an NVTX range (CuPy resolved lazily and only when enabled; absence is harmless) and accumulates perf_counter deltas into a module dict. When disabled it returns a shared no-op singleton — one boolean check and no dict writes, no perf_counter, no NVTX (verified by test: profile_span(x) is _NULL_SPAN).
  • log_profile_summary() — logs one [PROFILE] label=1.23s ... line (sorted by cost) and resets; wired into _pathfinder_negotiation right after the [ITER n] line.

Instrumented regions:

  • find_path_fullgraph_gpu_seeds (cuda_dijkstra.py): seed_prep (terminal dedupe + transfers + node-penalty upload), pool_reset (pool alloc/fills/key init), bitmap_setup (frontier scatter + owner bitmap build — two spans, same label), kernel (persistent launch, or the whole multi-launch loop), backtrace.
  • _route_all (unified_pathfinder.py): clear_path, owner_penalty, and — on both the GPU fast path and the ROI fallback commit block — commit_path, via_ownership, tracking.

Checked: both suites green with and without ORTHO_PROFILE=1; a live smoke test with ORTHO_PROFILE=1 produced [PROFILE] kernel=0.09s seed_prep=0.02s.

Phase 7 — Strict invariant mode

The warn-and-continue on verify_present_matches_canonical() failure in _pathfinder_negotiation moved into a small module function enforce_present_matches_canonical(accounting, iteration) (so it is testable without a router): default behavior identical (WARNING, continue); with ORTHO_STRICT=1 it raises RuntimeError naming the iteration. New tests/unit/test_strict_invariant.py (3 tests, modeled on tests/unit/test_edge_accountant.py): corrupt present directly → default warns / strict raises; clean accountant passes in both modes.

Tests: 345 passed / 9 skipped (+3 new) + 80, green.

Phase 8 — God-class decomposition

Three of the four extractions completed, one commit each, both suites green after each. unified_pathfinder.py: 11,367 → ~9,220 lines. All extractions use delegation: method bodies moved verbatim with self.self._router.; thin delegating methods stay on PathFinderRouter so every internal and external call site is unchanged.

  1. Geometry emissionmanhattan/geometry_emitter.py (GeometryEmitter, 10 methods, ~515 lines). GeometryPayload moved with it and is re-imported by unified_pathfinder for compatibility.
  2. Via accounting & barrel ownershipmanhattan/via_accounting.py (ViaAccounting, 15 methods, ~1,150 lines).
  3. Hotset & stagnation policymanhattan/hotset_policy.py (HotsetPolicy, 13 methods incl. the _rolling_progress_*/_pressure_* pair and two staticmethods, ~700 lines). The HOTSET MECHANISM and BLIND/BURIED VIA docstring sections moved from the giant header into these modules (geometry had no dedicated section).
  4. Escape/portal planning — SKIPPED, per the plan's own bail-out clause. The router carries ~40 portal/escape methods entangled with accounting, hotsets, via ownership, and GPU seeding, and they overlap conceptually with the existing 1,785-line pad_escape_planner.py. The reconciliation is not obvious on inspection; attempting it would have been rewriting, not moving.

Notable mechanics discovered during extraction (why the test contract mattered):

  • The engine smoke tests construct routers with object.__new__(PathFinderRouter) and even call methods unbound with duck-typed fixture objects (UnifiedPathFinder._rank_stagnation_offenders(fixture, ...)). Collaborators are therefore exposed as functools.cached_property (geometry, via accounting) or constructed inline in the delegator (hotset policy) rather than assigned in __init__.
  • Five multi-line getattr(self, "...") reads (line-wrapped, so the mechanical getattr(self, transform missed them) initially pointed at the collaborator instead of the router. The smoke suite caught the behavioral difference (_effective_history_hotset_cap returned 256 instead of 512; hotset composition changed) before commit — fixed and re-verified.

Tests after Phase 8: 345 passed / 9 skipped + 80 passed — baseline plus the three Phase 7 tests, nothing lost.

Unanticipated findings

(collected as encountered; recorded, not fixed, per the task rules)

  • GPUConfig.USE_PERSISTENT_KERNEL = False (unified_pathfinder.py ~line 578) with the comment "DISABLED: Hangs on cooperative kernel launch; using wavefront with atomic keys instead." The Phase 1 occupancy fix addresses the most likely cause of exactly that hang (a grid of 80 blocks that can't all be resident). Worth re-testing the persistent kernel on real hardware with the new grid sizing before considering it permanently dead. Not flipped here — that would be a behavior change.

  • Phase 4 note (verified before starting): the believed-dead methods reference each other — find_path_singlefind_path_batch, find_path_multisource_multisink_gpufind_paths_on_rois, find_paths_bidirectional_batchfind_path_bidirectional. Deletions proceed callers-before-callees so each grep check stays meaningful.

  • A Metal/MLX backend exists (orthoroute/algorithms/manhattan/pathfinder/metal_dijkstra.py, mlx on Darwin in requirements). The audit task doesn't mention it. It is left untouched; changes were checked to not import or alter it.

  • The audit's Phase 4 dead list wrongly included find_paths_on_rois and the K_pool batch machinery — they are live via find_path_roi_gpu (details in Phase 4).

  • _fallback_cpu_dijkstra (cuda_dijkstra.py) has zero callers but was not on the audit list; left in place as the only in-class CPU fallback. Future-cut candidate.

  • The engine smoke tests call some router methods unbound with duck-typed fixture objects — any future refactor of _rank_stagnation_offenders / _select_stagnation_victims / _effective_history_hotset_cap must preserve that.

  • The docstring FILE ORGANIZATION class list carried stale line numbers ("line ~380" etc., off by hundreds of lines); dropped the numbers and added the collaborator modules during Phase 8.

  • benchmarks/ and scripts/ reference no solver entry points at all (nothing had to be kept for tooling).