[adambernier]

Resolves #78.

Migrates domain models to Pydantic v2 while preserving
persistence and report JSON compatibility.

[Copilot]

🟡 Changes recommended

Pydantic entry points are inconsistent and several previously valid persistence inputs now fail or serialize incorrectly.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Migrates domain and change-point models to Pydantic v2 while retaining legacy persistence and reporting interfaces.

Changes:

• Converts series, metrics, statistics, and change points to Pydantic models.
• Adds deprecated serialization wrappers and compatibility aliases.
• Updates reports, storage integrations, documentation, and tests.

File summaries

File	Description
docs/BASICS.md	Documents the new persistence API.
otava/analysis.py	Migrates T-test statistics.
otava/bigquery.py	Uses change-point methods directly.
otava/change_point_divisive/base.py	Migrates core change-point models.
otava/change_point_divisive/significance_test.py	Migrates permutation statistics.
otava/postgres.py	Uses change-point methods directly.
otava/report.py	Removes serializer-wrapper usage.
otava/serialization.py	Adds legacy model-name aliases.
otava/series.py	Implements Pydantic persistence models.
otava/slack.py	Removes serializer-wrapper usage.
tests/series_test.py	Tests persistence and deprecation APIs.

Review details

Suppressed comments (4)

otava/series.py:98

• Restoring the caller's original list after validation can put unvalidated values back into the model. For example, Pydantic converts time=["1"] to [1], the validator sees the integer, and this assignment then replaces it with the string; timestamp comparisons and append() can subsequently fail. Preserve the list identity only after copying the validated values into it.

            object.__setattr__(self, "time", time)

otava/series.py:167

• Passing precomputed change_points without weak_change_points used to initialize an empty weak-change-point collection. This now stores None; because change points are already present, the lazy initializer does not replace it, and model_dump() fails at self.weak_change_points.by_metric(). Retain the prior empty-container default when change points are supplied.

        self._weak_change_points = weak_change_points if change_points is not None else None

otava/series.py:270

• Overriding only model_validate() does not make the historical persistence shape valid through other Pydantic v2 entry points. AnalyzedSeries.model_validate_json(json_payload) and TypeAdapter(AnalyzedSeries).validate_python(payload) bypass this method and validate against the internal {series, options} schema, so the advertised Pydantic persistence API is inconsistent. Move the legacy-shape transformation into Pydantic's validation schema (for example, a model-level before validator) so every entry point uses it.

    def model_validate(cls, obj, *args, **kwargs):
        """Validate both the domain representation and the historical flat document."""
        if isinstance(obj, dict) and "test_name" in obj:
            parsed = cls._parse_persistence_document(obj)

otava/series.py:497

• This manual override ignores all standard model_dump() options (mode, include, exclude, aliases, etc.) and is bypassed by Pydantic's serializer-backed model_dump_json() and nested/TypeAdapter serialization. Consequently, model_dump(exclude={"data"}) still includes data, while model_dump_json() emits only the internal series and options fields and omits persisted change points. Register the persistence representation with Pydantic's serializer machinery instead of overriding this method ad hoc.

    def model_dump(self, *args, **kwargs):

• Files reviewed: 11/11 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[henrikingo]

+1 but I would prefer to keep the to_json(). It's convenient, backward compatible and there doesn't appear to be a reason to remove it?

[henrikingo]

Why wouldn't you keep to_json() and let it be one line that calls model_dump(mode='json')? For example, further down you keep the .copy() method too.

[adambernier]

thanks for the review & makes sense!

i've updated the PR to keep .to_json() and .from_json() as supported convenience wrappers without deprecation warnings.

on AnalyzedSeries, .to_json() delegates directly to model_dump(mode="json").

for ChangePoint, model_dump(mode="json") produces the nested domain structure with stats: {...}, so ChangePoint.to_json() delegates to self.persistence_dict() to preserve the historical flat dictionary shape and guarantee 100% backward compatibility for existing callers.

[henrikingo]

Good!