Refactor: Unify ChangePoint and SignificanceTester#161
Conversation
The recent work in #96 to replace the original external dependency on the so called "signal processing" repository with our own implementation, introduced new classess ChangePoint and CandidateChangePoint, in change_point_divisive/base.py but also left in place the original ChangePoint class in analysis.py. These come together in series.py, where the newer is renamed as _ChangePoint() and also acts as a parent to older class, thus aligning their signature as much as possible. It turns out having two similarly named classes can be a source of confusion and bugs. For example, in #141 vishnuchalla fixes a bug that is due to this and has essentially blocked the --orig-edivisive code path completely. This patch is an effort to make the existence of two separate classes very explicit, by renaming them to ChagePointHunter and ChangePointOtava based on their "lineage". A test case is added to exercise the --orig-edivisive code path. The test fails, as predicted by #141. The test is now cmmented out. The bug is due to a missing cp.metric property in one variation of the ChangePoint class. Note that this patch is intended more for discussion than to merge.
* Unify the ChangePoint_ class in hunter code and the new ChangePoint introduced by the new edivisive implementation Then it got out of hand a bit ... * Separate index and timestamp into different domains. cp.index is used in the context of a single metric and its history of results. Time and commit otoh are on the ChangePointGroup level (essentially a "row"). Note that different metrics can now have different cp.index for the same cpg.time or cpg.attributes['commit'], if they have a different history. * Introduce a ChangePoints class which is just a list of ChangePointGroups but actually comes with 2 different implementations. The last one is supposed to become the class you are left holding once all the change points are computed. Until now we had lots of nice classes for each step of computation, but in the end you were left holding a dict[str, ChangePointGroup]. The new class now encapsulates that dict,
This moves stats and functionality up towards parent classes so that generic stats like mean are always computed for all variants. In fact TTestStat is now an empty class, it's functionality fully absorbed by the parent. (But note that the class name/type itself carries information about the pvalue.
55c1b65 to
6a24182
Compare
Gerrrr
left a comment
There was a problem hiding this comment.
This refactoring makes sense to me directionally.
As the main feedback at this stage, I would prefer these data structures to be more rigid type-wise OR, even better, structured in a way that the reader understand the interface of arguments/parameters. Verbose type signatures everywhere would help as well.
Maybe 8 years of Java development destroyed by sense of beauty, but I found it hard to reason about behavior here primarily due to type polymorphism.
| """ | ||
| Utility class with getters and json serialization for a ChangePoint. | ||
|
|
||
| TODO: Maintaining this is tedious. We should replace it with pydantic or some |
There was a problem hiding this comment.
Bump on this one. It would be awesome to get rid of it before merging this PR or at least create a follow-up issue.
There was a problem hiding this comment.
I'll create a follow up issue, at least provided that fixing the bugs already happening here isn't more work than migrating.
There was a problem hiding this comment.
Btw, thought about this after sleeping on it...
I think these to_json/from_json methods are currently overloaded. For example the rounded=True functionality is added (by me) because someone (me) used these methods to produce json that is sent to an API and a user interface. But I realized when working on this patch, that should rather be done in the Report class (which I don't), and these methods should be limited to serialization and deserialization, such as persisting the data to a database. If we could separate and limit functionality like that, perhaps these functions could be simplified a lot and there wouldn't be any issue.
| for metric, points in self.change_points.items(): | ||
| for cpg in sorted(points, key=lambda cpg: cpg.time): | ||
| assert isinstance(cpg, ChangePointGroup) | ||
| intermediate.append(cpg) |
There was a problem hiding this comment.
You probably want to copy cpg here and below; otherwise we are mutating objects here:
from otava.change_point_divisive.base import (
BaseStats, ChangePoint, ChangePointGroup, ChangePointsByMetric,
)
cp1 = ChangePoint(index=6, qhat=0.0, stats=BaseStats([1, 1], [2, 2]), metric="s1")
cp2 = ChangePoint(index=6, qhat=0.0, stats=BaseStats([1, 1], [2, 2]), metric="s2")
cpg_a = ChangePointGroup(time=6.0, attributes={}, changes={"s1": cp1})
cpg_b = ChangePointGroup(time=6.0, attributes={}, changes={"s2": cp2})
by_metric = ChangePointsByMetric({"s1": [cpg_a], "s2": [cpg_b]})
print("before:", cpg_a.changes) # {'s1': cp1}
by_metric.pivot()
print("after 1st pivot:", cpg_a.changes) # {'s1': cp1, 's2': cp2}, mutated
by_metric.pivot() # KeyError: "Duplicate keys. Shouldn't happen."There was a problem hiding this comment.
In the new push I did copy() as a deep copy but pivot() is just another view of the same data.
Yo can always do copy().pivot() if desired.
This comment was marked as resolved.
This comment was marked as resolved.
| for metric, cps in self.change_points.items(): | ||
| change_points_json[metric] = [cp.to_json(rounded=False) for cp in cps] | ||
| for cps in self.change_points: | ||
| change_points_json = [cp.to_json(rounded=False) for cp in cps] |
There was a problem hiding this comment.
Before this change, we were adding adding a new metric -> list-of-changepoints-as-json. With your change, we override an empty dict with list-of-changepoints-as-json.
There was a problem hiding this comment.
>>> from otava.series import Metric, Series
>>> s = Series('t', None, list(range(11)),
... {'a': Metric(1,1.0), 'b': Metric(1,1.0)},
... {'a': [1.02,0.95,0.99,1.00,1.12,0.90,0.50,0.51,0.48,0.48,0.55],
... 'b': [2.02,2.03,2.01,2.04,1.82,1.85,1.79,1.81,1.80,1.76,1.78]},
... {})
...
>>> j = s.analyze().to_json()
>>> print(type(j['change_points']))
<class 'list'>
>>> print(len(j['change_points']), j['change_points'][0]['metric'])
1 aThis is an incorrect answer because we lost b. At the very least, we should do change_points_json[metric] = instead of change_points_json = `.
While at it, let's take a step back evaluate the following scenario
Looking at to_json/from_json a bit more, I'd say we have a problem here. Say, we have a new entry time with time=100, commit=abc123 and a changepoint that regressed 2 metrics - latency got +50% and throughput -20%. In memory, this is a single ChangePointGroup with 2 changepoints.
If we apply the fix described above, it would look like:
{
"latency": [{"metric": "latency", "index": 5, "forward_change_percent": 50, ...}],
"throughput": [{"metric": "throughput", "index": 5, "forward_change_percent": -20, ...}]
}Note that time and commit attributes are missing. If we to write a roundtrip test (we should!), it would fail here.
Wouldn't it be cool if to_json persisted the attributes, so we'd get:
{
"time": 100.0,
"attributes": {"commit": "abc123"},
"changes": [
{"metric": "latency", "index": 5, "forward_change_percent": 50, ...},
{"metric": "throughput", "index": 5, "forward_change_percent": -20, ...}
]
}There was a problem hiding this comment.
You're right about the ChangePointGroups not serializing correctly. The glass half full version of this story is that it was easy to fix with the current ChangePoints* class and hierarcy of data structures.
Clearly the intent has been to do something that follows the by_metric() structure, so I fixed it based on that.
As for the latter half of your comment, the serialization doesn't lose data, rather timestamps and attributes are part of the test results data (Series) . A change point only has the minimal info about the change itself, and reference back to the overall Series, such as metric and index.
There was a problem hiding this comment.
punting on the test as I don't expect these custom to_json/from_json to survive the 0.8.x series...
There was a problem hiding this comment.
Thanks for fixing the immediate bug!
We still have issues like:
>>> from otava.series import Metric, Series, AnalyzedSeries
>>> s = Series(
... "t",
... None,
... list(range(11)),
... {"a": Metric(1, 1.0), "b": Metric(1, 1.0)},
... {
... "a": [1.02,0.95,0.99,1.00,1.12,0.90,0.50,0.51,0.48,0.48,0.55],
... "b": [2.02,2.03,2.01,2.04,1.82,1.85,1.79,1.81,1.80,1.76,1.78],
... },
... {},
... )
>>> j = s.analyze().to_json()
>>> AnalyzedSeries.from_json(j)
Traceback (most recent call last):
File "<python-input-8>", line 1, in <module>
AnalyzedSeries.from_json(j)
~~~~~~~~~~~~~~~~~~~~~~~~^^^
File "/Users/asorokoumov/Projects/otava/otava/series.py", line 419, in from_json
mean_1=cp["mean_before"],
~~^^^^^^^^^^^^^^^
KeyError: 'mean_before'but I don't mind addressing them in a follow-up.
Create separate copy(), from_dict(), from_list() constructors Constructors now just create an empty container Add tests for unified ChangePoint classes; fix many bugs Also move BaseStats compute logic into a calculate() staticmethod so copy() is a plain replace().
Other small fixes
|
Okay so this should represent a bit more serious effort to create a complete pantheon of ChangePoints/ChangePointGroup/ChangePoint/Candidate/Stats classes and with better test coverage too. On the todo side I still need to add the typing info to many functions, possibly some also need a docstring or :param: type info. Also I want to run pytest --coverage to ensure it's good. |
Add a few t-test specific parameters to the TTestStats class
Note: No from_json(), as the higher level from_json() functions can easily just call the constructor directly.
Added typing annotations and docstrings where I felt they were lacking. Cleaned up the serialization and insert to bigquery and postgres. Both of which continue to insert ChangePoint(s), even if they take a ChangePointGroup as arguments. This is because .time and .attributes are now both a property of the ChangePointGroup.
In fact, went all the way and achieved 100% coverage in the files mainly touched by this PR. Lint and format edits too.
|
Okay @Gerrrr and others, I think this is the first attempt at something actually worth reviewing seriously. Hopefully we can merge this to unlock 3 other pr's waiting on this one. I've done some docstrings at strategic locations, but in the end I guess I did fairly little :param: type of docs. Code should be easier to follow for example because constructors were broken into 3-4 different factory methods each of which does one thing only. Happy to add more comments if needed, but also didn't want to overdo it and curious to see your reaction now. On the testing side I installed pytest-cov to keep me honest, and then did something I never did before: 100% code covreage with unit tests! I feel like this could become a thing. |
Gerrrr
left a comment
There was a problem hiding this comment.
I think this refactoring is a huge improvement of the existing API and I am looking forward to its landing.
I did find a few bugs and left a suggestion about to_json/from_json. In general, it would make sense to add a round-trip test for them.
| for metric, cps in self.change_points.items(): | ||
| change_points_json[metric] = [cp.to_json(rounded=False) for cp in cps] | ||
| for cps in self.change_points: | ||
| change_points_json = [cp.to_json(rounded=False) for cp in cps] |
There was a problem hiding this comment.
>>> from otava.series import Metric, Series
>>> s = Series('t', None, list(range(11)),
... {'a': Metric(1,1.0), 'b': Metric(1,1.0)},
... {'a': [1.02,0.95,0.99,1.00,1.12,0.90,0.50,0.51,0.48,0.48,0.55],
... 'b': [2.02,2.03,2.01,2.04,1.82,1.85,1.79,1.81,1.80,1.76,1.78]},
... {})
...
>>> j = s.analyze().to_json()
>>> print(type(j['change_points']))
<class 'list'>
>>> print(len(j['change_points']), j['change_points'][0]['metric'])
1 aThis is an incorrect answer because we lost b. At the very least, we should do change_points_json[metric] = instead of change_points_json = `.
While at it, let's take a step back evaluate the following scenario
Looking at to_json/from_json a bit more, I'd say we have a problem here. Say, we have a new entry time with time=100, commit=abc123 and a changepoint that regressed 2 metrics - latency got +50% and throughput -20%. In memory, this is a single ChangePointGroup with 2 changepoints.
If we apply the fix described above, it would look like:
{
"latency": [{"metric": "latency", "index": 5, "forward_change_percent": 50, ...}],
"throughput": [{"metric": "throughput", "index": 5, "forward_change_percent": -20, ...}]
}Note that time and commit attributes are missing. If we to write a roundtrip test (we should!), it would fail here.
Wouldn't it be cool if to_json persisted the attributes, so we'd get:
{
"time": 100.0,
"attributes": {"commit": "abc123"},
"changes": [
{"metric": "latency", "index": 5, "forward_change_percent": 50, ...},
{"metric": "throughput", "index": 5, "forward_change_percent": -20, ...}
]
}| # Yes it's the same object, a no-op, not a copy. (Open to other opinions here) | ||
| assert by_time != same | ||
| assert by_time.at_timestamp(1.0) == same.at_timestamp(1.0) | ||
|
|
There was a problem hiding this comment.
It would be cool to have test asserting semantical parity between ByTime and ByMetric. Maybe property-based tests?
| """ | ||
| Utility class with getters and json serialization for a ChangePoint. | ||
|
|
||
| TODO: Maintaining this is tedious. We should replace it with pydantic or some |
There was a problem hiding this comment.
Bump on this one. It would be awesome to get rid of it before merging this PR or at least create a follow-up issue.
| Hence this calls pivot() and is slow. | ||
| Alternatively please use .at_timestamp(), .at_commit() or .get_change_points_for_metric() instead. | ||
| """ | ||
| return self.by_time()[n] |
There was a problem hiding this comment.
Thinking out loud, we may want to cache both views. Python's __get__ is expected to work at O(1). Here, we do full rebuild, then return a single item.
There was a problem hiding this comment.
Yes, I've been thinking it would actually be straightforward to essentially change ChangePoints class so that it always creates and maintains an up to date copy of both ChangePointsByMetric and ChangePointsByTime. But I intend to leave that as a follow-up task too and here I will focus on just getting the API right. Note that what I'm doing here should not be worse than we already have, as AnalyzedSeries maintans two separate collections of this and it is possible (although unlikekly) that you create an object where one is computed (by_metrict) but the other one is not (by_time).
henrikingo
left a comment
There was a problem hiding this comment.
Went through all comments in a single pass...
| """ | ||
| Utility class with getters and json serialization for a ChangePoint. | ||
|
|
||
| TODO: Maintaining this is tedious. We should replace it with pydantic or some |
There was a problem hiding this comment.
I'll create a follow up issue, at least provided that fixing the bugs already happening here isn't more work than migrating.
| Hence this calls pivot() and is slow. | ||
| Alternatively please use .at_timestamp(), .at_commit() or .get_change_points_for_metric() instead. | ||
| """ | ||
| return self.by_time()[n] |
There was a problem hiding this comment.
Yes, I've been thinking it would actually be straightforward to essentially change ChangePoints class so that it always creates and maintains an up to date copy of both ChangePointsByMetric and ChangePointsByTime. But I intend to leave that as a follow-up task too and here I will focus on just getting the API right. Note that what I'm doing here should not be worse than we already have, as AnalyzedSeries maintans two separate collections of this and it is possible (although unlikekly) that you create an object where one is computed (by_metrict) but the other one is not (by_time).
| for metric, cps in self.change_points.items(): | ||
| change_points_json[metric] = [cp.to_json(rounded=False) for cp in cps] | ||
| for cps in self.change_points: | ||
| change_points_json = [cp.to_json(rounded=False) for cp in cps] |
There was a problem hiding this comment.
You're right about the ChangePointGroups not serializing correctly. The glass half full version of this story is that it was easy to fix with the current ChangePoints* class and hierarcy of data structures.
Clearly the intent has been to do something that follows the by_metric() structure, so I fixed it based on that.
As for the latter half of your comment, the serialization doesn't lose data, rather timestamps and attributes are part of the test results data (Series) . A change point only has the minimal info about the change itself, and reference back to the overall Series, such as metric and index.
| for metric, cps in self.change_points.items(): | ||
| change_points_json[metric] = [cp.to_json(rounded=False) for cp in cps] | ||
| for cps in self.change_points: | ||
| change_points_json = [cp.to_json(rounded=False) for cp in cps] |
There was a problem hiding this comment.
punting on the test as I don't expect these custom to_json/from_json to survive the 0.8.x series...
|
@Gerrrr ready for another review round |
Gerrrr
left a comment
There was a problem hiding this comment.
I think we are almost there! Also, apologies for long review iterations.
| return self.pivot().__iter__() | ||
|
|
||
| def __len__(self): | ||
| return max([len(cpg) for metric, cpg in self._change_points.items()]) |
There was a problem hiding this comment.
This fails on an empty input:
>>> from otava.series import Series, Metric
>>> series = Series(
... "flat",
... branch=None,
... time=[0, 1, 2, 3],
... metrics={"m": Metric()},
... data={"m": [1, 1, 1, 1]},
... attributes={},
... )
>>> analyzed = series.analyze()
>>> len(analyzed.change_points)
Traceback (most recent call last):
File "<python-input-19>", line 1, in <module>
len(analyzed.change_points)
~~~^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/asorokoumov/Projects/otava/otava/change_point_divisive/base.py", line 666, in __len__
return max([len(cpg) for metric, cpg in self._change_points.items()])
ValueError: max() iterable argument is empty| attributes=self.__series.attributes_at(c.index), | ||
| ) | ||
| ) | ||
| # TODO: Remove this. It should not be a requirement that metrics have the same history. |
| self.change_points_by_time = self.__group_change_points_by_time(self.__series, self.change_points) | ||
| return result, weak_change_points | ||
| r = ChangePointsByMetric.from_dict(result) | ||
| w = ChangePointsByMetric.from_dict(weak_change_points) |
There was a problem hiding this comment.
It looks like we also need to update ChangePointsByTime:
>> from otava.series import Series, Metric
>>> series1 = [1.02, 0.95, 0.99, 1.00, 1.12, 0.90, 0.50, 0.51, 0.48, 0.48, 0.55]
>>> series2 = [2.02, 2.03, 2.01, 2.04, 1.82, 1.85, 1.79, 1.81, 1.80, 1.76, 1.78]
>>> series = Series(
... "test",
... branch=None,
... time=list(range(len(series1))),
... metrics={"series1": Metric(), "series2": Metric()},
... data={"series1": series1, "series2": series2},
... attributes={},
... )
>>>
>>> analyzed = series.analyze()
>>> analyzed.append(
... time=[11, 12],
... new_data={"series2": [33.33, 46.46]},
... attributes={},
... )
(<otava.change_point_divisive.base.ChangePointsByMetric object at 0x104cb2ea0>, <otava.change_point_divisive.base.ChangePointsByMetric object at 0x1129556d0>)
>>> by_metric = [
... int(cp.index)
... for cp in analyzed.change_points.get_change_points_for_metric("series2")
... ]
>>> by_time = [
... (group.time, sorted(group.changes))
... for group in analyzed.change_points_by_time
... ]
>>> print("by_metric:", by_metric)
by_metric: [4, 11]
>>> print("by_time:", by_time)
by_time: [(4, ['series2']), (6, ['series1'])]| self.change_points_by_time = self.__group_change_points_by_time(self.__series, self.change_points) | ||
| return result, weak_change_points | ||
| r = ChangePointsByMetric.from_dict(result) | ||
| w = ChangePointsByMetric.from_dict(weak_change_points) |
There was a problem hiding this comment.
We build and return w, but we never assign it. Is this correct?
| for metric, cps in self.change_points.items(): | ||
| change_points_json[metric] = [cp.to_json(rounded=False) for cp in cps] | ||
| for cps in self.change_points: | ||
| change_points_json = [cp.to_json(rounded=False) for cp in cps] |
There was a problem hiding this comment.
Thanks for fixing the immediate bug!
We still have issues like:
>>> from otava.series import Metric, Series, AnalyzedSeries
>>> s = Series(
... "t",
... None,
... list(range(11)),
... {"a": Metric(1, 1.0), "b": Metric(1, 1.0)},
... {
... "a": [1.02,0.95,0.99,1.00,1.12,0.90,0.50,0.51,0.48,0.48,0.55],
... "b": [2.02,2.03,2.01,2.04,1.82,1.85,1.79,1.81,1.80,1.76,1.78],
... },
... {},
... )
>>> j = s.analyze().to_json()
>>> AnalyzedSeries.from_json(j)
Traceback (most recent call last):
File "<python-input-8>", line 1, in <module>
AnalyzedSeries.from_json(j)
~~~~~~~~~~~~~~~~~~~~~~~~^^^
File "/Users/asorokoumov/Projects/otava/otava/series.py", line 419, in from_json
mean_1=cp["mean_before"],
~~^^^^^^^^^^^^^^^
KeyError: 'mean_before'but I don't mind addressing them in a follow-up.
| weak_change_points_json = {} | ||
| for metric, cps in self.weak_change_points.items(): | ||
| weak_change_points_json[metric] = [cp.to_json(rounded=False) for cp in cps] | ||
| wcpbm = self.change_points.by_metric() |
There was a problem hiding this comment.
Should this use self.weak_change_points?
| ChangePoint( | ||
| index=cp["index"], time=cp["time"], metric=cp["metric"], stats=stat | ||
| ) | ||
| ChangePoint(index=cp["index"], time=cp["time"], metric=cp["metric"], stats=stat) |
There was a problem hiding this comment.
time=cp["time"]
ChangePoint no longer accepts time, right?
Okay I don't know what to say... Originally I was just going to merge ChangePoint_ and ChangePoint and add the missing .metrics attribute to the survivor. Then it somehow escalated from there.
This needs much more test coverage, but sharing so you can see where I was going with this and also to get feedback on whether this makes the code better or worse...
ChangePoint:
SignificanceTester: