-
-
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
[Breaking] Remove rabit support for custom reductions and grow_local_histmaker
updater
#7992
Conversation
Not sure if this is the best approach, please take a look and let me know what you think. Next step I need to do some deeper refactoring to get rid of |
@trivialfis @RAMitchell turns out I'm not sure why the R tests are failing. Is it caused by this change? |
The R test is unrelated. I'm trying to unblock the CI. |
I'm happy to remove the hist maker. It's old code to implement global_approx and local_approx. I rewrote the former but the latter was kept in this hist_maker. We don't usually use it but I mentioned it in the doc https://github.com/dmlc/xgboost/blob/master/doc/treemethod.rst#approximated-solutions so it's a breaking change non the less. Please annotate the PR with |
grow_local_histmaker
updater
@trivialfis done. |
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.
LGTM. Please rebase to master branch.
Right now cpu tree methods rely on
rabit::Reducer
to perform custom reductions, but in reality they are just sums of gradients and hessians. Neither NCCL nor the new federated learning gRPC supports custom reductions, so removing this would move us closer to a common interface.Additionally
rabit::SerializeReducer
supports reduction on arbitrary data structure, but it's only used byupdater_histmaker.cc
, which is no longer being referenced anywhere in the code base, so both are removed.