[ML] Refactor model snapshot writing to use concurrent writer #12

dimitris-athanasiou · 2018-03-13T16:03:35Z

This refactors the writing of the model snapshot document and
model size stats document out of CJsonOutputWriter.

It achieves the following improvements:

model snapshot documents do not have to be queued up. They
can be written directly leveraging the safe concurrent writer
that was implemented for forecasts. This minimizes the delay
of a model snapshot being written which in failure cases may
result into having a more recent snapshot available.
better separation of concerns and a smaller CJsonOutputWriter class.
simplifies adding new fields in the model snapshot document as
needed for writing the minimum compatible version.

This refactors the writing of the model snapshot document and model size stats document out of CJsonOutputWriter. It achieves the following improvements: - model snapshot documents do not have to be queued up. They can be written directly leveraging the safe concurrent writer that was implemented for forecasts. This minimizes the delay of a model snapshot being written which in failure cases may result into having a more recent snapshot available. - better separation of concerns and a smaller CJsonOutputWriter class. - simplifies adding new fields in the model snapshot document as needed for writing the minimum compatible version.

tveasey

I think this is a good idea. Left some additional suggestions and corrections for typos.

tveasey · 2018-03-13T16:54:58Z

include/api/CModelSnapshotJsonWriter.h

+                                 const model::CResourceMonitor::SResults &modelSizeStats,
+                                 const std::string &normalizerState,
+                                 core_t::TTime latestRecordTime,
+                                 core_t::TTime latestFinalResultTime);


Just a thought, but you don't actually need to provide a constructor for this pod. You can just use a list initialiser and provide the arguments in the order in which they are declared in the object, i.e. SModelSnapshotReport report{time, description, ...}. The only downside is if someone reorders the fields in the class definition, but this should generally fail to compile since the types must be convertible to one another. That would be an odd thing to do as well.

tveasey · 2018-03-13T16:59:34Z

include/api/CModelSizeStatsJsonWriter.h

+        static const std::string BUCKET_ALLOCATION_FAILURES_COUNT;
+        static const std::string MEMORY_STATUS;
+        static const std::string TIMESTAMP;
+        static const std::string LOG_TIME;


These don't appear to be needed outside of this object. In which case, I generally prefer to just define them in an unnamed namespace in the cc file. It means if we want to write new fields we don't have to touch the header and also tidies up the class definition.

tveasey · 2018-03-13T17:00:31Z

include/api/CModelSizeStatsJsonWriter.h

+                          core::CRapidJsonConcurrentLineWriter &writer);
+};
+
+


Extra new line.

tveasey · 2018-03-13T17:00:40Z

include/api/CModelSnapshotJsonWriter.h

+        static const std::string LATEST_RECORD_TIME;
+        static const std::string LATEST_RESULT_TIME;
+        static const std::string QUANTILES;
+        static const std::string QUANTILE_STATE;


tveasey · 2018-03-13T17:01:51Z

lib/api/CModelSizeStatsJsonWriter.cc

+    writer.EndObject();
+}
+
+


Extra new line.

tveasey · 2018-03-13T17:04:45Z

lib/api/CModelSizeStatsJsonWriter.cc

+    writer.String(print(results.s_MemoryStatus));
+
+    writer.String(TIMESTAMP);
+    writer.Int64(results.s_BucketStartTime * 1000);


There are quite a few places where we magically multiply times by 1000 sometimes (correctly) casting to int64_t and sometimes not. Might be nice to add a utility function which does this; maybe in core::CPersistUtils.

Maybe static int64_t core::CTimeUtils::toJavaTimestamp(core_t::TTime)?

Or alternatively static int64_t core::CTimeUtils::toEpochMs(core_t::TTime).

Happy to do this but I'll open another PR for it.

Yes, that's a better suggestion. I up vote static int64_t core::CTimeUtils::toEpochMs(core_t::TTime).

Also, makes sense to defer to separate PR. There may well be other cases of this as well.

The only drawback of the add{String/Int/Uint/..}FieldToObj methods is that you have to construct an extra layer of C++ objects to represent the JSON document in memory before writing it as a text JSON document. We do this in cases where we need to return to the documents later to add extra fields. But in cases where a single document can be written start-to-end in one method it's an unnecessary overhead. For documents written as infrequently as model snapshots I don't think the performance difference will be measurable, but I wouldn't want to set a precedent that we only write JSON by constructing documents in memory.

Good point! (So I could simplify writing forecast results, too.)

What about a writer.Time(int64_t time) (to be implemented in CRapidJsonWriterBase.h). I still like the idea of moving the logic into the writer as we have addTimeFieldToObj already. IMHO that is more consistent to that.

