Evidence-gated FreshData performance investigation#141
Conversation
…ata.enterprise Both names are promised by the PEP 562 lazy surface in freshdata/__init__.py (_ENTERPRISE_EXPORTS) but were never re-exported by freshdata.enterprise, so fd.diff_schema / fd.ContractViolation raised AttributeError and the shipped schema_drift_monitoring example crashed. Adds a regression test that resolves every lazy enterprise export.
fd.profile() never accepted contract=/on_missing=; the supported way to have the baseline-free diff ride along is fd.suggest_plan(contract=...), which attaches it at plan.schema_diff. The example now runs end to end.
… not detection-disabling A column containing ±inf made the IQR quantiles NaN/infinite (leaking a numpy RuntimeWarning from quantile interpolation) and made skew/std undefined, so detection_bounds returned None and outlier handling silently skipped exactly the columns that need it most. Fences and skewness are now measured on the finite values; the infinities land outside the fences and are handled like any other outlier. Columns with no finite bulk are still skipped. Covers fd.clean (engine + outliers=clip/flag), fd.profile and fd.suggest_plan.
freshdata <cmd> and dbt-gate are documented for scripting/CI; a wrong input path previously dumped a ~29-line traceback. Both entry points now print '<prog>: error: ...' to stderr and exit 1. Only FileNotFoundError is caught; everything else still propagates intact.
Integer/bool/non-numeric columns cannot hold infinities, so drop_infinite now passes them through without the O(n) isinf scan added in the previous commit.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a complete benchmark, profiling, analysis, reporting, CI, and documentation workflow for FreshData performance investigation. It also adds strict benchmark result validation, semantic probing, baseline measurements, and a safeguard against unsafe scientific-notation numeric parsing. ChangesPerformance investigation
Numeric parsing safety
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
FreshData benchmark report —
|
| fixture | n_rows | n_cols | p50 s | p95 s | peak MB | repair % | false-repair % | preserve % | trust | monotonic | export % |
|---|
Authored-code reduction (Metric 6)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
benchmarks/performance/render.py (1)
19-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort key duplicated with
analysis._sort_key.
_case_sort_keyhere is byte-identical to_sort_keyinanalysis.py. Since both drive user-visible ordering, a future edit to one could silently diverge from the other. Consider exporting a single shared helper fromanalysisand reusing it.🤖 Prompt for 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. In `@benchmarks/performance/render.py` around lines 19 - 32, Remove the duplicated sorting logic from _case_sort_key and reuse the shared _sort_key helper from analysis, exporting it if necessary. Ensure both rendering and analysis continue using the identical user-visible ordering, including all existing key fields and case_id_for_result handling..github/workflows/performance-large.yml (1)
14-14: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift
timeout-minutes: 180may be too tight for the full 90-case matrix; a hit job timeout can silently break the evidence-preservation design.Summing baseline medians from
benchmarks/results/performance/baseline-report.mdfor just this matrix's row/width/config/report-mode combinations and multiplying by the 1-warmup+5-repetition factor gives a rough floor of ~140 minutes for the timed executions alone (on the 8-core Apple Silicon host the baseline was captured on) — before dependency install, ~90 subprocess spin-ups, and the analyze/render/upload steps.ubuntu-latestrunners are typically 2 vCPU and slower for this CPU-bound pandas workload, so real runtime is plausibly higher. If the job hits its 180-minute ceiling, GitHub Actions cancels the whole job andif: always()steps (analyze,render,upload, the final status-preserving step) get only a brief grace period — which can prevent the very evidence upload this workflow exists to guarantee.Consider raising
timeout-minuteswith margin (e.g. 300+), or splitting the matrix across astrategy.matrixof smaller jobs that each upload their own artifact, and adding dependency caching (actions/setup-python'scache: pip) to shave install time off every run.Also applies to: 29-77
🤖 Prompt for 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. In @.github/workflows/performance-large.yml at line 14, Increase the performance workflow’s job timeout from 180 minutes to at least 300 minutes to accommodate the full matrix and post-processing steps. Preserve the existing analyze, render, upload, and final status-preserving workflow steps, and enable pip dependency caching through the existing actions/setup-python configuration if available.
🤖 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 `@benchmarks/performance/cli.py`:
- Around line 144-155: Update the run/profile argument handling in main around
expand_cases to validate every requested config against CONFIGS before
expansion, routing unknown names through parser.error instead of allowing
KeyError. Also catch argparse.ArgumentTypeError from
_report_modes(arguments.report_modes) and pass its message to parser.error so
invalid report modes produce a clean CLI usage error.
In `@benchmarks/performance/worker.py`:
- Around line 70-87: Update the benchmark flow around the timing loop and
PeakRSS/tracemalloc block so RSS and Python allocation peak measurements run in
separate fresh worker processes, isolated from timing repetitions and from each
other. Preserve the existing warmup and timing sample collection, and ensure
each measurement process executes _run_clean(frame, case) with only its intended
instrumentation active.
---
Nitpick comments:
In @.github/workflows/performance-large.yml:
- Line 14: Increase the performance workflow’s job timeout from 180 minutes to
at least 300 minutes to accommodate the full matrix and post-processing steps.
Preserve the existing analyze, render, upload, and final status-preserving
workflow steps, and enable pip dependency caching through the existing
actions/setup-python configuration if available.
In `@benchmarks/performance/render.py`:
- Around line 19-32: Remove the duplicated sorting logic from _case_sort_key and
reuse the shared _sort_key helper from analysis, exporting it if necessary.
Ensure both rendering and analysis continue using the identical user-visible
ordering, including all existing key fields and case_id_for_result handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0b25cd1-edc6-4e4b-b65d-6fca991905d5
📒 Files selected for processing (45)
.github/workflows/performance-large.yml.gitignoreMakefilebenchmarks/performance/__init__.pybenchmarks/performance/__main__.pybenchmarks/performance/analysis.pybenchmarks/performance/baselines.pybenchmarks/performance/cli.pybenchmarks/performance/datasets.pybenchmarks/performance/environment.pybenchmarks/performance/instrumentation.pybenchmarks/performance/memory.pybenchmarks/performance/models.pybenchmarks/performance/render.pybenchmarks/performance/runner.pybenchmarks/performance/schema.pybenchmarks/performance/semantic_probe.pybenchmarks/performance/worker.pybenchmarks/results/performance/baseline-report.mdbenchmarks/results/performance/baseline-summary.jsondocs/benchmarks.mddocs/performance-investigation.mddocs/superpowers/plans/2026-07-11-freshdata-performance-baseline-profiling.mddocs/superpowers/plans/2026-07-13-freshdata-semantic-performance-optimization.mddocs/superpowers/specs/2026-07-11-freshdata-confirmed-performance-bottlenecks-design.mddocs/superpowers/specs/2026-07-11-freshdata-performance-scalability-design.mdexamples/schema_drift_monitoring.pymkdocs.ymlsrc/freshdata/enterprise/__init__.pysrc/freshdata/enterprise/cli.pysrc/freshdata/integrations/dbt/cli.pysrc/freshdata/steps/outliers.pytests/performance/conftest.pytests/performance/test_analysis_render.pytests/performance/test_contracts.pytests/performance/test_datasets.pytests/performance/test_instrumentation.pytests/performance/test_models_schema.pytests/performance/test_runner_cli.pytests/performance/test_semantic_probe.pytests/performance/test_worker.pytests/test_api.pytests/test_enterprise_cli.pytests/test_integrations/test_dbt.pytests/test_outliers.py
2 participants
Summary
Validation
src/freshdata: passed.benchmarks/performance/analysis.py: passed.git diff --check: passed.src/freshdataremains byte-for-byte unchanged from6f6c2fe.Evidence decision
The primary 100k/medium/semantic probe observed repeated within-build values, but the timed removable work was approximately 0.01664 seconds, or 1.06% of context-build time. The approved design therefore remains a documented rejection rather than an unsupported optimization claim.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests