perf(nodes): make anomaly detector z-score O(1) incremental#1626
Open
Ansh-Karnwal wants to merge 2 commits into
Open
perf(nodes): make anomaly detector z-score O(1) incremental#1626Ansh-Karnwal wants to merge 2 commits into
Ansh-Karnwal wants to merge 2 commits into
Conversation
The z_score method recomputed mean and standard deviation from scratch on every value: it copied the whole window under the lock and ran a two-pass scan over it. That made scoring O(window_size) per value, and with a window configurable up to 10000 on a per-pipeline shared detector it was a real throughput hotspot. Maintain the mean and variance incrementally instead. The detector now keeps assumed-mean shifted running sums over the window and adjusts them in O(1) as values enter and leave, so scoring is O(1) amortized. A shift constant plus a periodic full rebuild (once per window turnover) keeps the running sums accurate on large-magnitude, low-spread streams where the naive sum_sq/n - mean**2 identity loses precision to cancellation. Output is unchanged: score, severity, is_anomalous and the details string all match the previous two-pass result. The z_score path also no longer copies the window under the lock, which shortens the critical section. IQR and rolling_avg are untouched. A new test file pins the incremental output to an independent two-pass oracle (exact 4-decimal equality) across evictions, spikes, large magnitudes, the recompute boundary, non-finite no-ops, and concurrent access. The existing tests are unchanged and still pass.
Ansh-Karnwal
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
July 18, 2026 23:18
Contributor
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesZ-Score detection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AnomalyDetector
participant ZScoreAggregates
participant ZScoreFormatter
Caller->>AnomalyDetector: detect(value, method)
AnomalyDetector->>ZScoreAggregates: update aggregates under lock
ZScoreAggregates-->>AnomalyDetector: aggregate snapshot
AnomalyDetector->>ZScoreFormatter: format score outside lock
ZScoreFormatter-->>Caller: anomaly result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@nodes/test/test_anomaly_detector_zscore.py`:
- Around line 124-147: Add concise PEP 257-compliant docstrings to every newly
added public test_* method in the affected anomaly-detector test class,
including test_insufficient_data_below_two, test_golden_window_mean5_std2, and
test_severity_boundaries and the additionally referenced test ranges. Keep the
existing test logic unchanged.
- Around line 227-254: Strengthen TestLargeMagnitudeStability._assert_sane to
validate parsed mean, deviation, and z-score against a two-pass oracle, using
explicit tolerances appropriate for ~1e9 values. Retain the existing finite and
non-negative standard-deviation checks, and ensure both
test_large_base_small_spread and test_monotonic_timestamp_like exercise the
accuracy comparisons.
- Around line 349-356: Remove the explicit det._recompute_z_aggregates() call
and its lock wrapper from the post-concurrency verification in the anomaly
detector test. Compare det.detect(123.4) directly with the _ref_z_score result
derived from snapshot, preserving the existing assertion so corrupted
incremental aggregate state remains observable.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1492dce9-1eba-413a-ad60-9dea13c02f9d
📒 Files selected for processing (2)
nodes/src/nodes/anomaly_detector/detector.pynodes/test/test_anomaly_detector_zscore.py
…7 docstrings Address review on rocketride-org#1626: - Large-magnitude streams (~1e9) now compare parsed mean, std, and z against the two-pass oracle within explicit tolerances instead of only asserting finiteness. At 1e9 four decimals are well within float64 resolution, so the previous sanity-only checks would have accepted arbitrarily inaccurate values. - Concurrency consistency check no longer rebuilds the aggregates before the final comparison, so an update lost to a race stays observable. - Add docstrings to every public test method and the module helpers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Problem
The
anomaly_detectornode scores every value flowing through a pipeline against a sliding window. On the defaultz_scoremethod (also the fallback for any unrecognized method), eachdetect()call:window = list(self._window)), andmean = sum(window)/n, thenvariance = sum((x-mean)**2 for x in window)/n.So processing M values with window size W is O(M·W). The window is configurable up to 10000, and the detector is a per-pipeline shared singleton whose
threading.Lockalso serializes that O(W) snapshot across every instance sharing it. There is no functional reason for it to be O(W): mean and variance over a sliding window can be maintained in O(1) amortized.Change
Keep assumed-mean shifted running sums over the current window and adjust them in O(1) as values enter and leave:
_z_sx = sum(x - K)and_z_sxx = sum((x - K)**2)for a shift constantK.deque(maxlen)drops it).mean = K + sx/n,variance = sxx/n - (sx/n)**2.The scoring arithmetic now runs outside the lock (it is a pure function of scalars captured under the lock), so the z_score critical section drops from O(W) to O(1) as well.
Numerical stability
The naive
sum_sq/n - mean**2identity suffers catastrophic cancellation on large-magnitude, low-spread data (file sizes, epoch-second timestamps ~1e9), where it can return garbage or a negative variance. Two things prevent that here:Kkeeps the summed quantities small, andKis re-centered on the current mean at each rebuild so it stays near the data even on drifting streams.window_sizevalues the sums are rebuilt from scratch over the current window (one O(W) pass per window turnover, so still O(1) amortized). This also clears any per-op rounding residue. Amax(variance, 0.0)clamp keeps the tiny negative residue on a zero-variance window from reachingsqrt.Output is unchanged
score,severity,is_anomalous, and thedetailsstring all reproduce the previous two-pass result. IQR and rolling_avg keep their original snapshot-and-scan path untouched, as does the non-finite-input early return.Benchmark
Pure-Python, single machine, median of repeated runs, with the two implementations asserted to produce byte-identical rounded output across the whole 200k/30k-value workload before any timing:
The incremental version holds ~1.7 us/op regardless of window size; the baseline grows linearly with the window.
Tests
New file
nodes/test/test_anomaly_detector_zscore.pypins the incremental output to an independent from-scratch two-pass oracle, asserting exact 4-decimal result-dict equality (no epsilon) across:At 1e9 magnitude, four decimals on the mean is below float64 resolution for any algorithm, so those streams assert the numeric invariants (finite, non-negative variance, no
sqrtdomain error) rather than a byte-identical string; this is called out in the test docstring.The existing
test_anomaly_detector.pyis unchanged and still passes.Docs
No change.
services.json(fields, defaults, ranges) and the README behavior description are preserved exactly, and the generated params block is unaffected, so there is no observable-contract change to document.Verified locally vs CI
Ran the full suite (including the slow marker), ruff check, and ruff format in a clean venv; all green. The benchmark and its correctness gate were run locally. The engine build and the full CI matrix are left to CI.
Summary by CodeRabbit