fix(models): version learned PV and load state against its feature vector - #793
Merged
Conversation
…ctor
Both learned models restored by unmarshalling a bare blob, gated only on a
single sanity field, with nothing recording which feature vector the
coefficients were fitted against. Change the harmonic count, the bucket a
moment maps to, or what clear-sky irradiance refers to, and a stale Beta keeps
predicting with a plausible-looking MAE from a fit that describes a different
world.
Each persisted model is now wrapped in {schema_version, feature_hash, model}.
The fingerprint is derived from the feature functions themselves, evaluated
over a fixed probe, so a change to the feature math moves it with nobody
having to remember a constant; a short declared-semantics label next to the
feature definition covers the one thing a probe cannot see, a caller passing
in a differently defined quantity. On mismatch the model logs both hashes at
Info and cold starts, which both models recover from and neither recovers
from silent wrong coefficients.
Pre-envelope state is adopted, but only while the running build still computes
the space it was fitted against: each package freezes the fingerprint that was
current when the envelope landed, and the first feature change retires that
path on its own.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The problem
Both learned models restored by
json.Unmarshalof a bare blob, gated only onone sanity field —
Forgetting > 0for the PV twin,Alpha > 0for the loadmodel — followed by a hand-written migration. Nothing recorded which feature
vector the coefficients had been fitted against.
That is enough to survive a corrupt file. It is not enough to survive a change
to the features, and a change to the features is the failure that matters:
every stored
Betaindexes a vector it was never fitted to;clearSkyWmeans — horizontal GHI to plane-of-array, say — andFeatures()returns the same numbers for the same argument while thecoefficients now describe a different physical quantity;
under the model.
In all three cases the model keeps predicting. It reports a plausible MAE, it
passes every sanity gate it has, and it steers the plan from a fit for a
different world. Nothing in the stored numbers gives it away.
FTW has paid for this class of fault before:
3255deba(local-time bucketindexing corrupting the learned models across DST) and
41e59efb(modelslocking themselves out). Both were found from their effects.
The change
Each persisted model is now wrapped in
{schema_version, feature_hash, model}(
go/internal/modelstate). On any mismatch — hash, envelope schema, unreadableblob — the state is discarded and the model cold starts, logging at Info with
both hashes. A cold start is bounded and self-healing for both models. Silent
wrong coefficients are neither.
How the hash is derived, and why. The brief allowed a hand-maintained
constant as a fallback. That is the failure mode being fixed, so it is a
hybrid, and the automatic half carries the weight:
featureProbe) — the feature functions are evaluated over afixed grid and the results are hashed. Add a harmonic, reorder a slot, revive
the dead intercept, change the cloud exponent, drop the UTC coercion in
HourOfWeek, moveHeatingReferenceC: the probe values move, so the hashmoves, with nobody having to remember anything. Verified by temporarily
changing
2*hto2.001*hinFeatures— the hash moved and the tripwirefired, with no constant touched.
featureSemantics) — one short string next to the featuredefinition, naming what the inputs mean. This is the half a probe provably
cannot derive:
Features(clearSkyW, …)returns identical numbers whether thecaller passes horizontal GHI or plane-of-array irradiance. There is no way in
Go to fingerprint the meaning of a
float64without wrapping it in a type, arefactor that would reach into
forecast.goandpvperf— the exact filesthis PR must stay out of. So it is declared, in one obvious place, with a
comment saying it must change when a caller's input does.
Rounding. Probe values are hashed at twelve significant digits: far finer
than any real feature change, coarse enough to absorb the sub-ULP spread the
same expression can have between an amd64 build host and the arm64 Pi. A state
database stays portable.
Pre-envelope state is adopted, and the path retires itself. Every deployed
site has a bare blob. Discarding those would cost the PV twin an afternoon and
the load model weeks of bucket coverage, for no safety gain today: that state
was fitted against exactly the feature space this build still computes. So each
package freezes the fingerprint that was current when the envelope landed
(
legacyFeatureHash) and adopts unversioned state only whileFeatureHash()still equals it. The first feature change breaks that equality and unversioned
state is discarded from then on — no cleanup commit, nobody to remember. The
tests covering that path skip themselves the same way.
Deliberate scope
forecast.gonorpvperf, so it does not contend with theroofmodel family (feat(weather): add STRÅNG as an irradiance source and declare source coverage #734, feat(roofmodel): derive PV array geometry from Lantmäteriet open geodata #735, feat(roofmodel): optional shadow-aware irradiance via vostok #736). Land it first and they inherit the
guard; feat(forecast): project irradiance onto each PV array's plane #718 has already merged, but it changed the forecast's PV estimate,
not
forecast.ClearSkyW— the twin's input still means what the string says.typicalPriorshape is deliberately not part of the load model'sidentity. A bucket mean is measured watts and stays meaningful when the prior
it started from is retuned; the prior only supplies the fallback for buckets
nobody has observed. Cold-starting every site over a prior tweak would cost
more than it protects. There is a test asserting the prior does not leak into
the probe.
Beta[0] = 0migration is kept, and tested on both theversioned and the unversioned path.
heatingGainis extracted from the two places that had it inline, so theprobe can cover the shape
HeatingW_per_degCis measured against. Behaviouris identical.
restoreModelasthe per-profile keys, which means it also gets
repairPoisonedBuckets—previously only the per-profile path ran it, and the pre-profile key holds
the oldest, likeliest-poisoned state on any box.
Tests
go/internal/modelstatecovers the envelope: round trip, hash mismatch,unknown schema version, nine flavours of corrupt blob, and the self-retiring
legacy adoption.
Unwrapdecodes into a scratch value and copies over only onsuccess, so a blob damaged halfway through can never leave a model that is part
restored state and part cold start.
Each model package covers the same behaviours end to end through
NewService, plus a pinned-hash tripwire so a feature change cannot shipwithout someone noticing that every deployed site will relearn.
Verified the guard bites: removing the hash comparison in
UnwrapfailsTestStateFittedAgainstOtherFeaturesColdStartsin both packages andTestUnwrapDiscardsOnFeatureHashMismatchinmodelstate; restoring it passes.make verifyclean.🤖 Generated with Claude Code