-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Implement secure horizontal scheme for federated learning #10231
Implement secure horizontal scheme for federated learning #10231
Conversation
…ute under secure scenario
…valent to broadcast
…lobal best split, but need to further apply split correctly
…w histogram transmission data structure of a flat vector
src/processing/processor.h
Outdated
|
||
Processor* load(const std::string& plugin_name); | ||
|
||
void unload(); |
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 couldn't find any reference to this unload method? Is it leaking the handle?
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.
} | ||
|
||
// Save pairs for future operations | ||
this->gh_pairs_ = new std::vector<double>(pairs); |
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.
Is this vector freed?
src/tree/hist/evaluate_splits.h
Outdated
@@ -429,7 +430,7 @@ class HistEvaluator { | |||
all_entries[worker * entries.size() + nidx_in_set].split); | |||
} | |||
} | |||
if (is_secure_) { | |||
if (is_secure_ && is_col_split_) { |
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.
Looking at the code, the is_col_split_
must be true.
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.
ah indeed, thanks!
removed the redundant condition.
Support bool params in proc_params.
@@ -180,47 +221,100 @@ class HistogramBuilder { | |||
|
|||
common::BlockedSpace2d space( | |||
nodes_to_build.size(), [&](std::size_t) { return n_total_bins; }, 1024); | |||
common::ParallelFor2d(space, this->n_threads_, [&](size_t node, common::Range1d r) { | |||
common::ParallelFor2d(space, this->n_threads_, [&](std::size_t node, common::Range1d r) { | |||
// Merging histograms from each thread. | |||
this->buffer_.ReduceHist(node, r.begin(), r.end()); |
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.
Is this reduce hist function call actually useful for secure training?
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 so, this is the multi-thread part for each client
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 thought the histogram is being built in the plugin? If so, any operation for the histogram in XGBoost should be invalid right?
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.
ah you are right!
this reduce hist will have no impact for secure vertical training, it will be needed by secure horizontal, and non-secure federated schemes.
de4013f
into
dmlc:vertical-federated-learning
Implementation for the secure horizontal scheme (RFC #10170).
Modifictions added beyond #10124
Following the same processor interface design with new functions to perform secure histogram aggregation (at external server).