Resolve #78: migrate domain models to Pydantic v2 - #178
Open
adambernier wants to merge 10 commits into
Open
adambernier wants to merge 10 commits into
adambernier wants to merge 10 commits into
Conversation
Replace manual JSON-owning domain types with validated Pydantic models. Preserve the flat analysis persistence payload through and , including legacy metric and change-point input shapes. Deprecate / and ChangePointSerializer compatibility APIs, move consumers to model-derived change-point fields, and document the migration.
…ic-domain-models # Conflicts: # docs/README.md # otava/series.py # otava/test_config.py
There was a problem hiding this comment.
🟡 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 andappend()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_pointswithoutweak_change_pointsused to initialize an empty weak-change-point collection. This now storesNone; because change points are already present, the lazy initializer does not replace it, andmodel_dump()fails atself.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)andTypeAdapter(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-backedmodel_dump_json()and nested/TypeAdapterserialization. Consequently,model_dump(exclude={"data"})still includes data, whilemodel_dump_json()emits only the internalseriesandoptionsfields 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.
…ic-domain-models # Conflicts: # otava/bigquery.py # otava/postgres.py # otava/slack.py
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
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.
Resolves #78.
Migrates domain models to Pydantic v2 while preserving
persistence and report JSON compatibility.