Reduce model invalidation to a single version: the pvlearn release - #19
Merged
Merged
Conversation
The sidecar compared three version fields — feature_schema_version, pipeline_version and sklearn_version — while pvlearn_version was recorded but excluded. That split was both redundant and incomplete. Redundant: runtime dependencies are pinned with ==, so neither a dependency, nor the schema, nor the pipeline can change without a pvlearn release. Every event the three fields detect is already a new release. Incomplete, which matters more: the three cover scikit-learn but not numpy or joblib, and the persisted artefact depends on those too — joblib's pickle path is numpy-version-sensitive, visible today as a DeprecationWarning from joblib/numpy_pickle.py. The claim that numpy/pandas/scipy do not affect reproduction turned out to rest on a single manual run recorded only as prose in a comment; no CI job varies a dependency version, so it cannot fail if it stops being true. The pyproject comment is narrowed to what is actually tested. Only the release segment is compared, so a model survives the dev builds between two releases: setuptools_scm uses local_scheme = "node-and-date", and comparing the full string would invalidate every locally trained model on every commit and again on every new day. Reasoning and the alternatives in docs/adr/0003. Signed-off-by: Johannes Ott <deroetzi@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Johannes Ott <mail@johannes-ott.net>
test_a_sidecar_from_before_adr_0003_is_a_mismatch_not_unreadable built its sidecar with pvlearn_version="0.3.0" and asserted a mismatch. That holds only while the release under test is something other than 0.3.0: it passed locally against a stale editable install (0.2.0.post2) and failed in CI, where setuptools_scm resolves the v0.3.0 tag and the two sides agree. 0.0.1 is below every published release, so the assertion no longer depends on which version the test run was built from and needs no upkeep per release. The sibling test derives its version from release_version() and already moved along on its own. Signed-off-by: Johannes Ott <deroetzi@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Johannes Ott <mail@johannes-ott.net>
The comment calling scikit-learn's pin load-bearing claimed the frozen Phase 0 baseline "is only reproducible against 1.9.0". Same provenance as the numpy/pandas/scipy claim already narrowed in this branch -- prose in commit ad94aac, nothing executable behind it -- and this one is also contradicted by the project's own Phase 1a measurement, recorded in tests/test_extraction_regression.py: the baseline does not reproduce bit-for-bit across CPUs even with every version held fixed, with CI landing 5.26% off the MAE. The binding variable that was actually measured is the machine, not scikit-learn. test_records_the_versions_it_depends_on looks like it guards the claim but asserts only that the frozen sidecar records 1.9.0; it would pass unchanged if a newer scikit-learn reproduced the baseline perfectly. The real guard is the tolerance comparison in test_extraction_regression.py, which does run in CI -- the slow marker is declared but nothing deselects it. Also drops the references to chapters of the deleted Umsetzungsplan that the previous cleanup missed by grepping for the document name rather than "chapter". One of them, in freeze_baseline_forecast.py, pointed at the chapter requiring sklearn_version in the model metadata, which ADR 0003 has since removed. Signed-off-by: Johannes Ott <deroetzi@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Johannes Ott <mail@johannes-ott.net>
Records the convention that produced the previous two commits: no ADR, code comment or docstring may point at an implementation plan, roadmap or phase document. Those are written for one migration and then deleted, and every reference to one rots into a pointer at nothing -- deleting pvlearn-umsetzungsplan.md left nine dangling citations, one of them requiring a metadata field ADR 0003 had already removed. An ADR has to stand alone, so where a plan carried reasoning the record needs, the passage is copied or summarised into the ADR instead. ADR 0001's last citation is replaced that way: "the canonical, provider-independent schema in chapter 3.1" now states what that schema actually guarantees and points at pvlearn/schema.py, which moves with the code and breaks loudly when it stops existing. Naming a phase as the provenance of a measurement stays allowed -- it dates a result rather than sending the reader somewhere. The test is whether removing the document would leave the sentence broken. ADR 0003 also picks up the withdrawn scikit-learn reproducibility claim, so the rule's pointer to it covers both cases rather than one. Signed-off-by: Johannes Ott <deroetzi@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Johannes Ott <mail@johannes-ott.net>
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
1 participant
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.
Summary
The sidecar compared three version fields —
feature_schema_version,pipeline_version,sklearn_version— whilepvlearn_versionwas recorded but deliberately excluded. That split is both redundant and incomplete.Redundant: runtime dependencies are pinned with
==, so neither a dependency, nor the schema, nor the pipeline can change without a pvlearn release. Every event the three fields detect is already a new release.Incomplete, which matters more: the three cover scikit-learn but not numpy or joblib, and the persisted artefact depends on those too — joblib's pickle path is numpy-version-sensitive, visible today as a
DeprecationWarningfromjoblib/numpy_pickle.py. A model pickled under one numpy and unpickled under another is exactly the silent-failure case the sidecar exists to prevent.The claim that numpy/pandas/scipy do not affect reproduction turned out to rest on a single manual run recorded only as prose in a
pyproject.tomlcomment. No CI job varies a dependency version — both jobs differ only in Python version, and pinned deps make them install identical numpy/pandas/scipy/scikit-learn — so the claim cannot fail if it stops being true. The comment is narrowed to what is actually tested.Changes
ModelMetadatacompares one version: the release segment ofpvlearn_version.feature_schema_version,pipeline_version,sklearn_version,FEATURE_SCHEMA_VERSION,PIPELINE_VERSIONandsklearn_minor_version()are removed.0.3.0out of0.3.0.post2+gd22c402c0.d20260807):setuptools_scmuseslocal_scheme = "node-and-date", so comparing the full string would invalidate every locally trained model on every commit and again on every new day.pipeline_versionmechanism is marked superseded (its selection decision stands).A 0.3.0 sidecar still parses — pydantic ignores unknown fields — and is then rejected on its release with a precise reason rather than as unreadable metadata. Covered by a test.
Breaking change: sidecar schema changes and every persisted model is invalidated. Release as 0.4.0.
Test plan
pytest— 231 passedruff check/ruff format --check— cleanpyright— 0 errors