Skip to content

Commit ebb5c27

Browse files
tveaseytveasey
authored
[6.3][ML] Fix bug upgrading state from 5.6 direct to 6.3 causing SIGSEGV on open job (#145)
1 parent a6b0329 commit ebb5c27

9 files changed

+1981
-17
lines changed

‎docs/CHANGELOG.asciidoc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
* Fail start up if state is missing {ml-pull}4[#4]
3535
* Do not log incorrect model memory limit {ml-pull}3[#3]
3636
* The trend decomposition state wasn't being correctly upgraded potentially causing the autodetect process to abort {ml-pull}136[#136] (issue: {ml-issue}135[#135])
37+
* Fix a SIGSEGV in the autodetect process when jump upgrading from 5.6 to 6.3 {ml-pull}143[#143] (issue: {ml-issue}141[#141]})
3738

3839
=== Regressions
3940

‎include/maths/CTimeSeriesDecompositionDetail.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,8 @@ class MATHS_EXPORT CTimeSeriesDecompositionDetail {
350350
};
351351

352352
//! Initialize by reading state from \p traverser.
353-
bool acceptRestoreTraverser(core::CStateRestoreTraverser& traverser);
353+
bool acceptRestoreTraverser(core_t::TTime lastValueTime,
354+
core::CStateRestoreTraverser& traverser);
354355

355356
//! Persist state by passing information to \p inserter.
356357
void acceptPersistInserter(core::CStatePersistInserter& inserter) const;

‎lib/maths/CSeasonalComponentAdaptiveBucketing.cc

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -375,7 +375,11 @@ bool CSeasonalComponentAdaptiveBucketing::acceptRestoreTraverser(core::CStateRes
375375
RESTORE(LAST_UPDATES_OLD_TAG,
376376
core::CPersistUtils::fromString(traverser.value(), lastUpdates))
377377
} while (traverser.next());
378-
378+
// This wasn't present in version 5.6 so needs to be default
379+
// initialised if it is missing from the state object.
380+
if (lastUpdates.empty()) {
381+
lastUpdates.resize(regressions.size(), UNSET_TIME);
382+
}
379383
m_Buckets.clear();
380384
m_Buckets.reserve(regressions.size());
381385
for (std::size_t i = 0u; i < regressions.size(); ++i) {

‎lib/maths/CTimeSeriesDecomposition.cc

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -125,9 +125,9 @@ bool CTimeSeriesDecomposition::acceptRestoreTraverser(core::CStateRestoreTravers
125125
RESTORE(CALENDAR_CYCLIC_TEST_6_3_TAG,
126126
traverser.traverseSubLevel(boost::bind(&CCalendarTest::acceptRestoreTraverser,
127127
&m_CalendarCyclicTest, _1)))
128-
RESTORE(COMPONENTS_6_3_TAG,
129-
traverser.traverseSubLevel(boost::bind(
130-
&CComponents::acceptRestoreTraverser, &m_Components, _1)))
128+
RESTORE(COMPONENTS_6_3_TAG, traverser.traverseSubLevel(boost::bind(
129+
&CComponents::acceptRestoreTraverser,
130+
&m_Components, m_LastValueTime, _1)))
131131
}
132132
} else {
133133
// There is no version string this is historic state.
@@ -140,9 +140,9 @@ bool CTimeSeriesDecomposition::acceptRestoreTraverser(core::CStateRestoreTravers
140140
RESTORE(CALENDAR_CYCLIC_TEST_OLD_TAG,
141141
traverser.traverseSubLevel(boost::bind(&CCalendarTest::acceptRestoreTraverser,
142142
&m_CalendarCyclicTest, _1)))
143-
RESTORE(COMPONENTS_OLD_TAG,
144-
traverser.traverseSubLevel(boost::bind(
145-
&CComponents::acceptRestoreTraverser, &m_Components, _1)))
143+
RESTORE(COMPONENTS_OLD_TAG, traverser.traverseSubLevel(boost::bind(
144+
&CComponents::acceptRestoreTraverser,
145+
&m_Components, m_LastValueTime, _1)))
146146
} while (traverser.next());
147147
this->decayRate(decayRate);
148148
}

‎lib/maths/CTimeSeriesDecompositionDetail.cc

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -312,23 +312,22 @@ const std::string LAST_UPDATE_OLD_TAG{"j"};
312312

313313
const double MODEL_WEIGHT_UPGRADING_TO_VERSION_6_3{48.0};
314314

315-
bool upgradeTrendModelToVersion6p3(const core_t::TTime bucketLength,
316-
CTrendComponent& trend,
317-
core::CStateRestoreTraverser& traverser) {
315+
bool upgradeTrendModelToVersion_6_3(const core_t::TTime bucketLength,
316+
const core_t::TTime lastValueTime,
317+
CTrendComponent& trend,
318+
core::CStateRestoreTraverser& traverser) {
318319
using TRegression = CRegression::CLeastSquaresOnline<3, double>;
319320

320321
TRegression regression;
321322
double variance{0.0};
322323
core_t::TTime origin{0};
323-
core_t::TTime lastUpdate{0};
324324
do {
325325
const std::string& name{traverser.name()};
326326
RESTORE(REGRESSION_OLD_TAG,
327327
traverser.traverseSubLevel(boost::bind(
328328
&TRegression::acceptRestoreTraverser, &regression, _1)))
329329
RESTORE_BUILT_IN(VARIANCE_OLD_TAG, variance)
330330
RESTORE_BUILT_IN(TIME_ORIGIN_OLD_TAG, origin)
331-
RESTORE_BUILT_IN(LAST_UPDATE_OLD_TAG, lastUpdate)
332331
} while (traverser.next());
333332

334333
// Generate some samples from the old trend model.
@@ -337,7 +336,8 @@ bool upgradeTrendModelToVersion6p3(const core_t::TTime bucketLength,
337336
static_cast<double>(bucketLength) / static_cast<double>(4 * WEEK)};
338337

339338
CPRNG::CXorOShiro128Plus rng;
340-
for (core_t::TTime time = lastUpdate - 4 * WEEK; time < lastUpdate; time += bucketLength) {
339+
for (core_t::TTime time = lastValueTime - 4 * WEEK; time < lastValueTime;
340+
time += bucketLength) {
341341
double time_{static_cast<double>(time - origin) / static_cast<double>(WEEK)};
342342
double sample{regression.predict(time_) + CSampling::normalSample(rng, 0.0, variance)};
343343
trend.add(time, sample, weight);
@@ -975,7 +975,9 @@ CTimeSeriesDecompositionDetail::CComponents::CComponents(const CComponents& othe
975975
m_UsingTrendForPrediction{other.m_UsingTrendForPrediction}, m_Watcher{nullptr} {
976976
}
977977

978-
bool CTimeSeriesDecompositionDetail::CComponents::acceptRestoreTraverser(core::CStateRestoreTraverser& traverser) {
978+
bool CTimeSeriesDecompositionDetail::CComponents::acceptRestoreTraverser(
979+
core_t::TTime lastValueTime,
980+
core::CStateRestoreTraverser& traverser) {
979981
if (traverser.name() == VERSION_6_3_TAG) {
980982
while (traverser.next()) {
981983
const std::string& name{traverser.name()};
@@ -1020,8 +1022,8 @@ bool CTimeSeriesDecompositionDetail::CComponents::acceptRestoreTraverser(core::C
10201022
RESTORE_SETUP_TEARDOWN(TREND_OLD_TAG,
10211023
/**/,
10221024
traverser.traverseSubLevel(boost::bind(
1023-
upgradeTrendModelToVersion6p3,
1024-
m_BucketLength, boost::ref(m_Trend), _1)),
1025+
upgradeTrendModelToVersion_6_3, m_BucketLength,
1026+
lastValueTime, boost::ref(m_Trend), _1)),
10251027
m_UsingTrendForPrediction = true)
10261028
RESTORE_SETUP_TEARDOWN(
10271029
SEASONAL_OLD_TAG, m_Seasonal.reset(new SSeasonal),

‎lib/maths/CTrendComponent.cc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,10 @@ void CTrendComponent::forecast(core_t::TTime startTime,
278278
LOG_ERROR(<< "Bad confidence interval: " << confidence << "%");
279279
return;
280280
}
281+
if (!this->initialized()) {
282+
result.resize((endTime - startTime) / step, TDouble3Vec{0.0, 0.0, 0.0});
283+
return;
284+
}
281285

282286
endTime = startTime + CIntegerTools::ceil(endTime - startTime, step);
283287

‎lib/maths/unittest/CTimeSeriesDecompositionTest.cc

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2130,6 +2130,7 @@ void CTimeSeriesDecompositionTest::testUpgrade() {
21302130

21312131
std::string empty;
21322132

2133+
LOG_DEBUG(<< "**** From 6.2 ****");
21332134
LOG_DEBUG(<< "*** Seasonal and Calendar Components ***");
21342135
{
21352136
std::string xml;
@@ -2286,6 +2287,75 @@ void CTimeSeriesDecompositionTest::testUpgrade() {
22862287
decomposition.addPoint(time, 10.0);
22872288
}
22882289
}
2290+
2291+
LOG_DEBUG(<< "**** From 5.6 ****");
2292+
LOG_DEBUG(<< "*** Seasonal and Calendar Components ***");
2293+
{
2294+
std::string xml;
2295+
load("testfiles/CTimeSeriesDecomposition.5.6.seasonal.state.xml", xml);
2296+
LOG_DEBUG(<< "Saved state size = " << xml.size());
2297+
2298+
core::CRapidXmlParser parser;
2299+
CPPUNIT_ASSERT(parser.parseStringIgnoreCdata(xml));
2300+
core::CRapidXmlStateRestoreTraverser traverser(parser);
2301+
2302+
maths::CTimeSeriesDecomposition decomposition(
2303+
0.1, HALF_HOUR, maths::CTimeSeriesDecomposition::DEFAULT_COMPONENT_SIZE, traverser);
2304+
2305+
// Check that the decay rates match and the values and variances
2306+
// predictions match the values obtained from 6.2.
2307+
2308+
CPPUNIT_ASSERT_EQUAL(0.01, decomposition.decayRate());
2309+
2310+
double meanValue{decomposition.mean(18316800)};
2311+
double meanVariance{decomposition.meanVariance()};
2312+
LOG_DEBUG(<< "restored mean value = " << meanValue);
2313+
LOG_DEBUG(<< "restored mean variance = " << meanVariance);
2314+
CPPUNIT_ASSERT_DOUBLES_EQUAL(9.91269, meanValue, 0.005);
2315+
CPPUNIT_ASSERT_DOUBLES_EQUAL(3.99723, meanVariance, 0.5);
2316+
2317+
// Check some basic operations on the upgraded model.
2318+
TDouble3VecVec forecast;
2319+
decomposition.forecast(60480000, 60480000 + WEEK, HALF_HOUR, 90.0, 1.0, forecast);
2320+
for (core_t::TTime time = 60480000; time < 60480000 + WEEK; time += HALF_HOUR) {
2321+
decomposition.addPoint(time, 10.0);
2322+
}
2323+
}
2324+
2325+
LOG_DEBUG(<< "*** Trend and Seasonal Components ***");
2326+
{
2327+
std::string xml;
2328+
load("testfiles/CTimeSeriesDecomposition.5.6.trend_and_seasonal.state.xml", xml);
2329+
LOG_DEBUG(<< "Saved state size = " << xml.size());
2330+
2331+
core::CRapidXmlParser parser;
2332+
CPPUNIT_ASSERT(parser.parseStringIgnoreCdata(xml));
2333+
core::CRapidXmlStateRestoreTraverser traverser(parser);
2334+
2335+
maths::CTimeSeriesDecomposition decomposition(
2336+
0.1, HALF_HOUR, maths::CTimeSeriesDecomposition::DEFAULT_COMPONENT_SIZE, traverser);
2337+
2338+
// Check that the decay rates match and the values and variances
2339+
// predictions are close to the values obtained from 6.2. We can't
2340+
// update the state exactly in this case so the tolerances in this
2341+
// test are significantly larger.
2342+
2343+
CPPUNIT_ASSERT_EQUAL(0.024, decomposition.decayRate());
2344+
2345+
double meanValue{decomposition.mean(10366200)};
2346+
double meanVariance{decomposition.meanVariance()};
2347+
LOG_DEBUG(<< "restored mean value = " << meanValue);
2348+
LOG_DEBUG(<< "restored mean variance = " << meanVariance);
2349+
CPPUNIT_ASSERT_DOUBLES_EQUAL(96.5607, meanValue, 4.0);
2350+
CPPUNIT_ASSERT_DOUBLES_EQUAL(631.094, meanVariance, 7.0);
2351+
2352+
// Check some basic operations on the upgraded model.
2353+
TDouble3VecVec forecast;
2354+
decomposition.forecast(10366200, 10366200 + WEEK, HALF_HOUR, 90.0, 1.0, forecast);
2355+
for (core_t::TTime time = 60480000; time < 60480000 + WEEK; time += HALF_HOUR) {
2356+
decomposition.addPoint(time, 10.0);
2357+
}
2358+
}
22892359
}
22902360

22912361
CppUnit::Test* CTimeSeriesDecompositionTest::suite() {

0 commit comments

Comments
 (0)