Send ML feature information with UpdateNodeInfo. #11913
Merged
Conversation
This file contains 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
We achieve this by adding the `ml_{capable,enabled}` fields in
`system_info`. When streaming, these fields allow a parent to understand if
the child has ML and if it runs ML for itself.
The UpdateNodeInfo includes this information about a child, plus a
boolean that is set to true when the parent runs ML for the child.
vkalintiris
requested review from
stelfrag,
thiagoftsm and
vlvkobal
as code owners
thiagoftsm
reviewed
Dec 20, 2021
|
@vkalintiris the following warning is not associated with this PR, but it has relationship with ML: ml/ml-dummy.c: In function 'ml_get_host_info':
ml/ml-dummy.c:13:50: warning: control reaches end of non-void function [-Wreturn-type]
13 | char *ml_get_host_info(RRDHOST *RH) { (void) RH; } |
|
@vkalintiris last commit does not allow me to compile with the following errors: aclk/schema-wrappers/node_info.cc: In function 'int generate_node_info(nodeinstance::info::v1::NodeInfo*, aclk_node_info*)':
aclk/schema-wrappers/node_info.cc:65:29: error: 'MachineLearningInfo' is not a member of 'nodeinstance::info::v1'
65 | nodeinstance::info::v1::MachineLearningInfo *ml_info = info->mutable_ml_info();
| ^~~~~~~~~~~~~~~~~~~
aclk/schema-wrappers/node_info.cc:65:50: error: 'ml_info' was not declared in this scope; did you mean 'mc_info'?
65 | nodeinstance::info::v1::MachineLearningInfo *ml_info = info->mutable_ml_info();
| ^~~~~~~
| mc_info
aclk/schema-wrappers/node_info.cc:65:66: error: 'class nodeinstance::info::v1::NodeInfo' has no member named 'mutable_ml_info'
65 | nodeinstance::info::v1::MachineLearningInfo *ml_info = info->mutable_ml_info();
| ^~~~~~~~~~~~~~~
aclk/schema-wrappers/node_info.cc: In function 'char* generate_update_node_info_message(size_t*, update_node_info*)':
aclk/schema-wrappers/node_info.cc:93:29: error: 'MachineLearningInfo' is not a member of 'nodeinstance::info::v1'
93 | nodeinstance::info::v1::MachineLearningInfo *ml_info = msg.mutable_ml_info();
| ^~~~~~~~~~~~~~~~~~~
aclk/schema-wrappers/node_info.cc:93:50: error: 'ml_info' was not declared in this scope; did you mean 'mc_info'?
93 | nodeinstance::info::v1::MachineLearningInfo *ml_info = msg.mutable_ml_info();
| ^~~~~~~
| mc_info
aclk/schema-wrappers/node_info.cc:93:64: error: 'class nodeinstance::info::v1::UpdateNodeInfo' has no member named 'mutable_ml_info'
93 | nodeinstance::info::v1::MachineLearningInfo *ml_info = msg.mutable_ml_info(); |
@thiagoftsm this PR updates the aclk-schema submodule. You need to make sure it points to 9fa80e397be191b56320b0cfa7da97b249c3b3eb (ie. netdata/aclk-schemas#28). |
thiagoftsm
approved these changes
Dec 21, 2021
|
Hi @vkalintiris ! First test, with a single (no children) agent, ML enabled, connected to staging with new architecture: |
|
Same node with ML disabled: |
|
This is the UpdateNodeInfo message sent by the parent (winterland) for it's child (6am) to the cloud. Both are compiled with ML but not enabled: |
|
(Sorry for the spamming). Parent enabled ml, child not. Parent's UpdateNodeInfo for itself: Parent's UpdateNodeInfo for it's child: |
|
Both parent and child enabled ML, looks good: |
|
Child compiled with |
|
Same with the last check, if the child is from current master and not from this PR. I think it looks good! @vkalintiris anything else you think we should check? |
Nothing I can think of. |
MrZammler
approved these changes
Dec 22, 2021
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.
Summary
We achieve this by adding the
ml_{capable,enabled}fields insystem_info. When streaming, these fields allow a parent to understand ifthe child has ML and if it runs ML for itself.
A newly added protobuf message contains the aforementioned field. Inside
NodeInfothe information is an exact copy ofsystem_info. InsideUpdateNodeInfo the
ml_capablefield reflects the ability of the localhostto run ML, and the
ml_enabledfield denotes the training status of thespecified node instance.
Component Name
aclk
Test Plan
TBD
Additional Information
Setting the
ml_{capable,enabled}fields insiderrdhost_createis a little bitawkward. The reason we have to do this is because there's an ML configuration
option that allows a user to filter by hostname the nodes to be trained. This means
that we need to create/initialize the
RRDHOSTobject first and then check if theML component will train it based on the configuration options of the user.