-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML] Instrument Data Frame Analysis Job to report state information to Java backend #906
Conversation
# Conflicts: # include/maths/CBoostedTreeImpl.h
for (const auto& model : m_Models) { | ||
model.addOutlierScores(points, scores, m_RecordMemoryUsage); | ||
state.nextStep(step++); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, this constitutes a single step of the outlier detection job. WDYT @tveasey ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. This seems like a natural place to me as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I've done a first pass through. I really like the design here. I've left some minor comments. My two larger comments are I have some reservations about the naming and it feels like CDataFrameAnalysisStateInterface
is superfluous (and it's slightly odd it is a concrete type). I'd rejig things slightly to remove and factor out CStubAnalysisState
and use throughout the unit tests. Overall good job!
|
||
private: | ||
SInternalState m_InternalState; | ||
core::CConcurrentQueue<SInternalState, 10> m_StateQueue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is going to be a block if we try and push more than 10 state documents. Since we pop at fixed intervals in time we need to be aware of this behaviour. For example, if this gets updated many times it will potentially mean the working threads are blocked. I don't think this will cause a problem as it stands, but think we should document this behaviour in the class description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added that elements can be dropped if the queue is full.
for (const auto& model : m_Models) { | ||
model.addOutlierScores(points, scores, m_RecordMemoryUsage); | ||
state.nextStep(step++); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. This seems like a natural place to me as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working through those comments. I've done another pass. It now seems to me that we can ditch the queue altogether: as currently implemented it isn't serving a purpose, but in fact the concurrent line writer avoids the need for any synchronisation that this would be used for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has come together well. The unit test failure is caused by requiring that the training is instrumented. I think this is too severe. Aside from that it's LGTM.
# Conflicts: # lib/api/unittest/CDataFrameAnalyzerFeatureImportanceTest.cc
# Conflicts: # docs/CHANGELOG.asciidoc # include/maths/CBoostedTreeFactory.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment. Otherwise, LGTM. Let's get this in!
…o Java backend (elastic#906) In this PR I introduce a new class CDataFrameAnalysisState, which is responsible for centralized collection of statistics related to the data frame analytics jobs, i.e. progress, memory usage, etc. Hence, the related variables are also moved out of CDataFrameAnalysisRunner class. Right now we have two callback functions related to the job state: progressRecorder and memoryUsageRecorder. However, since we can expect that every new kind of statistics (quality of results, time, parameters) would require another update callback function in maths classes, I pass the reference to the State object. To avoid dependency from maths to api, I introduce a state interface class owned by maths.
In this PR I introduce a new class
CDataFrameAnalysisState
, which is responsible for centralized collection of statistics related to the data frame analytics jobs, i.e. progress, memory usage, etc. Hence, the related variables are also moved out ofCDataFrameAnalysisRunner
class.Right now we have two callback functions related to the job state:
progressRecorder
andmemoryUsageRecorder
. However, since we can expect that every new kind of statistics (quality of results, time, parameters) would require another update callback function inmaths
classes, I pass the reference to theState
object. To avoid dependency frommaths
toapi
, I introduce a state interface class owned bymaths
.Related to #976