perf(utils): opt-in chunked demean_map dispatch + corrected solver-phase memory attribution#605
Merged
Merged
Conversation
…ase memory attribution Adds an internal, opt-in memory lever for the Rust FE-absorption kernel and corrects a wrong attribution shipped with #603: - diff_diff/utils.py: _demean_map_rust can dispatch variables to the kernel in balanced near-equal column blocks (DIFF_DIFF_DEMEAN_CHUNK_COLS env knob, read per-call; invalid/non-positive values fall back silently per the DIFF_DIFF_BACKEND convention). DEFAULT IS OFF (single full-width dispatch - byte-identical behavior to #603). Chunking is exact partitioning: per-column outputs and iteration counts are unchanged by construction (no cross-column arithmetic), locked by exact-equality tests vs single-call dispatch plus numpy-engine parity at atol=1e-12. Balanced bounds (never a small remainder block) because a 2-column tail measured ~+11% wall-clock: it runs nearly serial across the rayon pool. - Corrected memory attribution (docs/performance-plan.md): the rust-backend firm-panel fit's ~21GB peak (vs ~13.4GB numpy-backend) is dominated by the SOLVER phase (faer path holds an owned copy of the ~2.5GB stacked design + SVD workspace), NOT the demean kernel's marshalling as the #603 narrative claimed. Measured: width-16 chunked dispatch cuts the kernel's transients to near-numpy footprint in isolation, yet the end-to-end fit still peaks ~19-21GB (-5-12%) at +2-7% wall-clock - which is why the knob ships opt-in rather than default-on (pre-agreed gate: >=20% RSS reduction AND <=5% slowdown; measured result fails both legs). Measurement note recorded: single-run macOS ru_maxrss is unreliable under ambient memory pressure (compressor deflates resident peaks). - TODO.md: new Actionable/Performance row for rust solve_ols peak memory on wide designs (the actual dominator); solver-bound SunAbraham speed row demoted Actionable -> Parked per the correctness-first decision (QR-reuse removes the SVD's independent near-collinearity detector and perturbs all coefficients; re-open on practitioner demand, gate on fixest/R parity). - tests/test_rust_backend.py: TestDemeanMapKernel +5 (chunked-vs-single exact equality incl. weighted, boundary splits with kernel call-count assertions, cross-chunk non-convergence warning naming, estimator-level ATT/SE parity at chunk=2, default-is-single-dispatch proof) and standalone TestDemeanChunkResolver (env parsing, invalid fallback, runs without the Rust build). All chunk tests delenv-guard against ambient DIFF_DIFF_DEMEAN_CHUNK_COLS exports. - REGISTRY.md note extended (opt-in knob, OFF by default, invariance by construction); doc-deps utils note updated; CHANGELOG Added entry. No Rust changes; no committed baseline changes (default path identical, PR-C-session fe_absorption_rust.json retained). Sweeps: 461 passed (rust) / 371 passed (DIFF_DIFF_BACKEND=python) on the targeted files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Overall Assessment✅ Looks good — no unmitigated P0/P1 findings. Executive Summary
Methodology
Code QualityNo findings. Performance
MaintainabilityNo findings. Tech DebtNo untracked tech-debt findings. The new solver-memory item is tracked under the actionable performance backlog in SecurityNo findings. The new env knob is read-only configuration and does not introduce secret handling or external input execution. Documentation/Tests
I did not run the full pytest suite; review was based on the provided diff plus targeted source inspection and a small sanity check of the balanced-boundary arithmetic. |
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
_demean_map_rustcan dispatch variables in balanced near-equal column blocks via the internalDIFF_DIFF_DEMEAN_CHUNK_COLSenv knob (read per call; invalid/non-positive values fall back silently, mirroring theDIFF_DIFF_BACKENDconvention). Default is OFF - a single full-width dispatch, byte-identical to the perf(rust): compiled MAP demeaning kernel - rayon across columns, 1.4-2.8x on FE-absorption workloads #603 behavior, so no baseline moves and no default-path performance change. Balanced bounds matter: a naive fixed-stride split leaves a small remainder block that runs nearly serial across the rayon pool and measured ~+11% wall-clock on the firm-panel workload.assert_array_equal) plus the house cross-engine parity contract vs_demean_map_numpy(iteration-count equality +assert_allcloseatol=1e-12).docs/performance-plan.md): width-16 chunked dispatch cuts the kernel's transients to near-numpy footprint in isolation, yet the end-to-end fit still peaks ~19-21GB (only ~5-12% below unchunked) at +2-7% wall-clock. That failed the pre-agreed default-on gate (>=20% RSS reduction AND <=5% slowdown), hence opt-in. A measurement-methodology note is recorded alongside: single-run macOSru_maxrssis unreliable under ambient memory pressure (the compressor deflates resident peaks; one probe read 15.1GB for a configuration that reproducibly peaks ~19-20GB).solve_olspeak memory on wide designs (the actual dominator;DIFF_DIFF_BACKEND=pythonis the interim OOM escape hatch); the solver-bound SunAbraham speed row moves Actionable -> Parked per the correctness-first decision (QR-reuse would remove the SVD's independent near-collinearity detector - deliberate defense-in-depth for the no-silent-failures contract - and perturb all coefficients at the last digits; re-open on practitioner demand, gated on fixest/R end-to-end parity).Methodology references (required if estimator / math changes)
docs/methodology/REGISTRY.md"Absorbed Fixed Effects with Survey Weights" (Rust-kernel Note extended with the opt-in chunking sentence).Validation
tests/test_rust_backend.py::TestDemeanMapKernel+5 cases (chunked-vs-single-call EXACT equality, weighted + unweighted; balanced-boundary splits with kernel call-count assertions proving the multi-chunk path ran; non-converging variable in a later chunk still named in the warning; estimator-levelDiD(absorb=)ATT/SE parity at chunk=2; default-is-single-dispatch proof) and a standaloneTestDemeanChunkResolver(env parsing incl. invalid-value fallback; runs without the Rust build). All chunk testsdelenv-guard against ambient env exports. Targeted sweeps: 461 passed (rust build) / 371 passed (DIFF_DIFF_BACKEND=python) across test_rust_backend, test_utils, test_within_transform, test_sun_abraham, test_estimators.docs/performance-plan.md(frozen-code, 3-repeat arms + single-run probes; identity gate vs committed after-baselines at 1e-13-1e-16 incl. the exempt stress scenario, both arms). No committed baseline changes - default behavior is identical to perf(rust): compiled MAP demeaning kernel - rayon across columns, 1.4-2.8x on FE-absorption workloads #603.Security / privacy
Generated with Claude Code