added data version mixin - #727
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #727 +/- ##
==========================================
- Coverage 78.75% 78.74% -0.01%
==========================================
Files 74 75 +1
Lines 10171 10188 +17
==========================================
+ Hits 8010 8023 +13
- Misses 2161 2165 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds version metadata to TimberModel’s serialized data so consumers can detect (and be warned about) potential incompatibilities when deserializing models created with a different compas_timber version.
Changes:
- Introduces
DataVersionMixinto inject a__version__field into__data__and warn on version mismatch during__from_data__. - Updates
TimberModelto use the mixin in its inheritance chain so model serialization/deserialization gains version awareness. - Adds unit tests covering matching/mismatching versions and legacy data without a version key; updates changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/compas_timber/data.py |
Adds DataVersionMixin that appends __version__ to serialized data and warns on mismatched versions during load. |
src/compas_timber/model.py |
Applies DataVersionMixin to TimberModel via multiple inheritance to enable versioned serialization. |
tests/compas_timber/test_data_version.py |
Adds tests validating no-warning roundtrips, mismatch warnings, and legacy payload behavior. |
CHANGELOG.md |
Documents the new serialization version field and mismatch warning behavior. |
| """ | ||
| This mixin adds versioning information to the data representation of a class, | ||
| allowing for compatibility checks when loading data created with different versions of the library. | ||
| """ |
There was a problem hiding this comment.
The class docstring starts with a leading newline and doesn’t follow the project’s numpy-style docstring convention used elsewhere (e.g., Parameters/Returns/Notes sections). Consider rewriting it with a one-line summary on the opening line and numpy-style structure so generated docs stay consistent.
| """ | |
| This mixin adds versioning information to the data representation of a class, | |
| allowing for compatibility checks when loading data created with different versions of the library. | |
| """ | |
| """Mixin that adds version metadata to COMPAS Timber data containers. | |
| This class extends :class:`compas.data.Data` to embed the current | |
| ``compas_timber`` package version into the serialized data. The version | |
| is written when exporting data and checked when loading it back. | |
| Notes | |
| ----- | |
| The version is stored under the ``"__version__"`` key in the data | |
| dictionary so that compatibility checks can be performed when | |
| deserializing. | |
| """ |
| if version != compas_timber.__version__: | ||
| warn( | ||
| f"Data was created with compas_timber version {version}, but you are using version {compas_timber.__version__}. " | ||
| "This may lead to incompatibilities and errors. Consider updating the data or using an older version of compas_timber." |
There was a problem hiding this comment.
The version-mismatch warning is emitted without an explicit warning category or stacklevel, which means the warning will point at this mixin rather than the deserialization call site. Consider specifying a category (e.g., UserWarning) and stacklevel (often 2) to make the warning more actionable for callers.
| "This may lead to incompatibilities and errors. Consider updating the data or using an older version of compas_timber." | |
| "This may lead to incompatibilities and errors. Consider updating the data or using an older version of compas_timber.", | |
| UserWarning, | |
| 2, |
| # Parse, remove the version key, and re-serialize | ||
| import json | ||
|
|
||
| raw = json.loads(data_str) | ||
|
|
||
| def _strip_version(obj): | ||
| if isinstance(obj, dict): | ||
| obj.pop("__version__", None) | ||
| for v in obj.values(): | ||
| _strip_version(v) | ||
| elif isinstance(obj, list): | ||
| for v in obj: | ||
| _strip_version(v) | ||
|
|
||
| _strip_version(raw) |
There was a problem hiding this comment.
This test removes "version" recursively from the entire JSON payload, which could accidentally strip unrelated "version" fields if they ever appear elsewhere in the serialized structure. To better model legacy TimberModel data, consider removing the key only from the TimberModel’s own data dict (the object under the root dtype’s "data"), leaving other objects untouched.
| # Parse, remove the version key, and re-serialize | |
| import json | |
| raw = json.loads(data_str) | |
| def _strip_version(obj): | |
| if isinstance(obj, dict): | |
| obj.pop("__version__", None) | |
| for v in obj.values(): | |
| _strip_version(v) | |
| elif isinstance(obj, list): | |
| for v in obj: | |
| _strip_version(v) | |
| _strip_version(raw) | |
| # Parse, remove the model-level __version__ key, and re-serialize | |
| import json | |
| raw = json.loads(data_str) | |
| data_dict = raw.get("data") | |
| if isinstance(data_dict, dict): | |
| data_dict.pop("__version__", None) |
| def __from_data__(cls, data): | ||
| version = data.pop("__version__", None) | ||
| if version is not None: | ||
| if version != compas_timber.__version__: |
There was a problem hiding this comment.
This will trigger for any version difference, including patch version differences. Ideally, we should trigger warnings only based on semver semantics, i.e. versions higher or equal within the same major are ok, warnings are triggered if major+minor is lower, or if major is different.
|
|
||
| @classmethod | ||
| def __from_data__(cls, data): | ||
| version = data.pop("__version__", None) |
There was a problem hiding this comment.
I know this is non-application data, but using dunder names in json is a bit foreign. The problem is that __version__ (or version) belongs to the same level at which dtype is, but this is injecting it inside data and that's somewhat conflictive.
gonzalocasas
left a comment
gonzalocasas
left a comment
There was a problem hiding this comment.
The mixin approach is cool, but is it going to be used anywhere else except TimberModel? Unless the plan is to upstream this. But assuming it's not used anywhere else, I would instead prefer to code the version stuff inside TimberModel directly. Also, I would try to tackle the concern of placing metadata in data, and would suggest that we overload __jsondump__(..) and @classmethod __jsonload__(..) in TimberModel and add the version field at the meta data level
Adding library version to data when serializing TimberModel with warning when deserializing data created by a different compas timber version.
What type of change is this?
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).compas_timber.datastructures.Beam.class_diagrams.rst(if appropriate).