Yes, sure we could have writer.Time(core_t::TTime time) and then all the methods that multiply by 1000 could encapsulate that logic by deferring to int64_t core::CTimeUtils::toEpochMs(core_t::TTime). That will make it easier if we ever change the time units we use in the C++ code.

👍 exactly

(I was only unsure if core_t::TTime or int64_t as the writer class is somewhat already in rapidjson space, but its hard to draw the line, so core_t::TTime is fine)

tveasey · 2018-03-13T17:07:26Z

lib/api/CModelSizeStatsJsonWriter.cc

+/*
+ * ELASTICSEARCH CONFIDENTIAL
+ *
+ * Copyright (c) 2016 Elasticsearch BV. All Rights Reserved.


2018, although not sure how this will interact with changes to license header.

tveasey · 2018-03-13T17:08:02Z

include/api/CModelSnapshotJsonWriter.h

+/*
+ * ELASTICSEARCH CONFIDENTIAL
+ *
+ * Copyright (c) 2016 Elasticsearch BV. All Rights Reserved.


2018, although not sure how this will interact with changes to license header.

tveasey · 2018-03-13T17:08:10Z

include/api/CModelSizeStatsJsonWriter.h

+/*
+ * ELASTICSEARCH CONFIDENTIAL
+ *
+ * Copyright (c) 2016 Elasticsearch BV. All Rights Reserved.


2018, although not sure how this will interact with changes to license header.

hendrikmuhs · 2018-03-13T20:31:53Z

include/api/CModelSizeStatsJsonWriter.h

+
+#include <core/CRapidJsonConcurrentLineWriter.h>
+
+#include <core/CNonInstantiatable.h>


combine with 2 lines above and sort?

Also, these should be in lexicographical order.

hendrikmuhs · 2018-03-13T20:58:58Z

lib/api/unittest/CBackgroundPersisterTest.cc

-    LOG_DEBUG("Persist complete with description: " << description);
-    snapshotIdOut = snapshotIdIn;
-    numDocsOut = numDocsIn;
+    LOG_INFO("Persist complete with description: " << modelSnapshotReport.s_Description);


you raised the log level, by intent or debug leftover?

hendrikmuhs · 2018-03-13T20:59:56Z

lib/api/unittest/CRestorePreviousStateTest.cc

-    LOG_DEBUG("Persist complete with description: " << description);
-    snapshotIdOut = snapshotIdIn;
-    numDocsOut = numDocsIn;
+    LOG_INFO("Persist complete with description: " << modelSnapshotReport.s_Description);


DEBUG->INFO ?

dimitris-athanasiou · 2018-03-14T11:21:50Z

All comments have been addressed. Could you guys have another look?

hendrikmuhs

LGTM

tveasey

LGTM

and remove some blank lines

This refactors the writing of the model snapshot document and model size stats document out of CJsonOutputWriter. It achieves the following improvements: - model snapshot documents do not have to be queued up. They can be written directly leveraging the safe concurrent writer that was implemented for forecasts. This minimizes the delay of a model snapshot being written which in failure cases may result into having a more recent snapshot available. - better separation of concerns and a smaller CJsonOutputWriter class. - simplifies adding new fields in the model snapshot document as needed for writing the minimum compatible version.

dimitris-athanasiou added v7.0.0 v6.3.0 review :ml >refactoring labels Mar 13, 2018

dimitris-athanasiou requested a review from hendrikmuhs March 13, 2018 16:03

tveasey reviewed Mar 13, 2018

View reviewed changes

Address Toms review comments

2cb5028

hendrikmuhs reviewed Mar 13, 2018

View reviewed changes

Fix includes order and some log levels

d4f7f32

hendrikmuhs approved these changes Mar 14, 2018

View reviewed changes

tveasey approved these changes Mar 14, 2018

View reviewed changes

Move more string constants in local namespace

3d48e68

and remove some blank lines

dimitris-athanasiou merged commit af650fd into elastic:master Mar 14, 2018

dimitris-athanasiou deleted the add-model-snapshot-min-version branch March 14, 2018 12:21

davidkyle mentioned this pull request Jun 20, 2023

[NLP] Catch exceptions thrown during inference and report as errors #2542

Merged


		#include <core/CRapidJsonConcurrentLineWriter.h>

		#include <core/CNonInstantiatable.h>

[ML] Refactor model snapshot writing to use concurrent writer #12

[ML] Refactor model snapshot writing to use concurrent writer #12

Uh oh!

Conversation

dimitris-athanasiou commented Mar 13, 2018

Uh oh!

tveasey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tveasey Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimitris-athanasiou commented Mar 14, 2018

Uh oh!

hendrikmuhs left a comment

Choose a reason for hiding this comment

Uh oh!

tveasey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tveasey Mar 13, 2018 •

edited

Loading