You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@otava.apache.org by "MrlixiangWE (via GitHub)" <gi...@apache.org> on 2026/08/13 18:30:49 UTC
[PR] Make AnalyzedSeries change points lazy properties [otava]
MrlixiangWE opened a new pull request, #169:
URL: https://github.com/apache/otava/pull/169
Constructing an `AnalyzedSeries` runs change point detection for every metric up front, even when the caller never reads the results. #79 asks for `change_points` and `change_points_by_time` to become properties instead.
`change_points`, `weak_change_points`, `change_points_by_time` and `change_points_timestamp` are now read-only properties backed by private fields. The first access computes everything once and caches it; `append()` and `from_json()` write the backing fields directly. Two behavior notes:
- `change_points_timestamp` now records when the change points were actually computed rather than when the object was constructed. A timestamp stored in JSON still takes precedence on deserialization.
- Constructing with an explicit `change_points` argument used to leave `weak_change_points` unset, so reading it raised `AttributeError` unless `from_json` patched it afterwards. It now defaults to an empty collection.
Construction cost measured on random data (Linux, Python 3.11):
| series | construction before | construction after | first access after |
|--|--|--|--|
| 1 metric x 2000 points | 64 ms | < 0.01 ms | 63 ms |
| 4 metrics x 5000 points | 575 ms | < 0.01 ms | 616 ms |
Callers that do read change points pay the same total cost as before, and repeated accesses hit the cache. Detection results are unchanged: no expected value in the test suite needed to change. The only adjusted test is `test_validate`, which used to fake an uncomputed state by assigning `change_points = None` and now constructs with an explicitly empty collection.
Verification:
- `pytest tests perf` (non-container): 194 passed, including three new regression tests for laziness/caching, `append()` on a freshly constructed instance, and deserialization without recomputation
- `flake8`, `ruff check`, `ruff format --check`, `isort --check-only`: clean
- `git diff --check`: clean
Closes #79
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make AnalyzedSeries change points lazy properties [otava]
Posted by "Gerrrr (via GitHub)" <gi...@apache.org>.
Gerrrr commented on code in PR #169:
URL: https://github.com/apache/otava/pull/169#discussion_r3848900013
##########
tests/series_test.py:
##########
@@ -407,3 +408,83 @@ def test_series_raw_initialization():
assert len(series.time) == 3
assert series.data["throughput"] == [10.0, 12.0, 11.5]
+
+
+def test_change_points_computed_lazily_and_cached(monkeypatch):
+ from otava import series as series_module
+
+ calls = {"count": 0}
+ real_compute = series_module.compute_change_points
+
+ def counting_compute(*args, **kwargs):
+ calls["count"] += 1
+ return real_compute(*args, **kwargs)
+
+ monkeypatch.setattr(series_module, "compute_change_points", counting_compute)
+
+ data = [1.0] * 10 + [5.0] * 10
+ test = Series(
+ "lazy_test",
+ branch=None,
+ time=list(range(len(data))),
+ metrics={"m1": Metric(1, 1.0), "m2": Metric(1, 1.0)},
+ data={"m1": data, "m2": data.copy()},
+ attributes={},
+ )
+
+ analyzed = test.analyze()
+ assert calls["count"] == 0
+
+ change_points = analyzed.change_points
+ assert calls["count"] == 2 # one computation per metric
+ assert [c.index for c in change_points.get_change_points_for_metric("m1")] == [10]
+
+ assert analyzed.change_points is change_points
+ assert len(analyzed.change_points_by_time) == 1
+ assert analyzed.weak_change_points is not None
+ assert analyzed.change_points_timestamp is not None
+ assert calls["count"] == 2
+
+
+def test_append_computes_change_points_first():
+ data = [1.0] * 10 + [5.0] * 10
+ test = Series(
+ "append_lazy_test",
+ branch=None,
+ time=list(range(len(data))),
+ metrics={"m1": Metric(1, 1.0)},
+ data={"m1": data},
+ attributes={},
+ )
+
+ analyzed = test.analyze()
+ analyzed.append(time=[len(data)], new_data={"m1": [5.0]}, attributes={})
Review Comment:
This only covers a series with an existing change point. A stable series computes an empty result, which `_validate_append()` mistakes for “not computed.” Could you please add a case that does not produce a change point and explicitly ensure computation rather than checking collection truthiness?
##########
otava/series.py:
##########
@@ -132,29 +132,56 @@ def analyze(self, options: AnalysisOptions = AnalysisOptions()) -> "AnalyzedSeri
class AnalyzedSeries:
"""
Time series data with computed change points.
+
+ Change points are computed lazily, on first access. Constructing an
Review Comment:
nit: I don't think we need this addition to the doc.
##########
otava/series.py:
##########
@@ -132,29 +132,56 @@ def analyze(self, options: AnalysisOptions = AnalysisOptions()) -> "AnalyzedSeri
class AnalyzedSeries:
"""
Time series data with computed change points.
+
+ Change points are computed lazily, on first access. Constructing an
+ instance is cheap for callers that never read them.
"""
__series: Series
options: AnalysisOptions
- change_points: ChangePointsByMetric
- change_points_by_time: ChangePointsByTime
- change_points_timestamp: datetime
def __init__(
self, series: Series, options: AnalysisOptions, change_points: Dict[str, ChangePoint] = None
):
self.__series = series
self.options = options
- # record when these change points were calculated
- self.change_points_timestamp = datetime.now(timezone.utc)
- self.change_points = None
- if change_points is not None:
- self.change_points = change_points
- else:
- cp, weak_cps = self.__compute_change_points(series, options)
- self.change_points = cp
- self.weak_change_points = weak_cps
- self.change_points_by_time = self.__group_change_points_by_time(series, self.change_points)
+ self.__change_points = change_points
+ self.__weak_change_points = ChangePointsByMetric() if change_points is not None else None
+ self.__change_points_by_time = None
+ # records when the change points were calculated
+ self.__change_points_timestamp = (
+ datetime.now(timezone.utc) if change_points is not None else None
+ )
+
+ def __ensure_change_points_computed(self):
+ if self.__change_points is None:
+ cp, weak_cps = self.__compute_change_points(self.__series, self.options)
Review Comment:
`logging.info("Computing change points...` from line 128 should move here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make AnalyzedSeries change points lazy properties [otava]
Posted by "MrlixiangWE (via GitHub)" <gi...@apache.org>.
MrlixiangWE commented on code in PR #169:
URL: https://github.com/apache/otava/pull/169#discussion_r3854850760
##########
otava/series.py:
##########
@@ -320,8 +350,9 @@ def append(self, time, new_data, attributes):
# r has a subset of all metrics, so can't just set change_points to r
for metric, cpglist in r.items():
self.change_points[metric] = cpglist
- self.weak_change_points = w
- self.change_points_by_time = self.change_points.by_time()
+ self.__weak_change_points = w
+ self.__change_points_by_time = self.change_points.by_time()
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make AnalyzedSeries change points lazy properties [otava]
Posted by "Gerrrr (via GitHub)" <gi...@apache.org>.
Gerrrr merged PR #169:
URL: https://github.com/apache/otava/pull/169
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make AnalyzedSeries change points lazy properties [otava]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #169:
URL: https://github.com/apache/otava/pull/169#discussion_r3840585620
##########
otava/series.py:
##########
@@ -331,8 +358,8 @@ def append(self, time, new_data, attributes):
# r has a subset of all metrics, so can't just set change_points to r
for metric, cpglist in r.items():
self.change_points[metric] = cpglist
- self.weak_change_points = w
- self.change_points_by_time = self.change_points.by_time()
+ self.__weak_change_points = w
+ self.__change_points_by_time = self.change_points.by_time()
Review Comment:
`append()` recomputes and replaces the cached change-point results, but it leaves `change_points_timestamp` at the time of the initial computation. After any later append, serialized consumers therefore receive a stale calculation timestamp. Refresh the timestamp alongside the other cached fields.
##########
tests/series_test.py:
##########
@@ -407,3 +408,83 @@ def test_series_raw_initialization():
assert len(series.time) == 3
assert series.data["throughput"] == [10.0, 12.0, 11.5]
+
+
+def test_change_points_computed_lazily_and_cached(monkeypatch):
+ from otava import series as series_module
+
+ calls = {"count": 0}
+ real_compute = series_module.compute_change_points
+
+ def counting_compute(*args, **kwargs):
+ calls["count"] += 1
+ return real_compute(*args, **kwargs)
+
+ monkeypatch.setattr(series_module, "compute_change_points", counting_compute)
+
+ data = [1.0] * 10 + [5.0] * 10
+ test = Series(
+ "lazy_test",
+ branch=None,
+ time=list(range(len(data))),
+ metrics={"m1": Metric(1, 1.0), "m2": Metric(1, 1.0)},
+ data={"m1": data, "m2": data.copy()},
+ attributes={},
+ )
+
+ analyzed = test.analyze()
+ assert calls["count"] == 0
Review Comment:
Making `analyze()` lazy causes the existing `test_change_point_detection_performance` loop at `tests/series_test.py:165` to stop running change-point detection; it now times only object construction, so detector performance regressions can no longer fail that test. The timed loop should access the resulting `change_points` (or `change_points_by_time`) to retain its original coverage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make AnalyzedSeries change points lazy properties [otava]
Posted by "MrlixiangWE (via GitHub)" <gi...@apache.org>.
MrlixiangWE commented on PR #169:
URL: https://github.com/apache/otava/pull/169#issuecomment-5409628733
Done, and rebased onto a1ef00d.
`_validate_append` now ensures computation rather than testing truthiness, which drops its `RuntimeError` branch — with lazy properties there's no reachable "not computed" state left.
The stable-series test then surfaced a second bug: `append()` raises `KeyError` for a metric with no change points. It reproduces on master with one changing and one stable metric, so it predates this PR. I added membership checks in `append()` rather than touching `select_metrics`, since #173 asserts that `KeyError`. Happy to split it out.
231 non-container tests pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make AnalyzedSeries change points lazy properties [otava]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #169:
URL: https://github.com/apache/otava/pull/169#discussion_r3854627653
##########
otava/series.py:
##########
@@ -320,8 +350,9 @@ def append(self, time, new_data, attributes):
# r has a subset of all metrics, so can't just set change_points to r
for metric, cpglist in r.items():
self.change_points[metric] = cpglist
- self.weak_change_points = w
- self.change_points_by_time = self.change_points.by_time()
+ self.__weak_change_points = w
+ self.__change_points_by_time = self.change_points.by_time()
Review Comment:
This eagerly rebuilds the by-time view on every append, even when `change_points_by_time` is never read, so that property is not fully lazy as requested by #79. Invalidate this cache instead; its property will rebuild it on first access from the updated metric cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org